Hi Thomas, Thank you for the review. On 10/17/23 11:18 PM, Thomas Weißschuh wrote:
Hi Junxiao, On 2023-10-12 14:55:24-0700, Junxiao Bi wrote:If a /dev/loopX is lost because someone might have removed it by mistake, future losetup operations on that loop device will fail. For examples, following cmds will fail when loop device file is lost. - "losetup -d $loop_device" - "losetup -f $file" or "mount $file $dev"What makes /dev/loopX more prone to accidental deletion than say /dev/loop-control or /dev/sda?
I can't see /dev/loopX is more prone to be deleted than other files. The issue was triggered by one customer, somehow the lost loop device file caused their automation script failure, they don't know how that loop device file get lost. I think it would be good to support this to make losetup more resilient.
This function can be used in other function to remove duplicated code, for example loopcxt_add_device(), i can add a new patch to do that in V2.Users can resurrect the loop device by recreating the device using mknod, but regular users might not be aware. Since /sysfs still have the loop device intact, this commit will detect that case by scanning sysfs and recreate the lost device file. Signed-off-by: Junxiao Bi <junxiao.bi@xxxxxxxxxx> --- lib/loopdev.c | 100 ++++++++++++++++++++++++++++++++++++++++---- sys-utils/losetup.c | 14 +++---- 2 files changed, 97 insertions(+), 17 deletions(-) diff --git a/lib/loopdev.c b/lib/loopdev.c index dae499f256fa..4fc617830bae 100644 --- a/lib/loopdev.c +++ b/lib/loopdev.c @@ -279,6 +279,87 @@ static struct path_cxt *loopcxt_get_sysfs(struct loopdev_cxt *lc) return lc->sysfs; }+static int loopcxt_get_devid(struct loopdev_cxt *lc)+{The only use of this function is to directly format the numer extracted from loopX back into loopX. Why not use the loopX string directly?
+ int devid = -1; + const char *p, *dev = loopcxt_get_device(lc); + + if (!dev) + return -EINVAL; + + p = strrchr(dev, '/'); + if (!p || (sscanf(p, "/loop%d", &devid) != 1 + && sscanf(p, "/%d", &devid) != 1)) + return -EINVAL; + + return devid; +} + +/* scan sysfs entry for loop device. */ +static int loopcxt_scan_sysfs(struct loopdev_cxt *lc) +{ + struct dirent *d; + DIR *sysblock; + int nr; + char name[256]; + + if (!loopcxt_sysfs_available(lc)) + return 0; + + sysblock = opendir(_PATH_SYS_BLOCK); + if (!sysblock) + return 0; + + nr = loopcxt_get_devid(lc); + sprintf(name, "loop%d", nr); + + while ((d = readdir(sysblock))) { + if (!strcmp(d->d_name, name)) + return 1; + } + + return 0;All of this could be replaced by ul_path_accessf().
I will see how to do it in v2.
+} + +/* + * losetup cmds could fail if /dev/loopX is removed by mistake. + * This function will try to detect whether that is case, + * if so, it will recreate the device file and open it.:q + * Return fd if succeed, otherwise negative error code. + */ +static int loopcxt_fix_lost_device_file(struct loopdev_cxt *lc) +{ + char path[PATH_MAX]; + int nr, ret; + FILE *fp; + unsigned int major, minor; + + if (!loopcxt_scan_sysfs(lc)) + return -1; + + nr = loopcxt_get_devid(lc); + if (nr < 0) + return -1; + ret = snprintf(path, PATH_MAX, "%s/loop%d/dev", + _PATH_SYS_BLOCK, nr); + if (ret <= 0 || ret >= PATH_MAX) + return -1; + fp = fopen(path, "r"); + if (!fp) + return -1; + ret = fscanf(fp, "%d:%d", &major, &minor);sysfs_devname_to_devno()
This function only works when /dev/loopX exist. Anyway this part of code will be removed in V2, as Karel suggested we should not do mknod in losetup.
Thanks, Junxiao.
+ fclose(fp); + if (ret != 2) + return -1; + sprintf(path, "/dev/loop%d", nr); + /* default loop device permission is "brw-rw----" */ + umask(0003); + ret = mknod(path, S_IFBLK|0660, makedev(major, minor)); + if (ret) + return -1; + return open(path, lc->mode | O_CLOEXEC); +} + static int __loopcxt_get_fd(struct loopdev_cxt *lc, mode_t mode) { int old = -1; @@ -303,7 +384,17 @@ static int __loopcxt_get_fd(struct loopdev_cxt *lc, mode_t mode) DBG(CXT, ul_debugobj(lc, "open %s [%s]: %m", lc->device, mode == O_RDONLY ? "ro" : mode == O_RDWR ? "rw" : "??")); - + /* loop device file not exist. */ + if (lc->fd < 0 && errno == ENOENT) { + lc->fd = loopcxt_fix_lost_device_file(lc); + DBG(CXT, ul_debugobj(lc, "recreate %s fd %d", + lc->device, lc->fd)); + /* loop file is not lost but doesn't exist, + * reset errno to have user know. + */ + if (lc->fd < 0) + errno = ENOENT; + } if (lc->fd < 0 && old >= 0) { /* restore original on error */ lc->fd = old; @@ -416,13 +507,6 @@ static int loopiter_set_device(struct loopdev_cxt *lc, const char *device) !(lc->iter.flags & LOOPITER_FL_FREE)) return 0; /* caller does not care about device status */- if (!is_loopdev(lc->device)) {- DBG(ITER, ul_debugobj(&lc->iter, "%s does not exist", lc->device)); - return -errno; - } - - DBG(ITER, ul_debugobj(&lc->iter, "%s exist", lc->device)); - used = loopcxt_get_offset(lc, NULL) == 0;if ((lc->iter.flags & LOOPITER_FL_USED) && used)diff --git a/sys-utils/losetup.c b/sys-utils/losetup.c index 0ca910ae3347..b45cc2ee8f3c 100644 --- a/sys-utils/losetup.c +++ b/sys-utils/losetup.c @@ -653,7 +653,7 @@ static int create_loop(struct loopdev_cxt *lc, }/* errors */- errpre = hasdev && loopcxt_get_fd(lc) < 0 ? + errpre = hasdev && lc->fd < 0 ? loopcxt_get_device(lc) : file; warn(_("%s: failed to set up loop device"), errpre); break; @@ -741,8 +741,7 @@ int main(int argc, char **argv) break; case 'c': act = A_SET_CAPACITY; - if (!is_loopdev(optarg) || - loopcxt_set_device(&lc, optarg)) + if (loopcxt_set_device(&lc, optarg)) err(EXIT_FAILURE, _("%s: failed to use device"), optarg); break; @@ -754,8 +753,7 @@ int main(int argc, char **argv) break; case 'd': act = A_DELETE; - if (!is_loopdev(optarg) || - loopcxt_set_device(&lc, optarg)) + if (loopcxt_set_device(&lc, optarg)) err(EXIT_FAILURE, _("%s: failed to use device"), optarg); break; @@ -883,8 +881,7 @@ int main(int argc, char **argv) else act = A_SHOW_ONE;- if (!is_loopdev(argv[optind]) ||- loopcxt_set_device(&lc, argv[optind])) + if (loopcxt_set_device(&lc, argv[optind])) err(EXIT_FAILURE, _("%s: failed to use device"), argv[optind]); optind++; @@ -935,8 +932,7 @@ int main(int argc, char **argv) case A_DELETE: res = delete_loop(&lc); while (optind < argc) { - if (!is_loopdev(argv[optind]) || - loopcxt_set_device(&lc, argv[optind])) + if (loopcxt_set_device(&lc, argv[optind])) warn(_("%s: failed to use device"), argv[optind]); optind++; -- 2.39.3 (Apple Git-145)
