[dm-devel] [PATCH] kpartx: Improve finding loopback device by file

[Martin Wilck]

On Tue, 2018-02-06 at 12:19 +0100, Julian Andres Klode wrote:
> On Mon, Feb 05, 2018 at 06:07:13PM +0100, Martin Wilck wrote:
> > On Mon, 2018-02-05 at 16:46 +0100, Julian Andres Klode wrote:
> > > On Mon, Feb 05, 2018 at 04:12:00PM +0100, Martin Wilck wrote:
> > > > 
> > > > Can we agree on the following:
> > > > 
> > > >  1 if realpath (filename) results in error, abort
> > > 
> > > OK
> > > 
> > > 
> > > >  2 if realpath(lo_name) results in ENODEV and filename matches
> > > > lo_name,
> > > > remove loop device
> > > >  3 if realpath(lo_name) results in another error code, skip it
> > > >  4 remove if realpath(filename) matches realpath(lo_name)
> > > 
> > > 
> > > This seems like a lot of ifs. It might be easier to just path if
> > > realpath(path) fails (as realpath(1) does), something like:
> > > 
> > > 		
> > > 		char *loopname = loopinfo.lo_name;
> > > 		if (realpath(loopinfo.lo_name, rloopfilename) != NULL)
> > > 			loopname = rloopfilename;
> > > 
> > > 		if (0 == strcmp(filename, loopinfo.lo_name) ||my
> > 
> > 		    0 == strcmp(rfilename, loopname))) {
> > > 			found = realpath(path, NULL);
> > > 			break;
> > > 		}
> > > 
> > > 
> > > That should solve all problems and produce useful results with
> > > not
> > > a lot of logic.
> > 
> > I've reproduced your issue. I can see that the current behavior is
> > wrong. 
> > 
> > This may be paranoid, but I really want to avoid false positives -
> > kpartx removing mappings it isn't supposed to remove. Therefore I'd
> > like to avoid compare by "filename" and "loopname". I think it's
> > possible.
> > 
> > IMO it would be better to have kpartx use the realpath while
> > *creating*
> > the partition mapping already:
> > 
> > --- a/kpartx/kpartx.c
> > +++ b/kpartx/kpartx.c
> > @@ -341,7 +341,7 @@ main(int argc, char **argv){
> >                 if (!loopdev) {
> >                         loopdev = find_unused_loop_device();
> >  
> > -                       if (set_loop(loopdev, device, 0, &ro)) {
> > +                       if (set_loop(loopdev, rpath, 0, &ro)) {
> >                                 fprintf(stderr, "can't set up
> > loop\n");
> >                                 exit (1);
> >                         }
> > 
> > 
> > That would make sure that kpartx can delete loop devices created by
> > itself, which is what we want. IMO it's sufficient to solve your
> > issue.
> 
> I was about to suggest that, but did not do it, as it's kind of
> weird. You
> add /a/b/c.img (pointing to /x/y/z.img) and losetup then shows you
> /x/y/z.img
> - I fear this might break some stuff that checks whether the binding
> was
> actually created (or checks which loop device it was created at).

IMO this is exactly the right thing to do. In order to identify loop
mappings, you shouldn't rely on something as volatile as arbitrary
symlinks. I'm aware that the "real path" isn't carved in stone either, 
but at least it uniquely identifies a file at a given point in time.
Internally, the loop driver refers to the inode anyway.

The scripts you mention, if they actually exist, would benefit if they
were fixed to work with real paths, too.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)