All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] add vif-route support to libxl/xl
@ 2013-02-06 18:04 Roger Pau Monne
  2013-02-06 18:04 ` [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification Roger Pau Monne
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Roger Pau Monne @ 2013-02-06 18:04 UTC (permalink / raw)
  To: xen-devel

Support for vif-route was missing in libxl, this patch series adds a
gatewaydev/netdev vif option and also allows setting a default
gatewaydev 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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification
  2013-02-06 18:04 [PATCH v2 0/4] add vif-route support to libxl/xl Roger Pau Monne
@ 2013-02-06 18:04 ` Roger Pau Monne
  2013-03-12 16:20   ` Ian Campbell
  2013-02-06 18:04 ` [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf Roger Pau Monne
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2013-02-06 18:04 UTC (permalink / raw)
  To: xen-devel; +Cc: Ulf Kreutzberg, George Dunlap, Ian Campbell, Roger Pau Monne

This option is used by the vif-route hotplug script. A new more
descriptive name is used, "gatewaydev", but "netdev" is also supported
as a deprecated backwards compatible option.

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>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
---
Changes since v1:
 * Don't pass a NULL as a value of a env variable
 * Change netdev to gatewaydev
---
 docs/misc/xl-network-configuration.markdown |   10 ++++++++++
 tools/libxl/libxl.c                         |    6 +++++-
 tools/libxl/libxl_linux.c                   |    9 ++++++++-
 tools/libxl/libxl_types.idl                 |    1 +
 tools/libxl/xl_cmdimpl.c                    |   14 ++++++++++++++
 5 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown
index 5e2f049..e0d3d2a 100644
--- a/docs/misc/xl-network-configuration.markdown
+++ b/docs/misc/xl-network-configuration.markdown
@@ -67,6 +67,15 @@ 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.
 
+### gatewaydev
+
+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 in the host
+by the vif-route hotplug script. See [wiki][vifroute] for guidance and
+examples.
+
+NOTE: netdev is a deprecated alias of this option.
+
 ### type
 
 This keyword is valid for HVM guests only.
@@ -158,3 +167,4 @@ on the underlying netback implementation.
 
 [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier
 [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking
+[vifroute]: http://wiki.xen.org/wiki/Vif-route
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 73e0dc3..5d590f1 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->gatewaydev) {
+        flexarray_append(back, "gatewaydev");
+        flexarray_append(back, libxl__strdup(gc, nic->gatewaydev));
+    }
 
     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..60fc533 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 *gatewaydev;
     int nr = 0;
     libxl_nic_type nictype;
 
-    const int arraysize = 13;
+    gatewaydev = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s", be_path,
+                                                        "gatewaydev"));
+
+    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++] = gatewaydev ? : "";
     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..0112a7a 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),
+    ("gatewaydev", string),
     ])
 
 libxl_device_pci = Struct("device_pci", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 080bbd8..dc1788e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1205,6 +1205,14 @@ 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")) {
+                    fprintf(stderr, "the netdev parameter is deprecated, "
+                                    "please use gatewaydev instead\n");
+                    free(nic->gatewaydev);
+                    nic->gatewaydev = strdup(p2 + 1);
+                } else if (!strcmp(p, "gatewaydev")) {
+                    free(nic->gatewaydev);
+                    nic->gatewaydev = strdup(p2 + 1);
                 }
             } while ((p = strtok(NULL, ",")) != NULL);
 skip_nic:
