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

[Richard W.M. Jones]

On Mon, Sep 21, 2009 at 10:08:50PM +0200, Jim Meyering wrote:
> > +  static const char *options = "fc:?";
> >    static const struct option long_options[] = {
> > +    { "channel", 0, 0, 'c' },
> 
> Shouldn't this be:
> 
>        { "channel", required_argument, 0, 'c' },

Yes, it should be :-(

> >      { "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.

I'll come up with a better patch tomorrow, and for the rest of the stuff
you mentioned below.

Thanks for looking at these patches.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw