Bug 1004089

IMO the "if self.cache == 1" branch in _getFile is just a leftover, and it should never run when the local file exists. Eg. getPackage() or yumdownloader handle it, and it does the checksum stuff, too.  I can't reproduce this bug, have you any idea why this yumdownloader code didn't run? Can you check that the file size indeed matches?

                local = os.path.join(opts.destdir, local)
                if (os.path.exists(local) and
                    os.path.getsize(local) == int(download.returnSimple('packagesize'))):
                    self.logger.error("%s already exists and appears to be complete" % local)
                    continue

It might be possible that the local file is actually larger, in this case we should unlink it (as we do in getPackage or retriveveMD).

Good call on the local file being larger, turns out most RPMs in my local cache have a second copy of the 8 xz footer magic bytes. I'm still investigating how that happened, but that's most likely unrelated to this bug.

yumdownloader does use getPackage(), but the latter only checks the system cache via _preload_pkg_from_system_cache(), so it wouldn't have any impact on files in the non-default destination directory used by yumdownloader.

Maybe the verifyLocalPkg() call in getPackage() can be decoupled from _preload_pkg_from_system_cache() so that it would check the local file even if system cache was missed? That way, the code you quoted (which is essentially an inferior duplication of functionality of verifyLocalPkg()) wouldn't be necessary.

Not quite.  getPackage() calls YumBase.verifyPkg().. This figures out that checksum does not match, and the following code runs:

            cursize = os.stat(fo)[6]
            totsize = long(po.size)
            if cursize >= totsize and not po.repo.cache:
                # if the path to the file is NOT inside the cachedir then don't
                # unlink it b/c it is probably a file:// url and possibly
                # unlinkable
                if fo.startswith(po.repo.cachedir):
                   ^^^^^^^^^^^^^^
                    os.unlink(fo)

The root of the problem is that since "fo" does not point to the cachedir, we don't dare to unlink it (it might be a nfs-mounted file:// repository). I can fix this test upstream, but EL6 uses very different yumdownloader.py, it'd be much easier to just handle the "os.path.getsize(local) > packagesize" case here.

--- yum-utils-1.1.30/yumdownloader.py.old	2013-09-09 15:39:21.451378664 +0200
+++ yum-utils-1.1.30/yumdownloader.py	2013-09-09 15:46:35.099294723 +0200
@@ -241,10 +241,13 @@ class YumDownloader(YumUtilBase):
                 if not os.path.exists(opts.destdir):
                     os.makedirs(opts.destdir)
                 local = os.path.join(opts.destdir, local)
-                if (os.path.exists(local) and 
-                    os.path.getsize(local) == int(download.returnSimple('packagesize'))):
+                c = os.path.exists(local) and cmp(os.path.getsize(local), int(download.returnSimple('packagesize')))
+                if c == 0:
                     self.logger.error("%s already exists and appears to be complete" % local)
                     continue
+                elif c == 1:
+                    self.logger.error("%s already exists and over the size. Unlinking." % local)
+                    yum.misc.unlink_f(local)
                 # Disable cache otherwise things won't download
                 repo.cache = 0
                 download.localpath = local # Hack: to set the localpath we want.

Testing the current behaviour with yum-utils-1.1.20-17 I am getting:

1. for local file smaller than the download:
Package does not match intended download. Suggestion: run yum clean metadata 

the message is not very descriptive and yum clean metadata won't help here.
Something like: rpm already exists but doesn't match the intended download but be better I guess

2. for already downloaded file:
xy.rpm already downloaded and appears to be complete

This is IMO OK

3. for a local file larger than the download:
The requested URL returned error: 416 Requested Range Not Satisfiable.


Could you please clarify how this is supposed to be fixed? I guess that the best option for 1. and 3. would be something similar to 2., e.g. just write that the local copy already exists but doesn't match

Please set needinfo to concrete people, it's more likely you get answers that way. Directing to Valentina ...

This is supposed to work pretty much like in upstream, so in cases 1 and 3 the existing file will be deleted and a new one will be downloaded.

By the way, the above patch is buggy, here is the fixed version:
--- /usr/bin/yumdownloader	2014-02-03 10:03:04.000000000 +0100
+++ ./yumdownloader	2014-03-28 16:44:37.976004627 +0100
@@ -241,10 +241,14 @@ class YumDownloader(YumUtilBase):
                 if not os.path.exists(opts.destdir):
                     os.makedirs(opts.destdir)
                 local = os.path.join(opts.destdir, local)
-                if (os.path.exists(local) and 
-                    os.path.getsize(local) == int(download.returnSimple('packagesize'))):
+                path_exists = os.path.exists(local)
+                size_diff = path_exists and cmp(os.path.getsize(local), int(download.returnSimple('packagesize')))
+                if path_exists and size_diff == 0:
                     self.logger.error("%s already exists and appears to be complete" % local)
                     continue
+                elif size_diff != 0:
+                    self.logger.error("%s already exists, but the size doesn't match. Unlinking." % local)
+                    yum.misc.unlink_f(local)
                 # Disable cache otherwise things won't download
                 repo.cache = 0
                 download.localpath = local # Hack: to set the localpath we want.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2014-1411.html