On 05/13/2016 03:08 AM, Chun Yan Liu wrote:
>
>
>>>> On 5/12/2016 at 11:00 PM, in message <57349A91.50301@xxxxxxxxxx>, Joao Martins
> <joao.m.martins@xxxxxxxxxx> wrote:
>> On 05/12/2016 12:54 AM, Jim Fehlig wrote:
>>> On 04/21/2016 05:10 AM, Chunyan Liu wrote:
>>>> 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@xxxxxxxx>
>>>> ---
>>>> 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)
>>>
>>> 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?
>> That's a good point - probably we can avoid it by using
>> xen{Format,Parse}Vif (with the signature change Chunyan proposes)
>> individually
>> on xenParseXM and xenParseXL. And there wouldn't be any xenParseConfigCommon
>> with device-specific parameters (as vif being one of the many devices that
>> the
>> routine is handling). The vif config is the same between xm and xl, with the
>> small difference wrt to the validation on xen libxl side - so having in
>> xen_common.c makes sense.
>>
>>> 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.
>> Hm, perhaps I fail to see what the large change would be. We would keep the
>> same
>> interface (i.e. model=netfront as valid on libvirt-side and converting to
>> type="vif" where applicable (libxl)) then the xml and .cfg won't change.
>
> 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.
>
Indeed.
> (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()).
>
I am not sure what's the etiquette in these cases but I sent you some patches
that fixes the tests and makes some adjustments to help out (all changelog-ed).
There were a couple of failures lately regarding vram defaults and what not so
some of the tests would fail as vram defaults would be wrongly calculated. Jim
sent a series fixing that which is Acked already but still to be pushed.
Regards,
Joao
>> Furthermore, we only use e1000 which is valid for both cases and Chunyan
>> adds
>> one test case to cover this series. So may be the adjustment you suggest
>> above
>> wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata
>> files?
>>
>>>
>>>> {
>>>> 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;
>>>> }
>>>>
>>>> -
>>>
>>> Joao already mentioned the spurious white space changes. My recommendation
>> is to
>>> stick with the prevalent pattern in the file.
>>>
>>>> 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;
>>>
>>> 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
>>>
>>>>
>>>> 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)
>>>
>>
>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list