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

Re: [PATCH 04/05] add common iscsi.py module



On Fri, 2006-02-03 at 14:04 -0800, Patrick Mansfield wrote:
> Add the iscsi.py module that has common code used by the iscsi gui and text
> install.

This will also probably want to change a bit with the refactoring that's
in progress.  

Some comments on things that could be nicer/cleaner in-line.

Generally, we try to avoid os.popen and do iutil.execWithCapture
instead, mostly for consistency purposes (and some more historical bugs)
and iutil.execWithRedirect instead of system().  

> diff -uprN -X /home/patman/dontdiff anaconda-10.91.12/iscsi.py iscsi-anaconda-10.91.12/iscsi.py
> --- anaconda-10.91.12/iscsi.py	1969-12-31 16:00:00.000000000 -0800
> +++ iscsi-anaconda-10.91.12/iscsi.py	2006-02-02 12:53:06.000000000 -0800
[snip]

> +    def login_out(self, action):
[snip]
> +        command = ("%s -m node" % (ISCSIADM, ))
> +        log.info("Running %s" % (command, ))
> +        records = os.popen(command, 'r').readlines()

Should use execWithCapture instead

> +        regex = re.compile('\[ ( [^\]]* ) ].*', re.VERBOSE)
> +        for i in records:
> +            recnum = regex.sub(r'\1', i)
> +            recnum = re.sub('\n', '', recnum)

This should be able to be more python-y.  Something like the following
probably works (not having looked exactly).
  for line in records:
    recnum = line.split()[0][1:-1]
  
> +            command = ("%s -m node -r %s %s" % (ISCSIADM, recnum, action))
> +            log.info("Running %s" % (command, ))
> +            os.system(command)

Should use execWithRedirect so that we can have a log of the output

> +    def shutdown(self):
[snip]
> +        # Note that iscsid has/had code to ignore 2 (SIGINT), hence the
> +        # 9 (SIGKILL).

If we run iscsid with -p and save the pid in /tmp, then we can have a
much more robust kill.  

> +    def startup(self):
> +        log.debug("Setting up %s" % (INITIATOR_FILE, ))
> +        if os.path.exists(INITIATOR_FILE):
> +            os.unlink(INITIATOR_FILE)
> +        fd = os.open(INITIATOR_FILE, os.O_RDWR | os.O_CREAT)
> +        os.write(fd, "InitiatorName=")
> +        os.write(fd, self.initiator)
> +        os.write(fd, "\n")
> +        os.close(fd)

Should just do this as a single
  os.write(fd, "InitiatorName=%s\n" %(self.initiator,))
instead of splitting it up into multiple calls.

> +        log.info("Starting %s" % (ISCSID_BIN, ))
> +        os.system(ISCSID_BIN)

execWithRedirect instead

> +        command = ("%s -m discovery -t st -p %s:%s" 
> +            % (ISCSIADM, self.ipaddr, self.port)) 
> +        log.info("Running %s" % (command, ))
> +        os.system(command)

The same

> +        self.login_out("--login")

Maybe it makes more sense to call this method "action"?

Jeremy


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