[libvirt] [libvirt PATCH v2] Introduce virDomainChrDefNew()
Make: passed Make check: passed Make syntax-check: passed Hi, this is the commit to introduce the function to create new character device definition for the domain as advised by Cole Robinson <crobinso@redhat.com>. The function is used on the relevant places and also new tests has been added. Michal Signed-off-by: Michal Novotny <minovotn@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++-- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 3 +- src/xenxs/xen_sxpr.c | 5 ++- src/xenxs/xen_xm.c | 6 +++- .../qemuxml2argv-multiple-serials.xml | 31 ++++++++++++++++ .../qemuxml2xmlout-multiple-serials.xml | 37 ++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 90a1317..a80719c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3212,6 +3212,22 @@ error: goto cleanup; } +/* Create a new character device definition and set + * default port. + */ +virDomainChrDefPtr +virDomainChrDefNew(void) { + virDomainChrDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + + def->target.port = -1; + return def; +} + /* Parse the XML definition for a character device * @param node XML nodeset to parse for net definition * @@ -3260,10 +3276,8 @@ virDomainChrDefParseXML(virCapsPtr caps, virDomainChrDefPtr def; int remaining; - if (VIR_ALLOC(def) < 0) { - virReportOOMError(); + if (!(def = virDomainChrDefNew())) return NULL; - } type = virXMLPropString(node, "type"); if (type == NULL) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 95bd11e..ecf44ca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1229,6 +1229,8 @@ void virDomainObjRef(virDomainObjPtr vm); /* Returns 1 if the object was freed, 0 if more refs exist */ int virDomainObjUnref(virDomainObjPtr vm) ATTRIBUTE_RETURN_CHECK; +virDomainChrDefPtr virDomainChrDefNew(void); + /* live == true means def describes an active domain (being migrated or * restored) as opposed to a new persistent configuration of the domain */ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 54e4482..4175ca0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -201,6 +201,7 @@ virDomainChrConsoleTargetTypeFromString; virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; virDomainChrDefFree; +virDomainChrDefNew; virDomainChrSourceDefFree; virDomainChrSpicevmcTypeFromString; virDomainChrSpicevmcTypeToString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fea0068..d258712 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -36,6 +36,7 @@ #include "c-ctype.h" #include "domain_nwfilter.h" #include "qemu_audit.h" +#include "domain_conf.h" #include <sys/utsname.h> #include <sys/stat.h> @@ -5315,7 +5316,7 @@ qemuParseCommandLineChr(const char *val) { virDomainChrDefPtr def; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainChrDefNew())) goto no_memory; if (STREQ(val, "null")) { diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 3a412a6..d1a0b0e 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -168,7 +168,7 @@ xenParseSxprChar(const char *value, char *tmp; virDomainChrDefPtr def; - if (VIR_ALLOC(def) < 0) { + if (!(def = virDomainChrDefNew())) { virReportOOMError(); return NULL; } @@ -1328,6 +1328,7 @@ xenParseSxpr(const struct sexpr *root, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + chr->target.port = 0; def->serials[def->nserials++] = chr; } } @@ -1343,6 +1344,7 @@ xenParseSxpr(const struct sexpr *root, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; + chr->target.port = 0; def->parallels[def->nparallels++] = chr; } } else { @@ -1350,6 +1352,7 @@ xenParseSxpr(const struct sexpr *root, if (!(def->console = xenParseSxprChar("pty", tty))) goto error; def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + def->console->target.port = 0; def->console->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } VIR_FREE(tty); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 22ad788..22d84dd 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -36,6 +36,7 @@ #include "xenxs_private.h" #include "xen_xm.h" #include "xen_sxpr.h" +#include "domain_conf.h" /* Convenience method to grab a int from the config file object */ static int xenXMConfigGetBool(virConfPtr conf, @@ -957,6 +958,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; + chr->target.port = 0; def->parallels[0] = chr; def->nparallels++; chr = NULL; @@ -981,7 +983,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, continue; } - if (VIR_ALLOC(chr) < 0) + if (!(chr = virDomainChrDefNew())) goto no_memory; if (!(chr = xenParseSxprChar(port, NULL))) goto cleanup; @@ -1010,6 +1012,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + chr->target.port = 0; def->serials[0] = chr; def->nserials++; } @@ -1018,6 +1021,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (!(def->console = xenParseSxprChar("pty", NULL))) goto cleanup; def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + def->console->target.port= 0; def->console->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml b/tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml new file mode 100644 index 0000000..0f98f51 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>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='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <serial type='pty'/> + <serial type='null'/> + <serial type='stdio'/> + <console type='pty'> + <target port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml new file mode 100644 index 0000000..878418a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>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='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <serial type='pty'> + <target port='0'/> + </serial> + <serial type='null'> + <target port='1'/> + </serial> + <serial type='stdio'> + <target port='2'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b86dbee..dbc3279 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -195,6 +195,7 @@ mymain(int argc, char **argv) DO_TEST_DIFFERENT("console-compat-auto"); DO_TEST_DIFFERENT("disk-scsi-device-auto"); DO_TEST_DIFFERENT("console-virtio"); + DO_TEST_DIFFERENT("multiple-serials"); virCapabilitiesFree(driver.caps); -- 1.7.3.2
On 04/14/2011 10:50 AM, Michal Novotny wrote:
Make: passed Make check: passed Make syntax-check: passed
Hi, this is the commit to introduce the function to create new character device definition for the domain as advised by Cole Robinson <crobinso@redhat.com>.
The function is used on the relevant places and also new tests has been added.
Michal
Thanks for fixing this. Some comments below.
Signed-off-by: Michal Novotny <minovotn@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++-- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 3 +- src/xenxs/xen_sxpr.c | 5 ++- src/xenxs/xen_xm.c | 6 +++- .../qemuxml2argv-multiple-serials.xml | 31 ++++++++++++++++ .../qemuxml2xmlout-multiple-serials.xml | 37 ++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 90a1317..a80719c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3212,6 +3212,22 @@ error: goto cleanup; }
+/* Create a new character device definition and set + * default port. + */ +virDomainChrDefPtr +virDomainChrDefNew(void) { + virDomainChrDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + + def->target.port = -1; + return def; +} + /* Parse the XML definition for a character device * @param node XML nodeset to parse for net definition * @@ -3260,10 +3276,8 @@ virDomainChrDefParseXML(virCapsPtr caps, virDomainChrDefPtr def; int remaining;
- if (VIR_ALLOC(def) < 0) { - virReportOOMError(); + if (!(def = virDomainChrDefNew())) return NULL; - }
type = virXMLPropString(node, "type"); if (type == NULL) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 95bd11e..ecf44ca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1229,6 +1229,8 @@ void virDomainObjRef(virDomainObjPtr vm); /* Returns 1 if the object was freed, 0 if more refs exist */ int virDomainObjUnref(virDomainObjPtr vm) ATTRIBUTE_RETURN_CHECK;
+virDomainChrDefPtr virDomainChrDefNew(void); + /* live == true means def describes an active domain (being migrated or * restored) as opposed to a new persistent configuration of the domain */ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 54e4482..4175ca0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -201,6 +201,7 @@ virDomainChrConsoleTargetTypeFromString; virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; virDomainChrDefFree; +virDomainChrDefNew; virDomainChrSourceDefFree; virDomainChrSpicevmcTypeFromString; virDomainChrSpicevmcTypeToString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fea0068..d258712 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -36,6 +36,7 @@ #include "c-ctype.h" #include "domain_nwfilter.h" #include "qemu_audit.h" +#include "domain_conf.h"
#include <sys/utsname.h> #include <sys/stat.h> @@ -5315,7 +5316,7 @@ qemuParseCommandLineChr(const char *val) { virDomainChrDefPtr def;
- if (VIR_ALLOC(def) < 0) + if (!(def = virDomainChrDefNew())) goto no_memory;
Here you want to goto error; While it doesn't make any difference at the moment, if ChrDefNew grew any more error conditions, we would end up overwriting it here with an OOM error.
if (STREQ(val, "null")) { diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 3a412a6..d1a0b0e 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -168,7 +168,7 @@ xenParseSxprChar(const char *value, char *tmp; virDomainChrDefPtr def;
- if (VIR_ALLOC(def) < 0) { + if (!(def = virDomainChrDefNew())) { virReportOOMError(); return NULL; }
Drop the OOMError here, ChrDefNew handles it.
@@ -1328,6 +1328,7 @@ xenParseSxpr(const struct sexpr *root, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + chr->target.port = 0; def->serials[def->nserials++] = chr; } } @@ -1343,6 +1344,7 @@ xenParseSxpr(const struct sexpr *root, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; + chr->target.port = 0; def->parallels[def->nparallels++] = chr; } } else { @@ -1350,6 +1352,7 @@ xenParseSxpr(const struct sexpr *root, if (!(def->console = xenParseSxprChar("pty", tty))) goto error; def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + def->console->target.port = 0; def->console->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } VIR_FREE(tty); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 22ad788..22d84dd 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -36,6 +36,7 @@ #include "xenxs_private.h" #include "xen_xm.h" #include "xen_sxpr.h" +#include "domain_conf.h"
/* Convenience method to grab a int from the config file object */ static int xenXMConfigGetBool(virConfPtr conf, @@ -957,6 +958,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; + chr->target.port = 0; def->parallels[0] = chr; def->nparallels++; chr = NULL; @@ -981,7 +983,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, continue; }
- if (VIR_ALLOC(chr) < 0) + if (!(chr = virDomainChrDefNew())) goto no_memory;
This should be goto cleanup;
if (!(chr = xenParseSxprChar(port, NULL))) goto cleanup; @@ -1010,6 +1012,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto no_memory; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + chr->target.port = 0; def->serials[0] = chr; def->nserials++; } @@ -1018,6 +1021,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (!(def->console = xenParseSxprChar("pty", NULL))) goto cleanup; def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + def->console->target.port= 0;
Whitespace is weird here
def->console->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; }
Hmm, why exactly are these port = 0 changes needed? Were they breaking some tests?
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml b/tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml new file mode 100644 index 0000000..0f98f51 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-multiple-serials.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>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='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <serial type='pty'/> + <serial type='null'/> + <serial type='stdio'/> + <console type='pty'> + <target port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml new file mode 100644 index 0000000..878418a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-multiple-serials.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>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='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <serial type='pty'> + <target port='0'/> + </serial> + <serial type='null'> + <target port='1'/> + </serial> + <serial type='stdio'> + <target port='2'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b86dbee..dbc3279 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -195,6 +195,7 @@ mymain(int argc, char **argv) DO_TEST_DIFFERENT("console-compat-auto"); DO_TEST_DIFFERENT("disk-scsi-device-auto"); DO_TEST_DIFFERENT("console-virtio"); + DO_TEST_DIFFERENT("multiple-serials");
virCapabilitiesFree(driver.caps);
I think these test files would be better named serial-target-port-auto. Thanks, Cole
[snip] I've fixed those OOMErrors and the whitespace already for v3 however I won't send the v3 of the patch until I get reply to this since some other approach may have to be chosen for the port = 0 things. comments inline ...
Hmm, why exactly are these port = 0 changes needed? Were they breaking some tests?
Exactly. It was breaking almost all of the xmconfigtests and sexpr2xmltests. The reason was that it was never set in the code to setup just one serial/parallel port and this is why it was breaking it. That's the reason for those changes to port = 0. [snip]
I think these test files would be better named serial-target-port-auto.
Good, I'll do that name change for v3. Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 04/14/2011 11:42 AM, Michal Novotny wrote:
[snip]
I've fixed those OOMErrors and the whitespace already for v3 however I won't send the v3 of the patch until I get reply to this since some other approach may have to be chosen for the port = 0 things. comments inline ...
Hmm, why exactly are these port = 0 changes needed? Were they breaking some tests?
Exactly. It was breaking almost all of the xmconfigtests and sexpr2xmltests. The reason was that it was never set in the code to setup just one serial/parallel port and this is why it was breaking it. That's the reason for those changes to port = 0.
Yeah, at first I didn't think they were necessary, but I see now that they are. Reason being that we only do the port allocation when parsing XML, not when formatting any random DomainDefPtr like I was assuming. So it's better to have all the callers manually set a target port if building a ChrDefPtr by hand. Sorry for the noise. Thanks, Cole
[snip]
I think these test files would be better named serial-target-port-auto.
Good, I'll do that name change for v3.
Thanks, Michal
participants (2)
-
Cole Robinson -
Michal Novotny