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.
Roger Pau Monne
2013-Jan-25 15:26 UTC
[PATCH 1/2] xl/libxl: add netdev to vif specification
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
Roger Pau Monne
2013-Jan-25 15:26 UTC
[PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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
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.
George Dunlap
2013-Jan-28 11:00 UTC
Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
George Dunlap
2013-Jan-28 17:11 UTC
Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Roger Pau Monné
2013-Jan-29 11:01 UTC
Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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).
Ian Campbell
2013-Feb-05 10:40 UTC
Re: [PATCH 1/2] xl/libxl: add netdev to vif specification
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
Ian Campbell
2013-Feb-05 10:41 UTC
Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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.
Roger Pau Monné
2013-Feb-05 10:56 UTC
Re: [PATCH 1/2] xl/libxl: add netdev to vif specification
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
Roger Pau Monné
2013-Feb-05 11:00 UTC
Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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. > >
Ian Campbell
2013-Feb-05 11:09 UTC
Re: [PATCH 2/2] xl: allow specifying a default netdev in xl.conf
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. > > > > >
Ian Campbell
2013-Feb-05 11:10 UTC
Re: [PATCH 1/2] xl/libxl: add netdev to vif specification
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.
Roger Pau Monné
2013-Feb-05 11:39 UTC
Re: [PATCH 1/2] xl/libxl: add netdev to vif specification
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. >
Ian Campbell
2013-Feb-05 11:41 UTC
Re: [PATCH 1/2] xl/libxl: add netdev to vif specification
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
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.