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

["Roger Pau Monné" <roger.pau@citrix.com>]

On 05/02/13 11:40, Ian Campbell wrote:
> On Fri, 2013-01-25 at 15:26 +0000, Roger Pau Monne wrote:
>> This option was supported in the past, according to
>> http://wiki.xen.org/wiki/Vif-route, so we should also support it in
>> libxl.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ulf Kreutzberg <ulf.kreutzberg@hosteurope.de>
>> ---
>>  docs/misc/xl-network-configuration.markdown |    6 ++++++
>>  tools/libxl/libxl.c                         |    6 +++++-
>>  tools/libxl/libxl_linux.c                   |    9 ++++++++-
>>  tools/libxl/libxl_types.idl                 |    1 +
>>  tools/libxl/xl_cmdimpl.c                    |    5 +++++
>>  5 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown
>> index 5e2f049..b98b28e 100644
>> --- a/docs/misc/xl-network-configuration.markdown
>> +++ b/docs/misc/xl-network-configuration.markdown
>> @@ -67,6 +67,12 @@ added to. The default is `xenbr0`. The bridge must be configured using
>>  your distribution's network configuration tools. See the [wiki][net]
>>  for guidance and examples.
>>  
>> +### netdev
>> +
>> +Specifies the name of the network interface which has an IP and which
>> +is in the network the VIF should communicate with.
> 
> I think this needs to clarify that it is a host network device.

Ack

> 
>>  This is used by the
>> +vif-route hotplug script. See [wiki][vif-route] for guidance and examples.
> 
> You need to add a URL for vif-route near the bottom I think.
> 
>> +
>>  ### type
>>  
>>  This keyword is valid for HVM guests only.
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 73e0dc3..03bfe1a 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -2826,7 +2826,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
>>      if (rc) goto out;
>>  
>>      front = flexarray_make(gc, 16, 1);
>> -    back = flexarray_make(gc, 16, 1);
>> +    back = flexarray_make(gc, 18, 1);
>>  
>>      if (nic->devid == -1) {
>>          if ((nic->devid = libxl__device_nextid(gc, domid, "vif") < 0)) {
>> @@ -2862,6 +2862,10 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
>>          flexarray_append(back, "ip");
>>          flexarray_append(back, libxl__strdup(gc, nic->ip));
>>      }
>> +    if (nic->netdev) {
>> +        flexarray_append(back, "netdev");
>> +        flexarray_append(back, libxl__strdup(gc, nic->netdev));
>> +    }
>>  
>>      if (nic->rate_interval_usecs > 0) {
>>          flexarray_append(back, "rate");
>> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
>> index 1fed3cd..4cbdc19 100644
>> --- a/tools/libxl/libxl_linux.c
>> +++ b/tools/libxl/libxl_linux.c
>> @@ -84,11 +84,16 @@ static char **get_hotplug_env(libxl__gc *gc,
>>                                char *script, libxl__device *dev)
>>  {
>>      const char *type = libxl__device_kind_to_string(dev->backend_kind);
>> +    char *be_path = libxl__device_backend_path(gc, dev);
>>      char **env;
>> +    char *netdev;
>>      int nr = 0;
>>      libxl_nic_type nictype;
>>  
>> -    const int arraysize = 13;
>> +    netdev = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s", be_path,
>> +                                                             "netdev"));
>> +
>> +    const int arraysize = 15;
>>      GCNEW_ARRAY(env, arraysize);
>>      env[nr++] = "script";
>>      env[nr++] = script;
>> @@ -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).

> 
>>      if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) {
>>          if (libxl__nic_type(gc, dev, &nictype)) {
>>              LOG(ERROR, "unable to get nictype");
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index acc4bc9..0e7943e 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -382,6 +382,7 @@ libxl_device_nic = Struct("device_nic", [
>>      ("nictype", libxl_nic_type),
>>      ("rate_bytes_per_interval", uint64),
>>      ("rate_interval_usecs", uint32),
>> +    ("netdev", string),
>>      ])
>>  
>>  libxl_device_pci = Struct("device_pci", [
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index e964bf1..92a64e4 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1205,6 +1205,9 @@ static void parse_config_data(const char *config_source,
>>                      parse_vif_rate(&config, (p2 + 1), nic);
>>                  } else if (!strcmp(p, "accel")) {
>>                      fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
>> +                } else if (!strcmp(p, "netdev")) {
>> +                    free(nic->netdev);
>> +                    nic->netdev = strdup(p2 + 1);
>>                  }
>>              } while ((p = strtok(NULL, ",")) != NULL);
>>  skip_nic:
>> @@ -5511,6 +5514,8 @@ int main_networkattach(int argc, char **argv)
>>              }
>>          } else if (MATCH_OPTION("bridge", *argv, oparg)) {
>>              replace_string(&nic.bridge, oparg);
>> +        } else if (MATCH_OPTION("netdev", *argv, oparg)) {
>> +            replace_string(&nic.netdev, oparg);
>>          } else if (MATCH_OPTION("ip", *argv, oparg)) {
>>              replace_string(&nic.ip, oparg);
>>          } else if (MATCH_OPTION("script", *argv, oparg)) {
> 
> 


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