Re: [Xen-devel] [PATCH 1/2] xl/libxl: add netdev to vif specification

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-02-05 at 11:39 +0000, Roger Pau Monne wrote:
> On 05/02/13 12:10, Ian Campbell wrote:
> > On Tue, 2013-02-05 at 10:56 +0000, Roger Pau Monne wrote:
> >>>> @@ -98,6 +103,8 @@ static char **get_hotplug_env(libxl__gc *gc,
> >>>>      env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, 
> >>>> dev->devid);
> >>>>      env[nr++] = "XENBUS_BASE_PATH";
> >>>>      env[nr++] = "backend";
> >>>> +    env[nr++] = "netdev";
> >>>> +    env[nr++] = netdev;
> >>>
> >>> Mightn't this be NULL?
> >>
> >> Yes, if we are using the vif-bridge script this will be NULL, but I
> >> prefer adding this NULL here rather than having a conditional and a
> >> variable array size (because we also have an assert(nr == arraysize) at
> >> the end of the code block).
> > 
> > Doesn't NULL terminate the env list? That might work right now while
> > this option is last but it will confuse the hell out of whoever adds the
> > next variable...
> 
> Indeed, good catch. Also we are enforcing this (not passing NULL values
> in env variables) in libxl__exec.

No, finding a NULL env entry signals the end of the list:
    if (env != NULL) {
        for (int i = 0; env[i] != NULL && env[i+1] != NULL; i += 2) {
            if (setenv(env[i], env[i+1], 1) < 0) {
                LOGEV(ERROR, errno, "setting env vars (%s = %s)",
                                    env[i], env[i+1]);
                goto out;
            }
        }
    }

I suppose we could treat env[i] != NULL  and env[i+1] == NULL specially
(e.g. ignore it and continue), but ick...

Ian 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel