Re: [PATCH] Rewrite iscsi code using libiscsi

[Hans de Goede <hdegoede redhat com>]

Chris Lumens wrote:
> Okay, I'll be the picky one.
>
> > diff --git a/backend.py b/backend.py
> > index 3008671..06f6c2b 100644
> > --- a/backend.py
> > +++ b/backend.py
> > @@ -79,9 +79,20 @@ class AnacondaBackend:
> >                  log.error("Could not copy firmware file %s: %s" % (f,  
> > e.strerror))
> >
> >      def doPostInstall(self, anaconda):
> > +        has_iscsi_disk = False
> > +
> > +        # See if we have an iscsi disk. If we do we rerun mkinitrd, as
> > +        # the initrd might need iscsi-initiator-utils, and chances are
> > +        # it was not installed yet the first time mkinitrd was run, as
> > +        # mkinitrd does not require it.
> > +        for disk in anaconda.id.diskset.disks.keys():
> > +            if isys.driveIsIscsi(disk):
> > +                has_iscsi_disk = True
>
> Don't you want to break here after setting has_iscsi_disk=True, since
> there's no need to keep scanning?

Fixed.

> > @@ -222,27 +114,38 @@ def randomIname():
> >          s += dig[random.randrange(0, 32)]
> >      return s
> >
> > +def stabilize(intf = None):
> > +    # Wait for udev to create the devices for the just added disks
> > +    if intf:
> > +        w = intf.waitWindow(_("Scanning iSCSI nodes"),
> > +                            _("Scanning iSCSI nodes"))
> > +    # It is possible when we get here the events for the new devices
> > +    # are not send yet, so sleep to make sure the events are fired
> > +    time.sleep(2)
> > +    iutil.execWithRedirect("/sbin/udevadm", [ "settle" ],
> > +                           stdout = "/dev/tty5", stderr="/dev/tty5")
>
> In general, we don't want to specify the path to a program when we use
> execWith*.  Instead, you just want to call "udevadm" and pass
> searchPath=1.

Fixed.

> > diff --git a/kickstart.py b/kickstart.py
> > index eafd4d8..9305ef0 100644
> > --- a/kickstart.py
> > +++ b/kickstart.py
> > @@ -313,9 +313,6 @@ class IscsiName(commands.iscsiname.FC6_IscsiName):
> >          retval = commands.iscsiname.FC6_IscsiName.parse(self, args)
> >
> >          self.handler.id.iscsi.initiator = self.iscsiname
> > -        self.handler.id.iscsi.startIBFT()
> > -        # FIXME: flush the drive dict so we figure drives out again
> > -        isys.flushDriveDict()
> >          return retval
> >
> >  class Keyboard(commands.keyboard.FC3_Keyboard):
>
> Are any kickstart syntax changes needed for this new iscsi stuff?

No

> > diff --git a/rescue.py b/rescue.py
> > index 2ae35e6..42e5b96 100644
> > --- a/rescue.py
> > +++ b/rescue.py
> > @@ -205,6 +205,7 @@ def runRescue(anaconda, instClass):
> >              else:
> >                  break
> >
> > +        anaconda.intf = None
> >          screen.finish()
> >
> >      # Early shell access with no disk access attempts
>
> I'm not sure what the point of this one is.  Could you explain?

We create an Rescue interface in top of screen, and then destroy screen (by 
calling screen.finish(), so the Rescue interface we have can no longer be used, 
notice if we need an interface again below, we recreate it.

Since the new iscsi code can use anaconda.intf in the rescue path (while making 
ibft disks available in the rescue environment), it is important that 
anaconda.intf does not reference to the Rescueinterface using the no longer 
valid screen object.

> I'm having a little trouble following the meat of this patch, which is
> probably just due to the fact that sometimes diff makes a colossal mess
> of things.  Could you either post the full iscsi.py somewhere or break
> it into smaller chunks?  Thanks.

Sure, here it is:
http://fpaste.org/paste/2983

This includes the execWithRedirect fix you asked for.

Regards,

Hans