Laine Stump wrote:

On 06/11/2014 12:25 AM, Jim Fehlig wrote:

...

Add support for <interface type='network'> in the libxl driver.

Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---

V2: Support both libvirt's traditional managed networks and libvirt networks that use an existing host bridge.

src/libxl/libxl_conf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cec37d6..9c453d8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -874,6 +874,7 @@ libxlMakeNic(virDomainDefPtr def, libxl_device_nic *x_nic) { bool ioemu_nic = STREQ(def->os.type, "hvm"); + virDomainNetType actual_type = virDomainNetGetActualType(l_nic);

/* TODO: Where is mtu stored? * @@ -899,16 +900,60 @@ libxlMakeNic(virDomainDefPtr def, if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0) return -1;

- switch (l_nic->type) { + switch (actual_type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: - if (VIR_STRDUP(x_nic->bridge, l_nic->data.bridge.brname) < 0) + if (VIR_STRDUP(x_nic->bridge, + virDomainNetGetActualBridgeName(l_nic)) < 0) return -1; /* fallthrough */ case VIR_DOMAIN_NET_TYPE_ETHERNET: if (VIR_STRDUP(x_nic->script, l_nic->script) < 0) return -1; break; - default: + case VIR_DOMAIN_NET_TYPE_NETWORK: + { + bool fail = false; + char *brname = NULL; + virNetworkPtr network; + virConnectPtr conn; + virErrorPtr errobj; + + if (!(conn = virConnectOpen("xen:///system"))) + return -1;

(The conn is necessary in order to call the functions that retrieve the bridge device name for the network)

One difference in the way that qemu deals with this is that in most cases the higher level function already has a conn ptr, and that is passed down the call stack to the user. There is one case where this doesn't happen - the autostart of a domain. In that case, qemu calls virConnectOpen(cfg->uri) to get a conn.

Right, and that depends on whether running privileged or not. From qemu_conf.c, virQEMUDriverConfigNew: cfg->uri = privileged ? "qemu:///system" : "qemu:///session";

In this case, you are always creating a new conn, even when one may already exist a few layers up the call stack,

I originally looked into passing conn to libxlMakeNic, but it is actually quite a bit of code refactoring. I think that should be done in conjunction with decomposing libxlDomainStart a bit. Currently that function builds the domain config and handles the details of starting the domain. I'd like to pull the build part out, allowing a cleaner approach to passing conn where needed. E.g. I could see conn being used in libxlMakeDisk some day.

and the conn you create is using the hardcoded "xen:///system" rather than taking something from config.

As long as this is okay with you, it's okay with me. I just wonder about two things:

I think it is okay.

1) does xen support non-privileged libvirt, and if so what would happen in this case (I suppose it would just fail due to lack of authorization).

Only supports privileged.

2) is the uri configurable anywhere, as it is for libvirt?

Do you mean s/libvirt/the qemu driver/ ? As mentioned above, cfg->uri depends on privileged in the qemu driver? For the libxl driver, privileged is always true.

...

+ + if (!(network = + virNetworkLookupByName(conn, l_nic->data.network.name))) { + virObjectUnref(conn); + return -1; + } + + if ((brname = virNetworkGetBridgeName(network))) { + if (VIR_STRDUP(x_nic->bridge, brname) < 0) + fail = true; + } else { + fail = true; + } + + VIR_FREE(brname); + + /* Preserve any previous failure */ + errobj = virSaveLastError(); + virNetworkFree(network); + virSetError(errobj); + virFreeError(errobj); + virObjectUnref(conn); + if (fail) + return -1; + break; + } + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight does not support network device type %s"), virDomainNetTypeToString(l_nic->type));

ACK, pending the answers to the above questions.

Thanks for asking the questions, which made me rethink this and feel confident it is okay :-). I'll push this, along with 2/2. Regards, Jim

On 06/11/2014 12:25 AM, Jim Fehlig wrote:

Generally, <interface> ... <script> is only supported for type='ethernet'. Due to the long and pervasive use of

<interface type='bridge'> ... <script path='foo'/> </interface>

in Xen domain configuration, it was agreed to allow the use of <script> with type='bridge' for backwards compatibility. See the following discussion thread

http://www.redhat.com/archives/libvir-list/2013-April/msg00755.html

This patch limits the use of <script> to interface types ethernet and bridge, raising an unsupported config error if <script> is specified for all other interface types.

While at it, use VIR_ERR_CONFIG_UNSUPPORTED instead of VIR_ERR_INTERNAL_ERROR when reporting unsupported interface types.

Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 9c453d8..6a025f0 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -881,6 +881,14 @@ libxlMakeNic(virDomainDefPtr def, * x_nics[i].mtu = 1492; */

+ if (l_nic->script && !(actual_type == VIR_DOMAIN_NET_TYPE_BRIDGE || + actual_type == VIR_DOMAIN_NET_TYPE_ETHERNET)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("specifying a script is only supported with " + "interface types bridge and ethernet")); + return -1; + } + libxl_device_nic_init(x_nic);

virMacAddrGetRaw(&l_nic->mac, x_nic->mac); @@ -954,8 +962,8 @@ libxlMakeNic(virDomainDefPtr def, case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("libxenlight does not support network device type %s"), + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported interface type %s"), virDomainNetTypeToString(l_nic->type)); return -1; }

ACK.