>>> On 5/16/2016 at 06:14 PM, in message <57399D91.1030307@xxxxxxxxxx>, Joao
Martins <joao.m.martins@xxxxxxxxxx> wrote:
>
> On 05/13/2016 05:54 PM, Jim Fehlig wrote:
> > On 05/13/2016 06:59 AM, Joao Martins wrote:
> >>
> >> On 05/12/2016 09:55 PM, Jim Fehlig wrote:
> >>> Joao Martins 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.
> >>> Nod.
> >>>
> >>>> 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.
> >>> Nod again :-).
> >>>
> >>>>> 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.
> >>>> 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?
> >>> 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.
> >>>
> >> Ah, totally missed that out: it looks a large change. I think XL vif won't
> >> diverge from XM anytime soon unless we start adding support for more
> qemu-ish
> >> features on xen libxl (e.g. vhostuser, or even block "target" field
> equivalent).
> >
> > 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.
> >
> >> I am fine with the approach on the patch, but the way you suggested is
> indeed
> >> more correct.
> >
> > 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.
> Yeap 'enum xenConfigFlavor' sounds like a generic enough representation to
> acommodate
> these differences, as opposed to a device specific parameter.
Agree. I'll update the interface.
-Chunyan
>
> Joao
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list