On 02/16/2011 08:19 AM, Michal Novotny wrote:
> Ping ... could somebody review this one please?
>
> Thanks,
> Michal
>
> On 01/22/2011 05:49 PM, Michal Novotny wrote:
>> Hi,
>> this is the patch to add support for multiple serial ports to the
>> libvirt Xen driver. It support both old style (serial = "pty") and
>> new style (serial = [ "/dev/ttyS0", "/dev/ttyS1" ]) definition and
>> tests for xml2sexpr, sexpr2xml and xmconfig have been added as well.
Sorry for dropping the ball on reviewing this one. Now I'm afraid that
we're too close to 0.8.8 to include this without risking breaking
something else, so if you don't mind waiting another day or so before we
apply it...
At any rate, I saw that round 1 of your patch had reviews, and didn't
notice that this was a v2 patch. At first glance, you addressed the
comments from round 1, including adding a test case:
>> .../xml2sexpr-fv-serial-dev-2-ports.sexpr | 1 +
>> .../xml2sexpr-fv-serial-dev-2-ports.xml | 44 ++++++
>> tests/xml2sexprtest.c | 1 +
>> 11 files changed, 370 insertions(+), 26 deletions(-)
...so that speaks in favor of this already.
>> +++ b/src/xen/xend_internal.c
>> @@ -1218,6 +1218,9 @@ xenDaemonParseSxprChar(const char *value,
>>
>> if (value[0] == '/') {
>> def->source.type = VIR_DOMAIN_CHR_TYPE_DEV;
>> + def->data.file.path = strdup(value);
>> + if (!def->data.file.path)
>> + goto error;
This should be goto no_memory.
>> } else {
>> if ((tmp = strchr(value, ':')) != NULL) {
>> *tmp = '\0';
>> @@ -2334,6 +2337,8 @@ xenDaemonParseSxpr(virConnectPtr conn,
>> tty = xenStoreDomainGetConsolePath(conn, def->id);
>> xenUnifiedUnlock(priv);
>> if (hvm) {
>> +
>> +
>> tmp = sexpr_node(root, "domain/image/hvm/serial");
Why the spurious whitespace change?
>> @@ -5958,10 +6011,22 @@ xenDaemonFormatSxpr(virConnectPtr conn,
>> virBufferAddLit(&buf, "(parallel none)");
>> }
>> if (def->serials) {
>> - virBufferAddLit(&buf, "(serial ");
>> - if (xenDaemonFormatSxprChr(def->serials[0],&buf)< 0)
>> - goto error;
>> - virBufferAddLit(&buf, ")");
>> + if (def->nserials> 1) {
>> + virBufferAddLit(&buf, "(serial (");
>> + for (i = 0; i< def->nserials; i++) {
>> + if
>> (xenDaemonFormatSxprChr(def->serials[i],&buf)< 0)
>> + goto error;
>> + if (i< def->nserials - 1)
>> + virBufferAddLit(&buf, " ");
>> + }
>> + virBufferAddLit(&buf, "))");
>> + }
You skipped "none" as a placeholder when parsing; but I'm not sure if
this generates "none" on output to match.
>> + }
>> + }
>> + /* If domain is not using multiple serial ports we parse data
>> old way */
>> + else {
Style-wise, I would have written:
} else {
/* If domain is not using multiple... */
rather than injecting the comment between } and else.
>> @@ -2685,17 +2758,41 @@ virConfPtr
>> xenXMDomainConfigFormat(virConnectPtr conn,
>> }
>>
>> if (def->nserials) {
>> - virBuffer buf = VIR_BUFFER_INITIALIZER;
>> - char *str;
>> - int ret;
>> + /* If there's a single serial port definition use the old
>> approach not to break old configs */
>> + if (def->nserials == 1) {
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> + char *str;
>> + int ret;
>> +
>> + ret = xenDaemonFormatSxprChr(def->serials[0],&buf);
>> + str = virBufferContentAndReset(&buf);
>> + if (ret == 0)
>> + ret = xenXMConfigSetString(conf, "serial", str);
>> + VIR_FREE(str);
>> + if (ret< 0)
>> + goto no_memory;
>> + }
>> + else {
Style - } and else should be on same line.
>> index 0000000..e709eb0
>> --- /dev/null
>> +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.sexpr
>> @@ -0,0 +1 @@
>> +(domain (domid 1)(name 'fvtest')(memory 400)(maxmem 400)(vcpus
>> 1)(uuid 'b5d70dd275cdaca517769660b059d8ff')(on_poweroff
>> 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel
>> '/usr/lib/xen/boot/hvmloader')(vcpus 1)(boot c)(cdrom
>> '/root/boot.iso')(acpi 1)(usb 1)(parallel none)(serial (/dev/ttyS0
>> /dev/ttyS1))(device_model '/usr/lib64/xen/bin/qemu-dm')(vnc
>> 1)))(device (vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode
>> 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script
>> 'vif-bridge')(type ioemu))))
Can we add a second test that uses (serial (none /dev/ttyS1)) to prove
that we handle placeholders correctly?
>> b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr
>> new file mode 100644
>> index 0000000..2048159
>> --- /dev/null
>> +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr
>> @@ -0,0 +1 @@
>> +(vm (name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid
>> 'b5d70dd2-75cd-aca5-1776-9660b059d8bc')(on_poweroff
>> 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel
>> '/usr/lib/xen/boot/hvmloader')(vcpus 1)(boot c)(cdrom
>> '/root/boot.iso')(acpi 1)(usb 1)(parallel none)(serial (/dev/ttyS0
>> /dev/ttyS1))(device_model '/usr/lib64/xen/bin/qemu-dm')(vnc
>> 1)))(device (vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode
>> 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script
>> 'vif-bridge')(model 'e1000')(type ioemu))))
>> \ No newline at end of file
Independent of your patch, but now that we have support for
backslash-newlines in the testsuite, we probably ought to convert all of
the .sexpr files to take advantage of it.
Can you post a v3 that addresses the first few concerns (don't worry
about the .sexpr \-\n conversions for now)?
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list