Re: [Libguestfs] [PATCH 07/10] Flexible guestfs_vmchannel parameter for future appliances.

> + static const char *options = "fc:?"; > static const struct option long_options[] = { > + { "channel", 0, 0, 'c' }, Shouldn't this be: { "channel", required_argument, 0, 'c' },

> { "foreground", 0, 0, 'f' }, > { "help", 0, 0, '?' }, > { 0, 0, 0, 0 } ... > + * > + * Sources: > + * --channel/-c option on the command line > + * guestfs_vmchannel=... from the kernel command line > + * guestfs=... from the kernel command line > + * built-in default > + * > + * At the moment we expect this to contain "tcp:ip:port" but in > + * future it might contain a device name, eg. "/dev/vcon4" for > + * virtio-console vmchannel. > + */ > + if (vmchannel == NULL && cmdline) { > + char *p; > + int len; size_t len; > + p = strstr (cmdline, "guestfs_vmchannel="); > + if (p) { > + len = strcspn (p + 18, " \t\n"); > + vmchannel = strndup (p + 18, len); > + if (!vmchannel) { > + perror ("strndup"); > + exit (1); > + } > + } > + > + /* Old libraries passed guestfs=host:port. Rewrite it as tcp:host:port. */ > + if (vmchannel == NULL) { > + p = strstr (cmdline, "guestfs="); > + if (p) { > + len = strcspn (p + 4, " \t\n"); > + vmchannel = strndup (p + 4, len); > + if (!vmchannel) { > + perror ("strndup"); > + exit (1); > + } > + memcpy (vmchannel, "tcp:", 4); > + } It took me a while to realize that the first two '4's above stand for strlen("guestfs=") - strlen("tcp:') Hoping to make it easier to follow, I wrote this: if (vmchannel == NULL) { p = strstr (cmdline, "guestfs="); if (p) { p += 8; len = strcspn (p, " \t\n"); vmchannel = malloc (strlen("tcp:") + len + 1); if (!vmchannel) { perror ("malloc"); exit (1); } stpcpy (stpncpy (stpcpy (vmchannel, "tcp:"), p, len), ""); } I don't particularly like the literal 8 or the duplicated "tcp:" strings, nor am I even sure which is the lesser of the evils ;-) And of course, if you're not used to the nested stpcpy idiom, that last line will look weird. Even if you are, it's probably not obvious that the outer stpcpy+"" arg is to NUL-terminate the result. So maybe just add a comment to yours.