[PATCH 0/2] add vif-route support to libxl/xl

[PATCH 0/2] add vif-route support to libxl/xl

[Roger Pau Monne]

Support for vif-route was missing in libxl, this patch series adds a 
"netdev" vif option and also allows setting a default netdev in 
xl.conf global configuration file.

vif-route doesn't support HVM domains because it doesn't support the 
"add" or "remove" actions.

I would like to request some testing from people that actually use 
this network mode.

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

[Roger Pau Monne]

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. This is used by the
+vif-route hotplug script. See [wiki][vif-route] for guidance and examples.
+
 ### 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;
     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)) {
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[Roger Pau Monne]

This adds a new global option in the xl configuration file called
"defaultnetdev", that is used to specify the default netdev to use
when none is passed in the vif specification.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ulf Kreutzberg <ulf.kreutzberg@hosteurope.de>
---
 docs/man/xl.conf.pod.5   |    6 ++++++
 tools/examples/xl.conf   |    3 +++
 tools/libxl/xl.c         |    4 ++++
 tools/libxl/xl.h         |    1 +
 tools/libxl/xl_cmdimpl.c |    5 +++++
 5 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 82c6b20..d1aa4c9 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -82,6 +82,12 @@ Configures the default bridge to set for virtual network devices.
 
 Default: C<xenbr0>
 
+=item B<defaultnetdev="NAME">
+
+Configures the default netdev to set for virtual network devices.
+
+Default: C<None>
+
 =item B<output_format="json|sxp">
 
 Configures the default output format used by xl when printing "machine
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 28ab796..120a41b 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -20,3 +20,6 @@
 # if disabled the old behaviour will be used, and hotplug scripts will be
 # launched by udev.
 #run_hotplug_scripts=1
+
+# default netdev to use with vif-route hotplug script
+#defaultnetdev="eth0"
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index ecbcd3b..06a09c2 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -43,6 +43,7 @@ int run_hotplug_scripts = 1;
 char *lockfile;
 char *default_vifscript = NULL;
 char *default_bridge = NULL;
+char *default_netdev = NULL;
 enum output_format default_output_format = OUTPUT_FORMAT_JSON;
 
 static xentoollog_level minmsglevel = XTL_PROGRESS;
@@ -91,6 +92,9 @@ static void parse_global_config(const char *configfile,
     if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0))
 	default_bridge = strdup(buf);
 
+    if (!xlu_cfg_get_string (config, "defaultnetdev", &buf, 0))
+        default_netdev = strdup(buf);
+
     if (!xlu_cfg_get_string (config, "output_format", &buf, 0)) {
         if (!strcmp(buf, "json"))
             default_output_format = OUTPUT_FORMAT_JSON;
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index be6f38b..5d43572 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -148,6 +148,7 @@ extern int dryrun_only;
 extern char *lockfile;
 extern char *default_vifscript;
 extern char *default_bridge;
+extern char *default_netdev;
 extern char *blkdev_start;
 
 enum output_format {
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 92a64e4..2bbbbc1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1141,6 +1141,11 @@ static void parse_config_data(const char *config_source,
                 nic->bridge = strdup(default_bridge);
             }
 
+            if (default_netdev) {
+                free(nic->netdev);
+                nic->netdev = strdup(default_netdev);
+            }
+
             p = strtok(buf2, ",");
             if (!p)
                 goto skip_nic;
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH 0/2] add vif-route support to libxl/xl

[Roger Pau Monné]

On 25/01/13 16:26, Roger Pau Monne wrote:
> Support for vif-route was missing in libxl, this patch series adds a 
> "netdev" vif option and also allows setting a default netdev in 
> xl.conf global configuration file.
> 
> vif-route doesn't support HVM domains because it doesn't support the 
> "add" or "remove" actions.
> 
> I would like to request some testing from people that actually use 
> this network mode.

I've realized that I haven't added the necessary compatibility defines
in libxl.h, so this should not be applied as is.

Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 793 bytes --]

On Fri, Jan 25, 2013 at 3:26 PM, Roger Pau Monne <roger.pau@citrix.com>wrote:

> This adds a new global option in the xl configuration file called
> "defaultnetdev", that is used to specify the default netdev to use
> when none is passed in the vif specification.
>

I'm not a fan of the name, though; it doesn't seem very scalable.  It looks
like we already have "defaultbridge', so I can see this is just following
precedent, but I wonder if it might be worth putting some more thought into
it before proceeding?

