[libvirt] [PATCH] Introduce virDomainChrDefNew()
Hi, this is the commit to introduce the function to create new character device definition for the domain as advices by Cole Robinson <crobinso@redhat.com>. The function is used on the relevant places and the make, make check and make syntax-check all passed. Michal Signed-off-by: Michal Novotny <minovotn@redhat.com> Signed-off-by: Michal Novotny <mignov@gmail.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_xm.c | 3 ++- 5 files changed, 24 insertions(+), 5 deletions(-) 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..c13e416 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -333,6 +333,7 @@ virDomainWatchdogActionTypeFromString; virDomainWatchdogActionTypeToString; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; +virDomainChrDefNew; # domain_event.h 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_xm.c b/src/xenxs/xen_xm.c index 22ad788..0b47f1f 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, @@ -981,7 +982,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; -- 1.7.3.2
On 04/13/2011 11:14 AM, Michal Novotny wrote:
Hi, this is the commit to introduce the function to create new character device definition for the domain as advices by Cole Robinson <crobinso@redhat.com>.
The function is used on the relevant places and the make, make check and make syntax-check all passed.
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com> Signed-off-by: Michal Novotny <mignov@gmail.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_xm.c | 3 ++- 5 files changed, 24 insertions(+), 5 deletions(-)
There's at least one other instance that needs changing in xenxs/xen_sxpr.c And I really don't think we should commit this until there is a regression test demonstrating the regression (and that this patch fixes it). I mentioned this earlier: https://www.redhat.com/archives/libvir-list/2011-April/msg00287.html Aside from that the patch looks fine. Thanks, Cole
On 04/13/2011 05:38 PM, Cole Robinson wrote:
On 04/13/2011 11:14 AM, Michal Novotny wrote:
Hi, this is the commit to introduce the function to create new character device definition for the domain as advices by Cole Robinson <crobinso@redhat.com>.
The function is used on the relevant places and the make, make check and make syntax-check all passed.
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com> Signed-off-by: Michal Novotny <mignov@gmail.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_xm.c | 3 ++- 5 files changed, 24 insertions(+), 5 deletions(-)
There's at least one other instance that needs changing in xenxs/xen_sxpr.c
I tried to implement to xen_sxpr.c as well and it broke xen tests entirely (sexpr2xml and xmconfigtest) - almost all of the tests in those were failing so I dropped it.
And I really don't think we should commit this until there is a regression test demonstrating the regression (and that this patch fixes it). I mentioned this earlier:
https://www.redhat.com/archives/libvir-list/2011-April/msg00287.html
Not sure about implementing this is necessary since there are already some tests - and they were failing when xen_sexpr.c was patched. Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 04/13/2011 09:14 AM, Michal Novotny wrote:
Hi, this is the commit to introduce the function to create new character device definition for the domain as advices by Cole Robinson
s/advices/advised/
<crobinso@redhat.com>.
The function is used on the relevant places and the make, make check and make syntax-check all passed.
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com> Signed-off-by: Michal Novotny <mignov@gmail.com>
Technically, a sign-off line is for a person, not an email address, so having two sign-offs looks weird. Given that your redhat address is the one listed in AUTHORS, as well as the one you sent this message with, that should be sufficient. But if you ever do want to use multiple addresses, we maintain a .mailmap file which lets you commit under more than one alias while still mapping back to a single AUTHORS line. Cole's review just came in while I was typing this, so I'll wait for a v2.
+++ b/src/libvirt_private.syms @@ -333,6 +333,7 @@ virDomainWatchdogActionTypeFromString; virDomainWatchdogActionTypeToString; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; +virDomainChrDefNew;
In addition to Cole's finding, please keep this file sorted (chunks by header name, then within a chunk sort by function name). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/13/2011 05:42 PM, Eric Blake wrote:
On 04/13/2011 09:14 AM, Michal Novotny wrote:
Hi, this is the commit to introduce the function to create new character device definition for the domain as advices by Cole Robinson s/advices/advised/
Thanks for noticing this typo.
<crobinso@redhat.com>.
The function is used on the relevant places and the make, make check and make syntax-check all passed.
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com> Signed-off-by: Michal Novotny <mignov@gmail.com> Technically, a sign-off line is for a person, not an email address, so having two sign-offs looks weird. Given that your redhat address is the one listed in AUTHORS, as well as the one you sent this message with, that should be sufficient. But if you ever do want to use multiple addresses, we maintain a .mailmap file which lets you commit under more than one alias while still mapping back to a single AUTHORS line.
The second sign-off by clause has been added automatically by git format-patch and I was having incorrect setup of the git config so it should be fine by now. I don't want to use multiple e-mail addresses for libvirt and I want to use redhat address only so this was a misconfiguration of my git.
Cole's review just came in while I was typing this, so I'll wait for a v2.
+++ b/src/libvirt_private.syms @@ -333,6 +333,7 @@ virDomainWatchdogActionTypeFromString; virDomainWatchdogActionTypeToString; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; +virDomainChrDefNew; In addition to Cole's finding, please keep this file sorted (chunks by header name, then within a chunk sort by function name).
I see what you mean. I will. Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
participants (3)
-
Cole Robinson -
Eric Blake -
Michal Novotny