[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device



Eric,


> -----Original Message-----
> From: libvir-list-bounces redhat com [mailto:libvir-list-bounces redhat com] On Behalf Of Eric Blake
> Sent: Tuesday, May 28, 2013 7:08 PM
> Cc: libvir-list redhat com; dle-develop lists sourceforge net; Tomoki Sekiyama
> Subject: Re: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device
> 
> On 05/24/2013 04:46 PM, Eric Blake wrote:
> > From: Seiji Aguchi <seiji aguchi hds com>
> >
> > [Problem]
> > Currently, guest OS's messages can be logged to a local disk of host OS
> > by creating chadevs with options below.
> >   -chardev file,id=charserial0,path=<log file's path> -device isa-serial,chardev=chardevserial0,id=serial0
> 
> >
> > Seiji's patch plus my initial review comments; I've ack'd this much,
> > but want to add a round-trip xml2xml test before pushing anything
> > (Seiji, if you want to use this as a starting point, rather than
> > waiting for my long weekend, feel free to post a v3).
> 
> 
> Sorry, but this needs a v3, and I'm going to hand it back to you to fix.
>  I tried adding a testsuite, but it proved that you weren't outputting
> the startupPolicy during dumpxml formatting.  My initial attempt to add
> it backfired (I'll post it below); 

Thank you for handling/testing my patch.

>I'm worried that we are playing too
> fast and loose with a union type, since even with my patch in place,
> 'make check' fails with problems like:
> 
> 84) QEMU XML-2-XML serial-dev                                         ...
> Offset 913
> Expect [/>
>       <target port='0'/>
>     </serial>
>     <console type='dev'>
>       <source path='/dev/ttyS2]
> Actual [ startupPolicy='(null)'/>
>       <target port='0'/>
>     </serial>
>     <console type='dev'>
>       <source path='/dev/ttyS2' startupPolicy='(null)]
> 
> which is a bug, since you said startupPolicy should only be tied to
> files and not other <serial> types.  Do we need to repeat the
> startupPolicy on the <console> mirror of the first <serial> device, or
> is that unnecessary back-compat, and where we can safely declare that
> startupPolicy is only emitted for the <serial> side of things?
> 

I need to think carefully about it before deciding it in a hurry.

> All told, your patch is not complete until it passes 'make check' with a
> new xml2xml test that proves we can round trip the new policy.

Anyway, I will post the v3 patch by fixing the issue.

Seiji

> 
> diff --git c/src/conf/domain_conf.c w/src/conf/domain_conf.c
> index c24a9f0..5af5e40 100644
> --- c/src/conf/domain_conf.c
> +++ w/src/conf/domain_conf.c
> @@ -14547,8 +14547,14 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>          if (def->type != VIR_DOMAIN_CHR_TYPE_PTY ||
>              (def->data.file.path &&
>               !(flags & VIR_DOMAIN_XML_INACTIVE))) {
> -            virBufferEscapeString(buf, "      <source path='%s'/>\n",
> +            virBufferEscapeString(buf, "      <source path='%s'",
>                                    def->data.file.path);
> +
> +            if (def->data.file.path && def->data.file.startupPolicy) {
> +                const char *policy =
> virDomainStartupPolicyTypeToString(def->data.file.startupPolicy);
> +                virBufferAsprintf(buf, " startupPolicy='%s'", policy);
> +            }
> +            virBufferAddLit(buf, "/>\n");
>          }
>          break;
> 
> diff --git
> c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
> w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
> new file mode 100644
> index 0000000..24977f2
> --- /dev/null
> +++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
> @@ -0,0 +1,8 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu
> -S -M \
> +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=readline \
> +-no-acpi -boot c -usb -hdc /tmp/idedisk.img \
> +-chardev file,id=charserial0,path=/tmp/serial.log \
> +-device isa-serial,chardev=charserial0,id=serial0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> diff --git
> c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
> w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
> new file mode 100644
> index 0000000..34e0eb9
> --- /dev/null
> +++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
> @@ -0,0 +1,35 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <disk type='file' device='disk'>
> +      <source file='/tmp/idedisk.img'/>
> +      <target dev='hdc' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='2'/>
> +    </disk>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <serial type='file'>
> +      <source path='/tmp/serial.log' startupPolicy='optional'/>
> +      <target port='0'/>
> +    </serial>
> +    <console type='file'>
> +      <source path='/tmp/serial.log'/>
> +      <target type='serial' port='0'/>
> +    </console>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git c/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c
> index 2c7fd01..22f4782 100644
> --- c/tests/qemuxml2argvtest.c
> +++ w/tests/qemuxml2argvtest.c
> @@ -727,6 +727,8 @@ mymain(void)
>              QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
>      DO_TEST("console-compat-chardev",
>              QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
> +    DO_TEST("serial-source-optional",
> +            QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
> 
>      DO_TEST("channel-guestfwd",
>              QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
> diff --git c/tests/qemuxml2xmltest.c w/tests/qemuxml2xmltest.c
> index 64a271c..1147519 100644
> --- c/tests/qemuxml2xmltest.c
> +++ w/tests/qemuxml2xmltest.c
> @@ -86,6 +86,8 @@ testCompareXMLToXMLHelper(const void *data)
>          ret = testCompareXMLToXMLFiles(xml_in,
>                                         info->different ? xml_out : xml_in,
>                                         false);
> +        if (ret < 0)
> +            goto cleanup;
>      }
>      if (info->when & WHEN_ACTIVE) {
>          ret = testCompareXMLToXMLFiles(xml_in,
> @@ -231,6 +233,7 @@ mymain(void)
>      DO_TEST("console-virtio-many");
>      DO_TEST("channel-guestfwd");
>      DO_TEST("channel-virtio");
> +    DO_TEST("serial-source-optional");
> 
>      DO_TEST("hostdev-usb-address");
>      DO_TEST("hostdev-pci-address");
> 
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]