@@ -5471,6 +5479,12 @@ 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)) {
+            fprintf(stderr, "the netdev parameter is deprecated, "
+                            "please use gatewaydev instead\n");
+            replace_string(&nic.gatewaydev, oparg);
+        } else if (MATCH_OPTION("gatewaydev", *argv, oparg)) {
+            replace_string(&nic.gatewaydev, 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-06 18:04 [PATCH v2 0/4] add vif-route support to libxl/xl Roger Pau Monne
  2013-02-06 18:04 ` [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification Roger Pau Monne
@ 2013-02-06 18:04 ` Roger Pau Monne
  2013-02-14 15:09   ` Ulf Kreutzberg
  2013-03-12 16:22   ` Ian Campbell
  2013-02-06 18:04 ` [PATCH v2 3/4] xl: add vif.default.bridge Roger Pau Monne
  2013-02-06 18:04 ` [PATCH v2 4/4] xl: add vif.default.script Roger Pau Monne
  3 siblings, 2 replies; 20+ messages in thread
From: Roger Pau Monne @ 2013-02-06 18:04 UTC (permalink / raw)
  To: xen-devel; +Cc: Ulf Kreutzberg, George Dunlap, Ian Campbell, Roger Pau Monne

This adds a new global option in the xl configuration file called
"vif.default.gatewaydev", that is used to specify the default
gatewaydev 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>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
---
Changes since v1:
 * Rename defaultnetdev to vif.default.gatewaydev
---
 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..f8f7e7f 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<vif.default.gatewaydev="NAME">
+
+Configures the default gateway device 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..9a03fff 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 gateway device to use with vif-route hotplug script
+#vif.default.gatewaydev="eth0"
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index ecbcd3b..f657216 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_gatewaydev = 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, "vif.default.gatewaydev", &buf, 0))
+        default_gatewaydev = 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..b881f92 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_gatewaydev;
 extern char *blkdev_start;
 
 enum output_format {
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index dc1788e..7796498 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_gatewaydev) {
+                free(nic->gatewaydev);
+                nic->gatewaydev = strdup(default_gatewaydev);
+            }
+
             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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 3/4] xl: add vif.default.bridge
  2013-02-06 18:04 [PATCH v2 0/4] add vif-route support to libxl/xl Roger Pau Monne
  2013-02-06 18:04 ` [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification Roger Pau Monne
  2013-02-06 18:04 ` [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf Roger Pau Monne
@ 2013-02-06 18:04 ` Roger Pau Monne
  2013-03-12 16:25   ` Ian Campbell
  2013-02-06 18:04 ` [PATCH v2 4/4] xl: add vif.default.script Roger Pau Monne
  3 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2013-02-06 18:04 UTC (permalink / raw)
  To: xen-devel; +Cc: George Dunlap, Ian Campbell, Roger Pau Monne

This is a replacement for defaultbridge xl.conf option. The now
deprecated defaultbridge is still supported.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/examples/xl.conf |    3 +++
 tools/libxl/xl.c       |   13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 9a03fff..4451176 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -23,3 +23,6 @@
 
 # default gateway device to use with vif-route hotplug script
 #vif.default.gatewaydev="eth0"
+
+# default bridge device to use with vif-bridge hotplug scripts
+#vif.default.bridge="bridge0"
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index f657216..100ab32 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -89,8 +89,17 @@ static void parse_global_config(const char *configfile,
     if (!xlu_cfg_get_string (config, "vifscript", &buf, 0))
         default_vifscript = strdup(buf);
 
-    if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0))
-	default_bridge = strdup(buf);
+    if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0)) {
+        fprintf(stderr, "the global config option defaultbridge is deprecated, "
+                        "please switch to vif.default.bridge\n");
+        free(default_bridge);
+        default_bridge = strdup(buf);
+    }
+
+    if (!xlu_cfg_get_string (config, "vif.default.bridge", &buf, 0)) {
+        free(default_bridge);
+        default_bridge = strdup(buf);
+    }
 
     if (!xlu_cfg_get_string (config, "vif.default.gatewaydev", &buf, 0))
         default_gatewaydev = strdup(buf);
-- 
1.7.7.5 (Apple Git-26)


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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 4/4] xl: add vif.default.script
  2013-02-06 18:04 [PATCH v2 0/4] add vif-route support to libxl/xl Roger Pau Monne
                   ` (2 preceding siblings ...)
  2013-02-06 18:04 ` [PATCH v2 3/4] xl: add vif.default.bridge Roger Pau Monne
@ 2013-02-06 18:04 ` Roger Pau Monne
  2013-02-07 11:31   ` George Dunlap
  2013-03-12 16:25   ` Ian Campbell
  3 siblings, 2 replies; 20+ messages in thread
From: Roger Pau Monne @ 2013-02-06 18:04 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Campbell, George Dunlap, Roger Pau Monne

Replace vifscript with vif.default.script. The old config option is
kept for backwards compatibility.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 100ab32..92565d1 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -86,8 +86,17 @@ static void parse_global_config(const char *configfile,
         exit(1);
     }
 
-    if (!xlu_cfg_get_string (config, "vifscript", &buf, 0))
+    if (!xlu_cfg_get_string (config, "vifscript", &buf, 0)) {
+        fprintf(stderr, "the global config option vifscript is deprecated, "
+                        "please switch to vif.default.script\n");
+        free(default_vifscript);
         default_vifscript = strdup(buf);
+    }
+
+    if (!xlu_cfg_get_string (config, "vif.default.script", &buf, 0)) {
+        free(default_vifscript);
+        default_vifscript = strdup(buf);
+    }
 
     if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0)) {
         fprintf(stderr, "the global config option defaultbridge is deprecated, "
-- 
1.7.7.5 (Apple Git-26)


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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 4/4] xl: add vif.default.script
  2013-02-06 18:04 ` [PATCH v2 4/4] xl: add vif.default.script Roger Pau Monne
@ 2013-02-07 11:31   ` George Dunlap
  2013-03-12 16:25   ` Ian Campbell
  1 sibling, 0 replies; 20+ messages in thread
From: George Dunlap @ 2013-02-07 11:31 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Ian Campbell, xen-devel

On 06/02/13 18:04, Roger Pau Monne wrote:
> Replace vifscript with vif.default.script. The old config option is
> kept for backwards compatibility.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>

I haven't done a detailed review of the code, but the naming looks fine 
to me.

  -George


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-06 18:04 ` [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf Roger Pau Monne
@ 2013-02-14 15:09   ` Ulf Kreutzberg
  2013-02-14 15:17     ` Roger Pau Monné
  2013-03-12 16:22   ` Ian Campbell
  1 sibling, 1 reply; 20+ messages in thread
From: Ulf Kreutzberg @ 2013-02-14 15:09 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: George Dunlap, Ian Campbell, xen-devel


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

Roger,


after applying patch v2 1/4 and 2/4 (are there missing some?)
to git checkout c23ea051ccee613e668b2a87817d49a28215ac8b of
git://xenbits.xen.org/xen.git xen

I get the following error(s) during compiling with 'make tools':

In file included from libxl.h:355,
                 from xl_cmdtable.c:17:
_libxl_types.h:437: error: duplicate member ‘gatewaydev’
make[3]: *** [xl_cmdtable.o] Error 1


On 06.02.2013 19:04, Roger Pau Monne wrote:
> This adds a new global option in the xl configuration file called
> "vif.default.gatewaydev", that is used to specify the default
> gatewaydev to use when none is passed in the vif specification.
> 

Thanks and best regards,
Ulf


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-14 15:09   ` Ulf Kreutzberg
@ 2013-02-14 15:17     ` Roger Pau Monné
  2013-02-15 14:06       ` Ulf Kreutzberg
  0 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monné @ 2013-02-14 15:17 UTC (permalink / raw)
  To: Ulf Kreutzberg; +Cc: George Dunlap, Ian Campbell, xen-devel

On 14/02/13 16:09, Ulf Kreutzberg wrote:
> Roger,
> 
> 
> after applying patch v2 1/4 and 2/4 (are there missing some?)

Well, you might want to try 3/4 and 4/4 also, but they are not mandatory.

> to git checkout c23ea051ccee613e668b2a87817d49a28215ac8b of
> git://xenbits.xen.org/xen.git xen
> 
> I get the following error(s) during compiling with 'make tools':
> 
> In file included from libxl.h:355,
>                  from xl_cmdtable.c:17:
> _libxl_types.h:437: error: duplicate member ‘gatewaydev’
> make[3]: *** [xl_cmdtable.o] Error 1

Could you take a look at tools/libxl/libxl_types.idl?

You should have something like the following chunk:

libxl_device_nic = Struct("device_nic", [
    ("backend_domid", libxl_domid),
    ("devid", libxl_devid),
    ("mtu", integer),
    ("model", string),
    ("mac", libxl_mac),
    ("ip", string),
    ("bridge", string),
    ("ifname", string),
    ("script", string),
    ("nictype", libxl_nic_type),
    ("rate_bytes_per_interval", uint64),
    ("rate_interval_usecs", uint32),
    ("gatewaydev", string),
    ])

(With only one "gatewaydev").

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-14 15:17     ` Roger Pau Monné
@ 2013-02-15 14:06       ` Ulf Kreutzberg
  2013-02-18  7:38         ` Roger Pau Monné
  0 siblings, 1 reply; 20+ messages in thread
From: Ulf Kreutzberg @ 2013-02-15 14:06 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: George Dunlap, Ian Campbell, xen-devel


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

Hello Roger,
hello everybody,

sorry for the late reply.

On 14.02.2013 16:17, Roger Pau Monné wrote:
> Could you take a look at tools/libxl/libxl_types.idl?
>
> You should have something like the following chunk:
>
> libxl_device_nic = Struct("device_nic", [
>     ("backend_domid", libxl_domid),
>     ("devid", libxl_devid),
>     ("mtu", integer),
>     ("model", string),
>     ("mac", libxl_mac),
>     ("ip", string),
>     ("bridge", string),
>     ("ifname", string),
>     ("script", string),
>     ("nictype", libxl_nic_type),
>     ("rate_bytes_per_interval", uint64),
>     ("rate_interval_usecs", uint32),
>     ("gatewaydev", string),
>     ])
>
> (With only one "gatewaydev").
>
That was my copy'n'paste mistake - fixed.

I could see that after applying patches V1 the parameter "netdev" is
parsed correctly and the main ip is passed to xen scripts.
However, I could not get xen 4.3 running so I had to test 4.3 tools on
4.2.1 hypervisor, which could not start the domU, but debug output of
the scripts showed me everything is parsed as expected.

The mac address still is not parsed if digits are not padded with
leading zero, like "mac=de:ad:a:1e:42:3" - the rest of the line is
ignored without any error.

However, I was not able to apply patches V2 after that, they do not seem
to work together with the first patch series.
Only patching with V2 resulted in missing "netdev" member in
libxl_device_nic (libxl_types.idl). After editing the file and adding
the definition, it compiled so far.

xl with only patches V2 applied does parse neither netdev nor gatewaydev
as it seems.


Best regards,
Ulf



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-15 14:06       ` Ulf Kreutzberg
@ 2013-02-18  7:38         ` Roger Pau Monné
  2013-02-19 16:04           ` Ulf Kreutzberg
  0 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monné @ 2013-02-18  7:38 UTC (permalink / raw)
  To: Ulf Kreutzberg; +Cc: George Dunlap, Ian Campbell, xen-devel

On 15/02/13 15:06, Ulf Kreutzberg wrote:
> Hello Roger,
> hello everybody,
> 
> sorry for the late reply.
> 
> On 14.02.2013 16:17, Roger Pau Monné wrote:
>> Could you take a look at tools/libxl/libxl_types.idl?
>>
>> You should have something like the following chunk:
>>
>> libxl_device_nic = Struct("device_nic", [
>>     ("backend_domid", libxl_domid),
>>     ("devid", libxl_devid),
>>     ("mtu", integer),
>>     ("model", string),
>>     ("mac", libxl_mac),
>>     ("ip", string),
>>     ("bridge", string),
>>     ("ifname", string),
>>     ("script", string),
>>     ("nictype", libxl_nic_type),
>>     ("rate_bytes_per_interval", uint64),
>>     ("rate_interval_usecs", uint32),
>>     ("gatewaydev", string),
>>     ])
>>
>> (With only one "gatewaydev").
>>
> That was my copy'n'paste mistake - fixed.
> 
> I could see that after applying patches V1 the parameter "netdev" is
> parsed correctly and the main ip is passed to xen scripts.
> However, I could not get xen 4.3 running so I had to test 4.3 tools on
> 4.2.1 hypervisor, which could not start the domU, but debug output of
> the scripts showed me everything is parsed as expected.
> 
> The mac address still is not parsed if digits are not padded with
> leading zero, like "mac=de:ad:a:1e:42:3" - the rest of the line is
> ignored without any error.
> 
> However, I was not able to apply patches V2 after that, they do not seem
> to work together with the first patch series.

Hello Ulf,

Version v2 should be applied without v1, they are not additional
patches, it's a revision of the same patches (trying to apply both will
lead to errors). To simplify it I've pushed the patches to my git repo,
so you can fetch them easily:

$ git clone -b vif_route_v2 git://xenbits.xen.org/people/royger/xen.git

More information about how to build Xen from source can be found at
http://wiki.xen.org/wiki/Compiling_Xen_From_Source#Building_from_Source.

Hope this helps, Roger.

> Only patching with V2 resulted in missing "netdev" member in
> libxl_device_nic (libxl_types.idl). After editing the file and adding
> the definition, it compiled so far.
> 
> xl with only patches V2 applied does parse neither netdev nor gatewaydev
> as it seems.
> 
> 
> Best regards,
> Ulf
> 
> 


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-18  7:38         ` Roger Pau Monné
@ 2013-02-19 16:04           ` Ulf Kreutzberg
  2013-03-04 11:02             ` Roger Pau Monné
  0 siblings, 1 reply; 20+ messages in thread
From: Ulf Kreutzberg @ 2013-02-19 16:04 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: George Dunlap, Ian Campbell, xen-devel


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

Hello Roger,
hi all,

On 18.02.2013 08:38, Roger Pau Monné wrote:
>> The mac address still is not parsed if digits are not padded with
>> leading zero, like "mac=de:ad:a:1e:42:3" - the rest of the line is
>> ignored without any error.
>>
>> However, I was not able to apply patches V2 after that, they do not seem
>> to work together with the first patch series.
> 
> Hello Ulf,
> 
> Version v2 should be applied without v1, they are not additional
> patches, it's a revision of the same patches (trying to apply both will
> lead to errors). To simplify it I've pushed the patches to my git repo,
> so you can fetch them easily:
> 
> $ git clone -b vif_route_v2 git://xenbits.xen.org/people/royger/xen.git
> 

Many thanks, building that tree definitely works. (Still could not get
xen 4.3 running, but that is not the issue here). I have tested the
utils with Xen 4.2.1 and parsing of domu configs with netdev= and
gatewaydev= parameter works perfectly. Domu does start up and is reacheable.
I wonder if you could backport these changes to 4.2?

One caveat: the mac address and therfore the rest of the domu config
still is not parsed if not padded with leading zeros, as mentioned before.

Best regards,
Ulf


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-19 16:04           ` Ulf Kreutzberg
@ 2013-03-04 11:02             ` Roger Pau Monné
  2013-03-12 16:39               ` Ulf Kreutzberg
  0 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monné @ 2013-03-04 11:02 UTC (permalink / raw)
  To: Ulf Kreutzberg; +Cc: George Dunlap, Ian Jackson, Ian Campbell, xen-devel

On 19/02/13 17:04, Ulf Kreutzberg wrote:
> Hello Roger,
> hi all,
> 
> On 18.02.2013 08:38, Roger Pau Monné wrote:
>>> The mac address still is not parsed if digits are not padded with
>>> leading zero, like "mac=de:ad:a:1e:42:3" - the rest of the line is
>>> ignored without any error.
>>>
>>> However, I was not able to apply patches V2 after that, they do not seem
>>> to work together with the first patch series.
>>
>> Hello Ulf,
>>
>> Version v2 should be applied without v1, they are not additional
>> patches, it's a revision of the same patches (trying to apply both will
>> lead to errors). To simplify it I've pushed the patches to my git repo,
>> so you can fetch them easily:
>>
>> $ git clone -b vif_route_v2 git://xenbits.xen.org/people/royger/xen.git
>>
> 
> Many thanks, building that tree definitely works. (Still could not get
> xen 4.3 running, but that is not the issue here). I have tested the
> utils with Xen 4.2.1 and parsing of domu configs with netdev= and
> gatewaydev= parameter works perfectly. Domu does start up and is reacheable.
> I wonder if you could backport these changes to 4.2?

