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@xxxxxxxxxx>.
>
> 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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list