util-linux/umount.c umount error: Invalid argument

[Christian Melki]

> On Thursday 17 August 2006 7:42 am, Christian Melki wrote:
> > Hi.
> > 
> > Im looking at umount.c
> 
> /me runs, hides...
> 
> > and tracing output from the busybox umount to find  
> > that busybox umount uses a expanded version of "path" used at the
> command 
> > line to umount whatever target we are looking at.
> 
> http://busybox.net/downloads/patches/svn-13880.patch
> 
> > This leads to problems that I have seen a few people 
> > have. "umount /dev/sda1" -> -1: Invalid argument from the call
> oldumount.
> > Shouldn't the code use m->dir in the places of "path" in the umount? 
> 
> If /etc/mtab exists, yes.  But if /etc/mtab doesn't exist, it still
> needs to 
> umount something.

Ok. Hmm.. I guess it depends on how nice you want to be against the user..
My idea of "umount flow" would be something like this.. 
1. Parse path from commandline, expand it. quit if fail.
2. Read mtab if it exists (we might be nice and check for proc/mounts
(ugly) if the user forgot to symlink /etc/mtab). If we dont find mtab (or
proc/mounts), continue with a warning assuming the user is keeping state
himself, knowing the exact mount path, double mounts etc (not devices).
3. Assuming -a, and assuming the list is  >= 1, loop through all mount
paths, umounting them with umount or umount2 if used with flags, else just
umount or umount2 the lookup of "path" in the parsed list of "devices" |
"paths" as it does now and using the true "dir" in mtab as umount argument.

Of course this has to be done while paying attention to loop devices etc.. 
Btw. Isn't there a way to check for a loopdev instead of just issuing the
ioctl?

> > Umount should look up mtab, either maintained or symlinked to kernel 
> > parse it, match "path" for both dir and device and if it finds a 
> > appropriate entry, use the "dir" part of that entry to umount it.
> > This is not the case.
> 
> let's see, if you're doing a "umount /mnt" what happens:
> 
> parse arguments
> line 60: open /etc/mtab and read contents into mtl.  This leaves "m" 
> initialized to the last element in the list, but the list has been
> reversed 
> so this is the start of the list for traversal purposes.  If there was 
> no /etc/mtab, then m and mtl will be null.
> line 73: If ! -a, null out m and make sure we've got at least one
> argument.
> 
> line 83 (in the loop starting on line 79): if m is set then we're in
> the -a 
> case and there was an /etc/mtab.  Not relevant here.
> line 85: check for -a, and if we get here we hit EOF on /etc/mtab. 
> Again not 
> relevant.
> Line 87: Stop if we're out of command line arguments.  Again, not
> relevant.
> line 89: play with path.  Where did path come from?  Set directly from
> argv...  
> And in this case, that's what winds up in the umount() calls.
> 
> Right, we need a new variable...

Or just checking that it isn't empty when reaching the umount calls?
Hmm.. Maybe i just misunderstood you..

> > It will parse and match the "path" from mtab. But when calling the
> actual 
> > umount it will use the "path" which is the /dev/sda1 from
> commandline.
> > I patched umount and used m->dir instead of path in umounts and it 
> > started behaving as expected.
> 
> I wouldn't call it simple, but most of the logic's already there.  We
> have to 
> make sure we're matching the _last_ entry in the list, not the first. 
> (Then 
> again I think I'm reversing the list when reading it in to make sure
> that 
> things happen in the right order.)  Yes you can have two mounts on the
> same 
> directory: that's an overmount and we have to support it.
> 
> Huh, I should add a comment that "umount /dev/hdc" will unmount _every_
> instance of hdc, not just the most recent.  Or possibly I should make
> it so 
> you have to go "umount -a /dev/hdc" in order to get that behavior? 
> (I'm open 
> to suggestions.)

I would prefer -a /dev/hdc for multiple umounts. -a would then naturally
become "more that one umount, whatever the case". Instead of the corner
case where you have multiple mounts on a device and just assumes that you
want to umount them all.. since preferably umount would be called with the
path.. not the device.. assuming umount all without -a would be strange.

> Yes, I tested this code fairly extensively at one point.  Not sure
> when/why it 
> stopped working.

Maybe the magic bitmunching aliens in the svn repository broke it. Not
that uncommon. ;)
 
> > My problem is; it cant be this obvious? is there some glibc trickery
> for 
> > umount(); that make them work for device paths instead of file system
> > paths on certain systems or what?
> 
> Even if it is, if it's not working now (and I just tried it on Ubuntu
> and it 
> didn't) then we can't depend on it.
> 
> Try http://busybox.net/downloads/patches/svn-15806.patch
> 
> > best regards,
> > Christian
> 
> Rob
> -- 
> Never bet against the cheap plastic solution.