Re: [PATCH] 473260 - unmount swap files on upgrades

[David Cantrell <dcantrell redhat com>]

Chris Lumens wrote:
>>> +        # path is something stupid like instPath+"swap", which is totally
>>> +        # useless.  But we don't have instPath anywhere else so trim off
>>> +        # the end of path.
>>> +        swapFile = path[:path.rfind("/")] + device.device
>> I'm not quite following what this line does.  Is it doing a dirname on
>> path and then adding device.device to it?  I guess I don't understand
>> why this is necessary, or why we can't use realpath() to normalize it
>> before sending it to swapoff.
> 
> path is "/mnt/sysimage/swap".  device.device is "/var/swap".  We need to
> combine the two to make "/mnt/sysimage/var/swap".  dirname would
> definitely be smarter here.  Can you tell it was way at the end of the
> day when I was writing this?

Ah, I get it after seeing some sample input.

swapFile = os.path.realpath(os.path.dirname(path) + device.device)

>>> +
>>> +        iutil.execWithRedirect("swapoff", [swapFile],
>>> +                               stdout="/dev/tty5", stderr="/dev/tty5",
>>> +                               searchPath=1)
>>>  
>> Do we care about swapoff failing and notifying the caller?
> 
> We probably should, yes, but we do a pretty bad job of catching
> exceptions from other places where umount can fail so I didn't consider
> it to be too important.  Easy enough to fix.

If it's easy enough to fix, I say add it in.  Otherwise, I imagine when
we start touching all of the partitioning code, this will get cleaned up.

Other than those things, looks good.

-- 
David Cantrell <dcantrell redhat com>
Red Hat / Honolulu, HI