[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: PATCH fix 510970, 529551, 530541



Hi Jerry et all,

Below is a review of your patch:


diff -up ./backend.py.orig ./backend.py
--- ./backend.py.orig	2009-11-16 17:58:30.000000000 -0600
+++ ./backend.py	2009-11-16 18:00:15.000000000 -0600
@@ -153,29 +153,45 @@ class AnacondaBackend:
         if not flags.setupFilesystems:
             return

-        if self._loopbackFile and os.path.exists(self._loopbackFile):
-            return
+        log.info("mediaDevice is %s" % anaconda.mediaDevice )
+        # If they booted with a boot.iso, just continue using that install.img.

-        # If we've booted off the first CD/DVD (so, not the boot.iso) then
-        # copy the install.img to the filesystem and switch loopback devices
-        # to there.  Otherwise we won't be able to unmount and swap media.
-        if not anaconda.mediaDevice or not os.path.exists(installimg):
+        if os.path.exists("/mnt/stage2/images/install.img"):
+            log.info("Don't need to transfer stage2 image")
             return


This is not correct, we may need to unmount stage2 when it is an install CD / DVD
as we may need to change DVD / CD as the needed packages may be split over multiple
disks.

-
-        free = anaconda.id.storage.fsFreeSpace
+
+        log.info("Using %s as stage2 image" % installimg)
         self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
-                                                             free[0][0])
+                                                               "/tmp")
+        log.info("Full image path is %s" % self._loopbackFile )
+

So you're hardcoding the use of /tmp here, but there are no ideas /tmp is a
good candidate in case of an upgrade it might even be a tmpfs for people who
have modified their fstab to make it such.

You are on to something here, the old code is using the first
element of anaconda.id.storage.fsFreeSpace, which is sorted by size, but
that is the element with the smallest size! I think this is a bug and the
code should use the last element

+        if self._loopbackFile and os.path.exists(self._loopbackFile):
+            log.info("and exists and removing %s" % self._loopbackFile )
+            try:
+                os.unlink(self._loopbackFile)
+            except:
+                pass
+

Not sure what you are trying to do here, self._loopbackFile is None (so false)
the first time we go through this code path, and if we go through this code
path multiple times, it should be a nop the second time (see the
check for this you removed above).

+        # This s/b the one that was copied into /tmp ....
+        # Why was this copied to ram when you have passed stage2=
+        # lets free the RAM
+        if os.path.exists("/tmp/install.img"):
+            log.info("OVERRIDE Using /tmp/install.img as stage2 image")
+            stage2img = "/tmp/install.img"
+            stage2ram = 1
+        else:
+            stage2img = installimg

Ok, you've alsmost completely lost me here, I have some clue what
you are trying to do, but it is completely unrelated to the previous parts
of this patch.

Please make *ONE* change per patch, and add a lot more verbose description
of the why, what and how of the patch. So put in the description:

1) What you are trying to fix
2) What is the current behaviour (if applicable in various scenarios)
3) What are you changing and how does this fix this.
4) How does this change impact other install scenarios ?


         try:
             win = anaconda.intf.waitWindow(_("Copying File"),
-                    _("Transferring install image to hard drive"))
-            shutil.copyfile(installimg, self._loopbackFile)
+                   _("Transferring install image to hard drive..."))
+            shutil.copyfile(stage2img, self._loopbackFile)
             win.pop()
         except Exception, e:
             if win:
                 win.pop()

-            log.critical("error transferring install.img: %s" %(e,))
+            log.critical("error transferring stage2img: %s" %(e,))

             if isinstance(e, IOError) and e.errno == 5:
                 msg = _("An error occurred transferring the install image "
@@ -195,7 +211,21 @@ class AnacondaBackend:
             return 1

         isys.lochangefd("/dev/loop0", self._loopbackFile)
-        isys.umount("/mnt/stage2")
+
+        # install.img is now in /tmp get rid of the original in boot
+        if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
+            log.info("REMOVING install.img from /mnt/sysimage/boot/upgrade")
+            os.unlink("/mnt/sysimage/boot/upgrade/install.img")
+
+        if stage2ram:
+            try:
+                os.unlink(stage2img)
+            except:
+                pass
+        try:
+            isys.umount("/mnt/stage2")
+        except:
+            pass

     def removeInstallImage(self):
         if self._loopbackFile:


diff -up ./yuminstall.py.orig ./yuminstall.py
--- ./yuminstall.py.orig	2009-11-16 17:59:45.000000000 -0600
+++ ./yuminstall.py	2009-11-16 18:00:15.000000000 -0600
@@ -625,6 +625,9 @@ class AnacondaYum(YumSorter):


     def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
+        stage2img = "%s/images/install.img" % self.tree
+        self.anaconda.backend.mountInstallImage(self.anaconda, stage2img)
+
         if hasattr(self, "preconf"):
             self.preconf.fn = fn
             self.preconf.root = root
@@ -828,12 +831,6 @@ class AnacondaYum(YumSorter):
         mkeys = self.tsInfo.reqmedia.keys()
         mkeys.sort(mediasort)

-        if len(mkeys) > 1:
-            stage2img = "%s/images/install.img" % self.tree
-            if self.anaconda.backend.mountInstallImage(self.anaconda, stage2img):
-                self.anaconda.id.storage.umountFilesystems()
-                return DISPATCH_BACK
-
         for i in mkeys:
             self.tsInfo.curmedia = i
             if i > 0:


Again, this seems yet another changed put in one and the same patch without any
explanation. Please split this patch up and thoroughly explain the reasoning behind
the patch. Also it seems wrong, as you are now transfering the image to HD, even when
we are doing a single dvd install, eating all that precious hard disk space you
are trying to avoid us running out of.

Regards,

Hans


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]