Are you OK if I add your "Tested-by"?

Could you please take a look at the patches when you can IanJ? I would
like to get this off my patch queue :)

> One caveat: the mac address and therfore the rest of the domu config
> still is not parsed if not padded with leading zeros, as mentioned before.
> 
> Best regards,
> Ulf
> 


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification
  2013-02-06 18:04 ` [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification Roger Pau Monne
@ 2013-03-12 16:20   ` Ian Campbell
  0 siblings, 0 replies; 20+ messages in thread
From: Ian Campbell @ 2013-03-12 16:20 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: George Dunlap, Ulf Kreutzberg, xen-devel

On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
> This option is used by the vif-route hotplug script. A new more
> descriptive name is used, "gatewaydev", but "netdev" is also supported
> as a deprecated backwards compatible option.
> 
> 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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> Cc: George Dunlap <george.dunlap@citrix.com>
> ---
> Changes since v1:
>  * Don't pass a NULL as a value of a env variable
>  * Change netdev to gatewaydev
> ---
>  docs/misc/xl-network-configuration.markdown |   10 ++++++++++
>  tools/libxl/libxl.c                         |    6 +++++-
>  tools/libxl/libxl_linux.c                   |    9 ++++++++-
>  tools/libxl/libxl_types.idl                 |    1 +
>  tools/libxl/xl_cmdimpl.c                    |   14 ++++++++++++++
>  5 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown
> index 5e2f049..e0d3d2a 100644
> --- a/docs/misc/xl-network-configuration.markdown
> +++ b/docs/misc/xl-network-configuration.markdown
> @@ -67,6 +67,15 @@ 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.
>  
> +### gatewaydev
> +
> +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 in the host
> +by the vif-route hotplug script. See [wiki][vifroute] for guidance and
> +examples.
> +
> +NOTE: netdev is a deprecated alias of this option.
> +
>  ### type
>  
>  This keyword is valid for HVM guests only.
> @@ -158,3 +167,4 @@ on the underlying netback implementation.
>  
>  [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier
>  [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking
> +[vifroute]: http://wiki.xen.org/wiki/Vif-route
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 73e0dc3..5d590f1 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->gatewaydev) {
> +        flexarray_append(back, "gatewaydev");
> +        flexarray_append(back, libxl__strdup(gc, nic->gatewaydev));
> +    }
>  
>      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..60fc533 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 *gatewaydev;
>      int nr = 0;
>      libxl_nic_type nictype;
>  
> -    const int arraysize = 13;
> +    gatewaydev = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s", be_path,
> +                                                        "gatewaydev"));
> +
> +    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++] = gatewaydev ? : "";
>      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..0112a7a 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),
> +    ("gatewaydev", string),
>      ])
>  
>  libxl_device_pci = Struct("device_pci", [
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 080bbd8..dc1788e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1205,6 +1205,14 @@ 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")) {
> +                    fprintf(stderr, "the netdev parameter is deprecated, "
> +                                    "please use gatewaydev instead\n");
> +                    free(nic->gatewaydev);
> +                    nic->gatewaydev = strdup(p2 + 1);
> +                } else if (!strcmp(p, "gatewaydev")) {
> +                    free(nic->gatewaydev);
> +                    nic->gatewaydev = strdup(p2 + 1);
>                  }
>              } while ((p = strtok(NULL, ",")) != NULL);
>  skip_nic:
> @@ -5471,6 +5479,12 @@ 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)) {
> +            fprintf(stderr, "the netdev parameter is deprecated, "
> +                            "please use gatewaydev instead\n");
> +            replace_string(&nic.gatewaydev, oparg);
> +        } else if (MATCH_OPTION("gatewaydev", *argv, oparg)) {
> +            replace_string(&nic.gatewaydev, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-02-06 18:04 ` [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf Roger Pau Monne
  2013-02-14 15:09   ` Ulf Kreutzberg
@ 2013-03-12 16:22   ` Ian Campbell
  2013-03-12 16:27     ` Ian Campbell
  1 sibling, 1 reply; 20+ messages in thread
From: Ian Campbell @ 2013-03-12 16:22 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: George Dunlap, Ulf Kreutzberg, xen-devel

On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
> This adds a new global option in the xl configuration file called
> "vif.default.gatewaydev", that is used to specify the default
> gatewaydev 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>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> ---
> Changes since v1:
>  * Rename defaultnetdev to vif.default.gatewaydev

That whole file is a bit uncomfortable ad-hoc in its naming, but oh
well.

Acked-by: Ian Campbell <ian.campbell@citrix.com>



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] xl: add vif.default.bridge
  2013-02-06 18:04 ` [PATCH v2 3/4] xl: add vif.default.bridge Roger Pau Monne
@ 2013-03-12 16:25   ` Ian Campbell
  2013-03-13 16:40     ` Roger Pau Monné
  0 siblings, 1 reply; 20+ messages in thread
From: Ian Campbell @ 2013-03-12 16:25 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: George Dunlap, xen-devel

On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
> This is a replacement for defaultbridge xl.conf option. The now
> deprecated defaultbridge is still supported.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/examples/xl.conf |    3 +++
>  tools/libxl/xl.c       |   13 +++++++++++--

No change to docs/man/xl.conf.pod.5? 2 files changed, 14 insertions(+),
2 deletions(-)
> 
> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
> index 9a03fff..4451176 100644
> --- a/tools/examples/xl.conf
> +++ b/tools/examples/xl.conf
> @@ -23,3 +23,6 @@
>  
>  # default gateway device to use with vif-route hotplug script
>  #vif.default.gatewaydev="eth0"
> +
> +# default bridge device to use with vif-bridge hotplug scripts
> +#vif.default.bridge="bridge0"

Common names (at least in Linux-land) are "xenbr0" and "br0". Unless
this conflicts massively with *BSD perhaps using one of those would be
useful?


> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index f657216..100ab32 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -89,8 +89,17 @@ static void parse_global_config(const char *configfile,
>      if (!xlu_cfg_get_string (config, "vifscript", &buf, 0))
>          default_vifscript = strdup(buf);
>  
> -    if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0))
> -	default_bridge = strdup(buf);
> +    if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0)) {
> +        fprintf(stderr, "the global config option defaultbridge is deprecated, "
> +                        "please switch to vif.default.bridge\n");
> +        free(default_bridge);
> +        default_bridge = strdup(buf);
> +    }
> +
> +    if (!xlu_cfg_get_string (config, "vif.default.bridge", &buf, 0)) {
> +        free(default_bridge);
> +        default_bridge = strdup(buf);
> +    }

Put else if (!xlu.... ("defaultbridge")... ) here instead of above so we
only warn if the user gave defaultbridge but not vif.default.bridge?

Ian.


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 4/4] xl: add vif.default.script
  2013-02-06 18:04 ` [PATCH v2 4/4] xl: add vif.default.script Roger Pau Monne
  2013-02-07 11:31   ` George Dunlap
@ 2013-03-12 16:25   ` Ian Campbell
  1 sibling, 0 replies; 20+ messages in thread
From: Ian Campbell @ 2013-03-12 16:25 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: George Dunlap, xen-devel

On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
> Replace vifscript with vif.default.script. The old config option is
> kept for backwards compatibility.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/xl.c |   11 ++++++++++-

Needs docs changes too I expect.

>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 100ab32..92565d1 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -86,8 +86,17 @@ static void parse_global_config(const char *configfile,
>          exit(1);
>      }
>  
> -    if (!xlu_cfg_get_string (config, "vifscript", &buf, 0))
> +    if (!xlu_cfg_get_string (config, "vifscript", &buf, 0)) {
> +        fprintf(stderr, "the global config option vifscript is deprecated, "
> +                        "please switch to vif.default.script\n");
> +        free(default_vifscript);
>          default_vifscript = strdup(buf);
> +    }
> +
> +    if (!xlu_cfg_get_string (config, "vif.default.script", &buf, 0)) {
> +        free(default_vifscript);
> +        default_vifscript = strdup(buf);
> +    }

Same comment as 3/4 about checking the deprecated name second.



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-03-12 16:22   ` Ian Campbell
@ 2013-03-12 16:27     ` Ian Campbell
  2013-03-13 16:29       ` Roger Pau Monné
  0 siblings, 1 reply; 20+ messages in thread
From: Ian Campbell @ 2013-03-12 16:27 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: George Dunlap, Ulf Kreutzberg, xen-devel

On Tue, 2013-03-12 at 16:22 +0000, Ian Campbell wrote:
> On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
> > This adds a new global option in the xl configuration file called
> > "vif.default.gatewaydev", that is used to specify the default
> > gatewaydev 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>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: George Dunlap <george.dunlap@citrix.com>
> > ---
> > Changes since v1:
> >  * Rename defaultnetdev to vif.default.gatewaydev
> 
> That whole file is a bit uncomfortable ad-hoc in its naming, but oh
> well.

Although if you fancied writing down some sort of simple "schema" (which
is far too grand a word for what I'm thinking of) which fits the scheme
you are using here and could serve as guidance for future new variable
names, that would be very nice.



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-03-04 11:02             ` Roger Pau Monné
@ 2013-03-12 16:39               ` Ulf Kreutzberg
  0 siblings, 0 replies; 20+ messages in thread
From: Ulf Kreutzberg @ 2013-03-12 16:39 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: George Dunlap, Ian Jackson, Ian Campbell, xen-devel


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

Roger,


sorry for the very-late reply...

On 04.03.2013 12:02, Roger Pau Monné wrote:
> 
> Are you OK if I add your "Tested-by"?
> 
That is OK, of course.

Thanks and regards,
Ulf



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf
  2013-03-12 16:27     ` Ian Campbell
@ 2013-03-13 16:29       ` Roger Pau Monné
  0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2013-03-13 16:29 UTC (permalink / raw)
  To: Ian Campbell; +Cc: George Dunlap, Ulf Kreutzberg, xen-devel

On 12/03/13 17:27, Ian Campbell wrote:
> On Tue, 2013-03-12 at 16:22 +0000, Ian Campbell wrote:
>> On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
>>> This adds a new global option in the xl configuration file called
>>> "vif.default.gatewaydev", that is used to specify the default
>>> gatewaydev 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>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: George Dunlap <george.dunlap@citrix.com>
>>> ---
>>> Changes since v1:
>>>  * Rename defaultnetdev to vif.default.gatewaydev
>>
>> That whole file is a bit uncomfortable ad-hoc in its naming, but oh
>> well.
> 
> Although if you fancied writing down some sort of simple "schema" (which
> is far too grand a word for what I'm thinking of) which fits the scheme
> you are using here and could serve as guidance for future new variable
> names, that would be very nice.

Thanks for the review, I guess the best place for this would be a
comment on the code next to the section that parses the global options.


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] xl: add vif.default.bridge
  2013-03-12 16:25   ` Ian Campbell
@ 2013-03-13 16:40     ` Roger Pau Monné
  0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2013-03-13 16:40 UTC (permalink / raw)
  To: Ian Campbell; +Cc: George Dunlap, xen-devel

On 12/03/13 17:25, Ian Campbell wrote:
> On Wed, 2013-02-06 at 18:04 +0000, Roger Pau Monne wrote:
>> This is a replacement for defaultbridge xl.conf option. The now
>> deprecated defaultbridge is still supported.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> ---
>>  tools/examples/xl.conf |    3 +++
>>  tools/libxl/xl.c       |   13 +++++++++++--
> 
> No change to docs/man/xl.conf.pod.5? 

My bad, I'm going to add it now.

> 2 files changed, 14 insertions(+),
> 2 deletions(-)
>>
>> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
>> index 9a03fff..4451176 100644
>> --- a/tools/examples/xl.conf
>> +++ b/tools/examples/xl.conf
>> @@ -23,3 +23,6 @@
>>  
>>  # default gateway device to use with vif-route hotplug script
>>  #vif.default.gatewaydev="eth0"
>> +
>> +# default bridge device to use with vif-bridge hotplug scripts
>> +#vif.default.bridge="bridge0"
> 
> Common names (at least in Linux-land) are "xenbr0" and "br0". Unless
> this conflicts massively with *BSD perhaps using one of those would be
> useful?

I use bridgeX on both Linux and BSD, but that's just my choice. br0 or
xenbr0 is probably more common, so I'm going to change it.

> 
> 
>> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
>> index f657216..100ab32 100644
>> --- a/tools/libxl/xl.c
>> +++ b/tools/libxl/xl.c
>> @@ -89,8 +89,17 @@ static void parse_global_config(const char *configfile,
>>      if (!xlu_cfg_get_string (config, "vifscript", &buf, 0))
>>          default_vifscript = strdup(buf);
>>  
>> -    if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0))
>> -	default_bridge = strdup(buf);
>> +    if (!xlu_cfg_get_string (config, "defaultbridge", &buf, 0)) {
>> +        fprintf(stderr, "the global config option defaultbridge is deprecated, "
>> +                        "please switch to vif.default.bridge\n");
>> +        free(default_bridge);
>> +        default_bridge = strdup(buf);
>> +    }
>> +
>> +    if (!xlu_cfg_get_string (config, "vif.default.bridge", &buf, 0)) {
>> +        free(default_bridge);
>> +        default_bridge = strdup(buf);
>> +    }
> 
> Put else if (!xlu.... ("defaultbridge")... ) here instead of above so we
> only warn if the user gave defaultbridge but not vif.default.bridge?

Since "defaultbridge" is now deprecated I think we should warn the user
about it, even if vif.default.bridge is also specified (which also
doesn't make much sense to use both)


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2013-03-13 16:40 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-06 18:04 [PATCH v2 0/4] add vif-route support to libxl/xl Roger Pau Monne
2013-02-06 18:04 ` [PATCH v2 1/4] xl/libxl: add gatewaydev/netdev to vif specification Roger Pau Monne
2013-03-12 16:20   ` Ian Campbell
2013-02-06 18:04 ` [PATCH v2 2/4] xl: allow specifying a default gatewaydev in xl.conf Roger Pau Monne
2013-02-14 15:09   ` Ulf Kreutzberg
2013-02-14 15:17     ` Roger Pau Monné
2013-02-15 14:06       ` Ulf Kreutzberg
2013-02-18  7:38         ` Roger Pau Monné
2013-02-19 16:04           ` Ulf Kreutzberg
2013-03-04 11:02             ` Roger Pau Monné
2013-03-12 16:39               ` Ulf Kreutzberg
2013-03-12 16:22   ` Ian Campbell
2013-03-12 16:27     ` Ian Campbell
2013-03-13 16:29       ` Roger Pau Monné
2013-02-06 18:04 ` [PATCH v2 3/4] xl: add vif.default.bridge Roger Pau Monne
2013-03-12 16:25   ` Ian Campbell
2013-03-13 16:40     ` Roger Pau Monné
2013-02-06 18:04 ` [PATCH v2 4/4] xl: add vif.default.script Roger Pau Monne
2013-02-07 11:31   ` George Dunlap
2013-03-12 16:25   ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.