In article <476DCE3D.7060402@tastylime.net>,
Jeff Rizzo  <riz@tastylime.net> wrote:
>-=-=-=-=-=-
>
>I've been working on various things lately to improve the state of
>wedges and wedge-related things - one of the (valid) complaints is that
>there is no way to 'nail down' wedge configuration currently.  With the
>attached patch, however, one can specify the wedge's *name* to mount
>from.  With the current DKWEDGE_AUTODISCOVER options, the name is set
>according to the underlying disk-labelling method:
>
>DKWEDGE_METHOD_GPT:  copied from the user-supplied label if it exists,
>the partition GUID if not
>DKWEDGE_METHOD_BSDLABEL:  set to the BSD disklabel partition name (e.g.
>"wd0a", "sd1g")
>DKWEDGE_METHOD_MBR: set to a bsd-style partition name beginning with the
>'e' partition - up to 4 per disk (e.g., "wd0e" through "wd0h")
>
>This is sufficient to ensure you're mounting the partition you
>*intended* to mount.  Currently, the kernel requires manual intervention
>if it tries to configure two wedges with the name wname.
>
>Wedges-by-name are specified by wedge:<wedgename>, following some prior
>art in the kernel from David Young.  With the attached patch, I can use
>this line in my fstab:
>
>wedge:b57b54a4-ad31-11dc-93e5-000c29746425 /foo ffs rw 1 2
>
>to mount "/foo":
>
>netbsd# mount /foo
>netbsd# df -h /foo
>Filesystem        Size       Used      Avail %Cap Mounted on
>/dev/dk4          7.9G       2.0K       7.5G   0% /foo
>netbsd#
>
>
>or, I can do it on the command line:
>
>netbsd# mount wedge:b57b54a4-ad31-11dc-93e5-000c29746425 /mnt
>netbsd# df -h /mnt
>Filesystem        Size       Used      Avail %Cap Mounted on
>/dev/dk4          7.9G       2.0K       7.5G   0% /mnt
>
>
>This is primarily a proof of concept;  assuming this meets with people's
>general approval, there's a lot of tools left to convert (including most
>of the mount_* commands - I've only done ffs and lfs so far) - but I
>wanted to get some feedback before doing any more work on it. 
>
>Is this a good approach?  How bad is my code?  :)
>
>Known issues:
>    - with this patch, wedge:/foo does not refer to an nfs mount.  :)
>    - wedge names are specified as uint8_t [] (Unicode UTF-8), but this
>patch treats them as char [].  What's the proper way to deal with utf-8?
>    - I wasn't sure the best way to handle the string returned by
>getwedgebyname() - suggestions?
>
>
>What have I missed?  Suggestions gratefully accepted.
>Thanks!

>+char *
>+getwedgebyname(const char *wname)
>+{
>+	size_t bufsize;
>+	char *disknames, *disk, *lastp;
>+	char *wedge;
>+
>+	if (sysctlbyname("hw.disknames", NULL, &bufsize, NULL, 0) == -1) {
>+	    warn("%s: could not get buffer size", __func__);
>+	    return NULL;
>+	}

KNF is indent by tab. I am also not sure if this belongs in libutil.
Specially if it uses warn(3). Library functions should not print errors.

>+
>+	bufsize += 200; /* arbitrary wiggle room */
>+	
>+	if ((disknames = (char *)malloc(bufsize)) == NULL) {
>+	    warn("Could not allocate buffer");
>+	    return NULL;
>+	}

No cast for malloc.
>+
>+	if (sysctlbyname("hw.disknames", disknames, &bufsize, NULL,
>+		0) == -1) {
>+	    warn("Could not get names");
>+	    return NULL;
>+	}
>+
>+	for (disk = strtok_r(disknames, " ", &lastp);
>+	    disk != NULL;
>+	    disk = strtok_r(NULL, " ", &lastp))
>+		if (findwedge(disk, wname)) {
>+		    wedge = (char *)malloc(strlen(disk));

No cast for malloc; +1 for NUL.

>+		    strcpy(wedge, disk);
>+		    free(disknames);
>+		    return wedge;
>+		}
>+	return NULL;
>+}


> 	case 2:
> 		/*
>+		 * Handle the wedge case first. XXX yes, this means
>+		 * NFS mounts from machines named "wedge" will no
>+		 * longer work correctly
>+		 */
>+		if (strncmp("wedge:", argv[0], strlen("wedge:")) == 0) {
>+			special = getwedgebyname(argv[0] +
>+				strlen("wedge:"));
>+			if (special == NULL)
>+				errx(1, "%s: unknown wedge name",
>+					argv[0] + strlen("wedge:"));
>+			snprintf(specbuf, MAXPATHLEN, "/dev/%s", special);
>+			free(special);
>+			special = specbuf;
>+		} else
>+			special = argv[0];
>+

The above code can be factored out into a function since it is used
in 3 places. Don't use strlen("wedge:"), use (sizeof("wedge:") - 1).
Perhaps put that in a define:

#define PREFIX	"wedge:"
#define PREFIX_LEN (sizeof(PREFIX) - 1)

I am not sure this is the right way to do things, since I will not be able
to mount from a host called "wedge" :) Perhaps a better syntax is to use
[wedgename].

christos