Re: [Xen-devel] [PATCH 3 of 5 V2] tools/libxl: setup/teardown Remus network buffering

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-10-14 at 07:23 -0700, Shriram Rajagopalan wrote:
> On Wed, Sep 4, 2013 at 8:17 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> wrote:

>         
>         > +static char **get_guest_vif_list(libxl__gc *gc, uint32_t domid,
>         > +                                 int *num_vifs)
>         > +{
>         > +    libxl_device_nic *nics;
>         > +    int nb, i = 0;
>         > +    char **vif_list = NULL;
>         > +
>         > +    nics = libxl_device_nic_list(CTX, domid, &nb);
>         > +    if (!nics) {
>         > +        *num_vifs = 0;
>         > +        return NULL;
>         > +    }
>         > +
>         > +    vif_list = libxl__calloc(gc, nb, sizeof(char *));
>         > +    for (i = 0; i < nb; ++i) {
>         > +        vif_list[i] = get_vifname(gc, domid, &nics[i]);
>         > +        libxl_device_nic_dispose(&nics[i]);
>         > +    }
>         > +    free(nics);
>         > +
>         > +    *num_vifs = nb;
>         > +    return vif_list;
>         > +}
>         
>         
>         I think rather than creating a list of char *'s you should just use 
> the
>         array of libxl_device_nic's and pass that around in your context, pass
>         the individual elements to libxl__netbuf_script_setup etc and then let
>         them determine the name as necessary. That would seem to remove a 
> bunch
>         of book keeping around this list.

> 
> The list of nics returned by libxl__device_nic_list is not managed by the gc.

Hrm, that's rather annoying. There is scope for making an interna
libxl__device_nic which does take a gc and having the external facing
libxl_device_nic_list pass in NOGC though.

> So at every exit point in the main functions, there has to be extra code to 
> free up the invididual
> nics in the list and the list itself. 

Yes, that would be less than ideal.

> In contrast, with the current approach, all allocations are under the
> gc's purview.  The list of nics are released immediately after creating the 
> array of vifnames.
> This seemed to eliminate redundant error handling logic and keeps the code 
> simpler.  I'll also add 
> checks for backend_domid!=0 to this function, to ensure that Remus works only 
> for domains
> with no driver domain backed interfaces.




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