| Message ID | 1517850433.14196.63.camel@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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).
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
--- 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); }