Xen project Mailing List

On Wed, Sep 4, 2013 at 8:17 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/libxl_netbuffer.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/libxl/libxl_netbuffer.c Thu Aug 29 14:36:36 2013 -0700
> @@ -0,0 +1,434 @@
> +/*
> + * Copyright (C) 2013
> + * Author Shriram Rajagopalan <rshriram@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +#include <netlink/cache.h>
> +#include <netlink/socket.h>
> +#include <netlink/attr.h>
> +#include <netlink/route/link.h>
> +#include <netlink/route/route.h>
> +#include <netlink/route/qdisc.h>
> +#include <netlink/route/qdisc/plug.h>
> +
> +typedef struct libxl__remus_netbuf_ctx {
> + struct rtnl_qdisc **netbuf_qdisc_list;
> + struct nl_sock *nlsock;
> + struct nl_cache *qdisc_cache;
> + char **vif_list;
> + char **ifb_list;
> + uint32_t num_netbufs;
> + uint32_t unused;
> +} libxl__remus_netbuf_ctx;
> +
> +/* If the device has a vifname, then use that instead of
> + * the vifX.Y format.
> + */
> +static char *get_vifname(libxl__gc *gc, uint32_t domid,
> + libxl_device_nic *nic)
> +{
> + char *vifname = NULL;
> + vifname = libxl__xs_read(gc, XBT_NULL,
> + libxl__sprintf(gc,
> + "%s/backend/vif/%d/%d/vifname",
> + libxl__xs_get_dompath(gc, 0),
> + domid, nic->devid));
> + if (!vifname) {
> + vifname = (char *)libxl__device_nic_devname(gc, domid,

Please don't cast away the const here. Either make this function return
const or dup this. Preferably const this function.

> + nic->devid,
> + nic->nictype);
> + }
> +
> + return vifname;
> +}
> +
> +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.

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.

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.

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