According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront. Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF. Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'. Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg. Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; } - static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup; - if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup; @@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) /* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) { if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1; - if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1; if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list, virDomainChrDefPtr serial) return -1; } - static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; } - - static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf, for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; } @@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf, /* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1; - if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1; if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf, int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename); int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - + virConnectPtr conn, + const char *vif_typename); int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def); diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1; - if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup; if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup; - if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup; if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1; - if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup; if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup; if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup; if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup; - if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup; if (xenFormatXMOS(conf, def) < 0) -- 2.1.4

On 04/21/2016 05:10 AM, Chunyan Liu wrote: One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Joao already mentioned the spurious white space changes. My recommendation is to stick with the prevalent pattern in the file. I think these types of unrelated cleanups should be done in a separate patch. It's a better approach in case someone wants to backport the bug you are fixing here. Regards, Jim

Joao Martins wrote: Nod. Nod again :-). On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g. ... vncunused = 1 vnclisten = "127.0.0.1" vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] parallel = "none" serial = "none" builder = "hvm" boot = "d" disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", "/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", "/root/boot.iso,raw,hdc,ro,devtype=cdrom" ] to ... vncunused = 1 vnclisten = "127.0.0.1" parallel = "none" serial = "none" builder = "hvm" boot = "d" vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", "/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", "/root/boot.iso,raw,hdc,ro,devtype=cdrom" ] Regards, Jim

On 05/13/2016 06:59 AM, Joao Martins wrote: That's a good point. Instead of creating a bunch of turmoil now over 'netfront' vs 'vif', we should wait until something more substantial drives the change. Perhaps as a compromise, the new xen{Format,Parse}ConfigCommon parameter could be of type 'enum xenConfigFlavor' or similar, with flavors XEN_CONFIG_FLAVOR_XL and XEN_CONFIG_FLAVOR_XM. That would accommodate other trivial differences we might find in the future. Regards, Jim

Martins <joao.m.martins(a)oracle.com&gt; wrote: Agree. I'll update the interface. -Chunyan

<joao.m.martins(a)oracle.com&gt; wrote: In fact at first I changed codes as Jim suggested, didn't change xenParseConfigCommon(), but extract xen{Format,Parse}Vif out from xenParseConfigCommon() and called from xen_{xl,xm}.c directly. But that will change the order device appears in .cfg or xml. So existing xml and .cfg will fail many times. (I spent quite a bit of time to update xml and .cfg to make all of them correct, still some not pass. That's why finally I updated xenParseConfigCommon()).