It seems like if we're going to have a default sub-option, it should at
least have the name of the option in which it resides.
"vif_netdev_default" or "default_vif_netdev" seem like better option.  Or
maybe "vif.netdev.default"? "defaults.vif.netdev"?

Any thoughts?

 -George

[-- Attachment #1.2: Type: text/html, Size: 1249 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 1144 bytes --]

On Mon, Jan 28, 2013 at 11:00 AM, George Dunlap <George.Dunlap@eu.citrix.com
> wrote:

> On Fri, Jan 25, 2013 at 3:26 PM, Roger Pau Monne <roger.pau@citrix.com>wrote:
>
>> This adds a new global option in the xl configuration file called
>> "defaultnetdev", that is used to specify the default netdev to use
>> when none is passed in the vif specification.
>>
>
> I'm not a fan of the name, though; it doesn't seem very scalable.  It
> looks like we already have "defaultbridge', so I can see this is just
> following precedent, but I wonder if it might be worth putting some more
> thought into it before proceeding?
>
> It seems like if we're going to have a default sub-option, it should at
> least have the name of the option in which it resides.
> "vif_netdev_default" or "default_vif_netdev" seem like better option.  Or
> maybe "vif.netdev.default"? "defaults.vif.netdev"?
>
> Any thoughts?
>

Also, it's probably worth thinking about what other options should have
defaults.  vif.backend is another one that it seems like it would be useful
to be able to override (e.g., to make it easy to switch to and from driver
domains).

 -George

[-- Attachment #1.2: Type: text/html, Size: 2049 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[Roger Pau Monné]

On 28/01/13 17:11, George Dunlap wrote:
> On Mon, Jan 28, 2013 at 11:00 AM, George Dunlap
> <George.Dunlap@eu.citrix.com <mailto:George.Dunlap@eu.citrix.com>> wrote:
> 
>     On Fri, Jan 25, 2013 at 3:26 PM, Roger Pau Monne
>     <roger.pau@citrix.com <mailto:roger.pau@citrix.com>> wrote:
> 
>         This adds a new global option in the xl configuration file called
>         "defaultnetdev", that is used to specify the default netdev to use
>         when none is passed in the vif specification.
> 
> 
>     I'm not a fan of the name, though; it doesn't seem very scalable. 
>     It looks like we already have "defaultbridge', so I can see this is
>     just following precedent, but I wonder if it might be worth putting
>     some more thought into it before proceeding?
> 
>     It seems like if we're going to have a default sub-option, it should
>     at least have the name of the option in which it resides. 
>     "vif_netdev_default" or "default_vif_netdev" seem like better
>     option.  Or maybe "vif.netdev.default"? "defaults.vif.netdev"?

vif_default_netdev sounds good to me, but I don't have any opinions
against the others. We should also add vif_default_bridge (while keeping
the old option). Someone has a strong opinion about any of this names,
or should I just choose the one I prefer?

> 
>     Any thoughts?
> 
> 
> Also, it's probably worth thinking about what other options should have
> defaults.  vif.backend is another one that it seems like it would be
> useful to be able to override (e.g., to make it easy to switch to and
> from driver domains).

Yes, but this should be added when driver domains are done
(vif_default_domain/block_default_domain).

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

[Ian Campbell]

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.

>  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?

>      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

Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[Ian Campbell]

On Mon, 2013-01-28 at 11:00 +0000, George Dunlap wrote:
> On Fri, Jan 25, 2013 at 3:26 PM, Roger Pau Monne
> <roger.pau@citrix.com> wrote:
>         This adds a new global option in the xl configuration file
>         called
>         "defaultnetdev", that is used to specify the default netdev to
>         use
>         when none is passed in the vif specification.
> 
> 
> I'm not a fan of the name, though; it doesn't seem very scalable.  It
> looks like we already have "defaultbridge', so I can see this is just
> following precedent, but I wonder if it might be worth putting some
> more thought into it before proceeding?
> 
> 
> It seems like if we're going to have a default sub-option, it should
> at least have the name of the option in which it resides.
> "vif_netdev_default" or "default_vif_netdev" seem like better option.
> Or maybe "vif.netdev.default"? "defaults.vif.netdev"?

netdev is also a bit non-descriptive, even if it is what the vif-route
script uses perhaps we present something more meaningful to the user?

"gatewaydev" or something along those lines perhaps?

I'd be inclined to use whatever name we decide here in the libxl
API/internals as well and just go netdev at the hotplug script
interface.

Ian.

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

[Roger Pau Monné]

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

Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[Roger Pau Monné]

On 05/02/13 11:41, Ian Campbell wrote:
> On Mon, 2013-01-28 at 11:00 +0000, George Dunlap wrote:
>> On Fri, Jan 25, 2013 at 3:26 PM, Roger Pau Monne
>> <roger.pau@citrix.com> wrote:
>>         This adds a new global option in the xl configuration file
>>         called
>>         "defaultnetdev", that is used to specify the default netdev to
>>         use
>>         when none is passed in the vif specification.
>>
>>
>> I'm not a fan of the name, though; it doesn't seem very scalable.  It
>> looks like we already have "defaultbridge', so I can see this is just
>> following precedent, but I wonder if it might be worth putting some
>> more thought into it before proceeding?
>>
>>
>> It seems like if we're going to have a default sub-option, it should
>> at least have the name of the option in which it resides.
>> "vif_netdev_default" or "default_vif_netdev" seem like better option.
>> Or maybe "vif.netdev.default"? "defaults.vif.netdev"?
> 
> netdev is also a bit non-descriptive, even if it is what the vif-route
> script uses perhaps we present something more meaningful to the user?
> 
> "gatewaydev" or something along those lines perhaps?

Will this also imply that xl should use gatewaydev instead of netdev in
the vif config line? Right now we don't support netdev, but I guess we
should add it for backwards compatibility.

> I'd be inclined to use whatever name we decide here in the libxl
> API/internals as well and just go netdev at the hotplug script
> interface.
> 
> Ian.
> 
> 

Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf

[Ian Campbell]

On Tue, 2013-02-05 at 11:00 +0000, Roger Pau Monne wrote:
> On 05/02/13 11:41, Ian Campbell wrote:
> > On Mon, 2013-01-28 at 11:00 +0000, George Dunlap wrote:
> >> On Fri, Jan 25, 2013 at 3:26 PM, Roger Pau Monne
> >> <roger.pau@citrix.com> wrote:
> >>         This adds a new global option in the xl configuration file
> >>         called
> >>         "defaultnetdev", that is used to specify the default netdev to
> >>         use
> >>         when none is passed in the vif specification.
> >>
> >>
> >> I'm not a fan of the name, though; it doesn't seem very scalable.  It
> >> looks like we already have "defaultbridge', so I can see this is just
> >> following precedent, but I wonder if it might be worth putting some
> >> more thought into it before proceeding?
> >>
> >>
> >> It seems like if we're going to have a default sub-option, it should
> >> at least have the name of the option in which it resides.
> >> "vif_netdev_default" or "default_vif_netdev" seem like better option.
> >> Or maybe "vif.netdev.default"? "defaults.vif.netdev"?
> > 
> > netdev is also a bit non-descriptive, even if it is what the vif-route
> > script uses perhaps we present something more meaningful to the user?
> > 
> > "gatewaydev" or something along those lines perhaps?
> 
> Will this also imply that xl should use gatewaydev instead of netdev in
> the vif config line? Right now we don't support netdev, but I guess we
> should add it for backwards compatibility.

netdev is the xend name too? I didn't realise that.

I guess we could accept netdev as a deprecated alias.

> 
> > I'd be inclined to use whatever name we decide here in the libxl
> > API/internals as well and just go netdev at the hotplug script
> > interface.
> > 
> > Ian.
> > 
> > 
> 

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

[Ian Campbell]

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

env[nr++] = netdev ? : "" might suffice?

Ian.

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

[Roger Pau Monné]

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.

> env[nr++] = netdev ? : "" might suffice?

Yes.

> Ian.
> 

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

[Ian Campbell]

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 

Re: [PATCH 0/2] add vif-route support to libxl/xl

[Ian Campbell]

On Fri, 2013-01-25 at 15:26 +0000, Roger Pau Monne wrote:
> Support for vif-route was missing in libxl, this patch series adds a 
> "netdev" vif option and also allows setting a default netdev in 
> xl.conf global configuration file.
> 
> vif-route doesn't support HVM domains because it doesn't support the 
> "add" or "remove" actions.

Should be easy to add?

> I would like to request some testing from people that actually use 
> this network mode.

Might be worth sending a Call-For-Testing to -users@ ?

Ian.