On Fri, Sep 14, 2018 at 05:21:17PM -0600, Jim Fehlig wrote:
> On 8/5/18 3:48 PM, Marek Marczykowski-Górecki wrote:
> > Handle PVH domain type in both directions (xen-xl->xml, xml->xen-xl).
> > And add a test for it.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > Does domain_conf.c (virDomainDefFormatInternal) still need to silently
> > convert VIR_DOMAIN_OSTYPE_XEN to VIR_DOMAIN_OSTYPE_LINUX? In case of
> > PVH, VIR_DOMAIN_OSTYPE_LINUX was never reported as a valid option...
>
> I'd prefer that we switch to formatting type as VIR_DOMAIN_OSTYPE_XEN
See discussion on patch 1/10...
> and
> indicating PV vs PVH with the machine attribute.
It's already here.
> For back compat we'd need
> to continue accepting <type>linux</type> when parsing XML. Other than a lot
> of changes to test suite data files, am I overlooking compatibility problems
> with such a change?
Still accepting <type>linux</type> obviously must stay. But if we change
what is reported when formatting XML, it may cause:
- unexpected configuration change, confusing to the user (no manual
change, but XML is different)
- that XML may not load on very old libvirt versions (I can't find
exactly which one, but older than 0.7.2, which was almost 10 years
ago)
Personally I don't think either of this is a problem.
>
> > ---
> > src/xenconfig/xen_common.c | 17 ++++++++++++-----
> > src/xenconfig/xen_xl.c | 5 +++++
> > tests/testutilsxen.c | 3 ++-
> > tests/xlconfigdata/test-pvh-type.cfg | 13 +++++++++++++
> > tests/xlconfigdata/test-pvh-type.xml | 25 +++++++++++++++++++++++++
> > tests/xlconfigtest.c | 1 +
> > 6 files changed, 58 insertions(+), 6 deletions(-)
> > create mode 100644 tests/xlconfigdata/test-pvh-type.cfg
> > create mode 100644 tests/xlconfigdata/test-pvh-type.xml
> >
> > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> > index c5d81d1..3c120a0 100644
> > --- a/src/xenconfig/xen_common.c
> > +++ b/src/xenconfig/xen_common.c
> > @@ -1081,6 +1081,7 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> > virCapsDomainDataPtr capsdata = NULL;
> > const char *str;
> > int hvm = 0, ret = -1;
> > + const char *machine = "xenpv";
> > if (xenConfigCopyString(conf, "name", &def->name) < 0)
> > goto out;
> > @@ -1089,25 +1090,31 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> > goto out;
> > if (xenConfigGetString(conf, "type", &str, NULL) == 0 && str) {
> > - if (STREQ(str, "pv"))
> > + if (STREQ(str, "pv")) {
> > hvm = 0;
> > - else if (STREQ(str, "hvm"))
> > + } else if (STREQ(str, "pvh")) {
> > + hvm = 0;
> > + machine = "xenpvh";
> > + } else if (STREQ(str, "hvm")) {
> > hvm = 1;
> > - else {
> > + machine = "xenfv";
> > + } else {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("type %s is not supported"), str);
> > return -1;
> > }
>
> Ah, I see you fixed up the braces in this patch. Regardless, 'make
> syntax-check' should pass with each patch.
>
> Reviewed-by: Jim Fehlig <jfehlig@xxxxxxxx>
>
> Regards,
> Jim
>
> > } else {
> > if ((xenConfigGetString(conf, "builder", &str, "linux") == 0) &&
> > - STREQ(str, "hvm"))
> > + STREQ(str, "hvm")) {
> > hvm = 1;
> > + machine = "xenfv";
> > + }
> > }
> > def->os.type = (hvm ? VIR_DOMAIN_OSTYPE_HVM : VIR_DOMAIN_OSTYPE_XEN);
> > if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
> > - VIR_ARCH_NONE, def->virtType, NULL, NULL)))
> > + VIR_ARCH_NONE, def->virtType, NULL, machine)))
> > goto out;
> > def->os.arch = capsdata->arch;
> > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> > index 19b6604..f1853bd 100644
> > --- a/src/xenconfig/xen_xl.c
> > +++ b/src/xenconfig/xen_xl.c
> > @@ -1285,6 +1285,11 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)
> > /* XXX floppy disks */
> > } else {
> > + if (STREQ(def->os.machine, "xenpvh")) {
> > + if (xenConfigSetString(conf, "type", "pvh") < 0)
> > + return -1;
> > + }
> > +
> > if (def->os.bootloader &&
> > xenConfigSetString(conf, "bootloader", def->os.bootloader) < 0)
> > return -1;
> > diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c
> > index c08ec4a..04f8920 100644
> > --- a/tests/testutilsxen.c
> > +++ b/tests/testutilsxen.c
> > @@ -18,7 +18,8 @@ testXLInitCaps(void)
> > "xenfv"
> > };
> > static const char *const xen_machines[] = {
> > - "xenpv"
> > + "xenpv",
> > + "xenpvh"
> > };
> > if ((caps = virCapabilitiesNew(virArchFromHost(),
> > diff --git a/tests/xlconfigdata/test-pvh-type.cfg b/tests/xlconfigdata/test-pvh-type.cfg
> > new file mode 100644
> > index 0000000..2493535
> > --- /dev/null
> > +++ b/tests/xlconfigdata/test-pvh-type.cfg
> > @@ -0,0 +1,13 @@
> > +name = "XenGuest2"
> > +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
> > +maxmem = 579
> > +memory = 394
> > +vcpus = 1
> > +localtime = 0
> > +on_poweroff = "destroy"
> > +on_reboot = "restart"
> > +on_crash = "restart"
> > +type = "pvh"
> > +kernel = "/tmp/vmlinuz"
> > +ramdisk = "/tmp/initrd"
> > +cmdline = "root=/dev/xvda1 console=hvc0"
> > diff --git a/tests/xlconfigdata/test-pvh-type.xml b/tests/xlconfigdata/test-pvh-type.xml
> > new file mode 100644
> > index 0000000..96b0e7a
> > --- /dev/null
> > +++ b/tests/xlconfigdata/test-pvh-type.xml
> > @@ -0,0 +1,25 @@
> > +<domain type='xen'>
> > + <name>XenGuest2</name>
> > + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid>
> > + <memory unit='KiB'>592896</memory>
> > + <currentMemory unit='KiB'>403456</currentMemory>
> > + <vcpu placement='static'>1</vcpu>
> > + <os>
> > + <type arch='x86_64' machine='xenpvh'>linux</type>
> > + <kernel>/tmp/vmlinuz</kernel>
> > + <initrd>/tmp/initrd</initrd>
> > + <cmdline>root=/dev/xvda1 console=hvc0</cmdline>
> > + </os>
> > + <clock offset='utc' adjustment='reset'/>
> > + <on_poweroff>destroy</on_poweroff>
> > + <on_reboot>restart</on_reboot>
> > + <on_crash>restart</on_crash>
> > + <devices>
> > + <console type='pty'>
> > + <target type='xen' port='0'/>
> > + </console>
> > + <input type='mouse' bus='xen'/>
> > + <input type='keyboard' bus='xen'/>
> > + <memballoon model='xen'/>
> > + </devices>
> > +</domain>
> > diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c
> > index ad6a964..b9cf939 100644
> > --- a/tests/xlconfigtest.c
> > +++ b/tests/xlconfigtest.c
> > @@ -283,6 +283,7 @@ mymain(void)
> > DO_TEST("rbd-multihost-noauth");
> > DO_TEST_FORMAT("paravirt-type", false);
> > DO_TEST_FORMAT("fullvirt-type", false);
> > + DO_TEST("pvh-type");
> > #ifdef LIBXL_HAVE_DEVICE_CHANNEL
> > DO_TEST("channel-pty");
> >
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
