On Sat, May 04, 2019 at 06:18:27PM +0100, Rafael Neves wrote: > On Sun, Apr 14, 2019 at 08:05:21PM +0100, Rafael Neves wrote: > > On Mon, Apr 08, 2019 at 12:35:41AM +0100, Rafael Neves wrote: > > > Hi tech@, > > > > > > When I had to change a HD between machines I figured out that -P option > > > of mount_mfs(8) does not work with DUIDs: > > > > > > # mount_mfs -P ca7552589896b01e.d swap /mnt > > > mount_mfs: cannot stat ca7552589896b01e.d: No such file or directory > > > > > > mount_mfs(8) already allows a DUID as the `special` argument, it is not > > > documented though. The patch bellow allow using -P with a DUID as block > > > device. > > > > > > Important: > > > I used the same approach used in the code (pointer swap) to resolve DUID > > > to device using opendev(3). But after the patch there will be two > > > opendev(3) calls with non-NULL realpath, and as manpage states the > > > stored path is overwritten by subsequent calls. > > > > > > It took some time of code reading and rereading until I convinced myself > > > that there is no problem in this *specific* situation. > > > > > > However, I think that should be safier to use malloc(size = PATH_MAX) + > > > strlcpy() on `pop` and `special`. That way, code refactoring doesn't to > > > care about the relative position of the opendev(3) calls. > > > > > > What do you think? Comments? > > > > > [snip] > > > > Here is a revised patch that uses malloc(3)+strlcpy(3) idiom, and > > document that newfs(8) accepts DUID as `special` and as an argument > > of -P option. > > > > [snip] > > > > ping? > > Here follows a updated patch, that removes a not used the declaration > that sneaked in the previous patch, and includes the lastest version > of newfs(8) manpage. > > [snip]
Todd asked if there was a reason to not put duid resolution inside the copy() function, and he pointed out that duid resolution there seemed a bit clearer. Revisiting the code I think that copy() is standalone and supposes that src and dst are both valid paths. In my opinion, putting duid resolution in src, and not to dst make its semantics strange. The other way round, putting duid to dev resolving in both of them seems an overkill, because we know that dst will always be a valid path. So I relocated duid resolution code and applied the newfs.c rev 1.112 changes to it. It seems clearer than the last patch and doesn't touch copy() function. I am testing this change for a month with no issues. I can change my mSATA/sdCards between my amd64 appliances and my diskless setups just work. Comments? If you think that the right place for this is inside copy(), just let me know that I make the changes. Updated patch: Index: sbin/newfs/newfs.8 =================================================================== RCS file: /cvs/src/sbin/newfs/newfs.8,v retrieving revision 1.75 diff -u -p -r1.75 newfs.8 --- sbin/newfs/newfs.8 23 Apr 2019 18:13:11 -0000 1.75 +++ sbin/newfs/newfs.8 9 Jul 2019 18:57:21 -0000 @@ -30,7 +30,7 @@ .\" .\" @(#)newfs.8 8.3 (Berkeley) 3/27/94 .\" -.Dd $Mdocdate: April 23 2019 $ +.Dd $Mdocdate: July 9 2019 $ .Dt NEWFS 8 .Os .Sh NAME @@ -88,8 +88,10 @@ The .Ar special file should be a raw device, for example -.Pa /dev/rsd0a ; -if a relative path like +.Pa /dev/rsd0a , +or a +.Xr disklabel 8 +UID (DUID); if a relative path like .Pa sd0a is specified, the corresponding raw device is used. @@ -293,7 +295,7 @@ is a directory, populate the created mfs contents of the directory. If .Ar file -is a block device, populate the created mfs file system with the +is a block device or a DUID, populate the created mfs file system with the contents of the FFS file system contained on the device. .El .Pp Index: sbin/newfs/newfs.c =================================================================== RCS file: /cvs/src/sbin/newfs/newfs.c,v retrieving revision 1.112 diff -u -p -r1.112 newfs.c --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112 +++ sbin/newfs/newfs.c 9 Jul 2019 18:57:21 -0000 @@ -174,7 +174,7 @@ main(int argc, char *argv[]) struct stat st; struct statfs *mp; struct rlimit rl; - int fsi = -1, oflagset = 0, fso, len, n, maxpartitions; + int fsi = -1, oflagset = 0, fso, fd, len, n, maxpartitions; char *cp = NULL, *s1, *s2, *special, *opstring, *realdev; #ifdef MFS char mountfromname[BUFSIZ]; @@ -382,7 +382,10 @@ main(int argc, char *argv[]) fso = opendev(special, O_WRONLY, 0, &realdev); if (fso == -1) fatal("%s: %s", special, strerror(errno)); - special = realdev; + + if ((special = malloc(PATH_MAX)) == NULL) + fatal("cannot allocate memory"); + strlcpy(special, realdev, PATH_MAX); /* Bail if target special is mounted */ n = getmntinfo(&mp, MNT_NOWAIT); @@ -516,8 +519,23 @@ havelabel: struct mfs_args args; char tmpnode[PATH_MAX]; - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0) - errx(1, "Cannot create tmp mountpoint for -P"); + if (pop != NULL) { + if (isduid(pop, 0)) { + fd = opendev(pop, O_RDONLY, OPENDEV_BLCK, + &realdev); + if (fd == -1) + err(1, "could not open %s", pop); + close(fd); + + if ((pop = malloc(PATH_MAX)) == NULL) + fatal("cannot allocate memory"); + strlcpy(pop, realdev, PATH_MAX); + } + + if (gettmpmnt(tmpnode, sizeof(tmpnode)) == 0) + errx(1, "Cannot create tmp mountpoint for -P"); + } + memset(&args, 0, sizeof(args)); args.base = membase; args.size = fssize * DEV_BSIZE;