Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
Ack, Thanks!
On Thu, Jul 9, 2020 at 2:48 PM Daniel P. Berrangé wrote:
>
> On Thu, Jul 09, 2020 at 02:40:34PM -0300, Nicolás Brignone wrote:
> > On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé
> > wrote:
> > >
> > > On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> > > > On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > > > > All pointers to virXMLPropString() use g_autofree.
> > > >
> > > >
> > > > I changed the summary line like this, to be more precise:
> > > >
> > > >
> > > > conf: use g_autofree for all pointers to virXMLPropString() in
> > > > device_conf.c
> > > >
> > > >
> > > > > All modified functions are similar, in all cases "cleanup" label is
> > > > > removed,
> > > > > along with all the "goto" calls.
> > > >
> > > >
> > > > I've been advised in the recent past to put the g_autofree additions and
> > > > corresponding removals of free functions into one patch, then do the
> > > > removal
> > > > of the extra labels (in favor of directly returning) in a separate
> > > > patch to
> > > > make it easier to hand-verify / review. Here's a couple recent examples:
> > > >
> > > >
> > > > https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> > > >
> > > >
> > > > In your case the changes are few enough that I'm okay with it a single
> > > > patch, except...
> > > >
> > > >
> > > > >
> > > > > Signed-off-by: Nicolas Brignone
> > > > > ---
> > > > > src/conf/device_conf.c | 183
> > > > > +
> > > > > 1 file changed, 56 insertions(+), 127 deletions(-)
> > > > >
> > > > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > > > > index 7d48a3f..9fa6141 100644
> > > > > --- a/src/conf/device_conf.c
> > > > > +++ b/src/conf/device_conf.c
> > > > > @@ -208,45 +208,43 @@ int
> > > > > virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > > > virPCIDeviceAddressPtr addr)
> > > > > {
> > > > > -char *domain, *slot, *bus, *function, *multi;
> > > > > xmlNodePtr cur;
> > > > > xmlNodePtr zpci = NULL;
> > > > > -int ret = -1;
> > > > > memset(addr, 0, sizeof(*addr));
> > > > > -domain = virXMLPropString(node, "domain");
> > > > > -bus = virXMLPropString(node, "bus");
> > > > > -slot = virXMLPropString(node, "slot");
> > > > > -function = virXMLPropString(node, "function");
> > > > > -multi= virXMLPropString(node, "multifunction");
> > > > > +g_autofree char *domain = virXMLPropString(node, "domain");
> > > > > +g_autofree char *bus = virXMLPropString(node, "bus");
> > > > > +g_autofree char *slot = virXMLPropString(node, "slot");
> > > > > +g_autofree char *function = virXMLPropString(node, "function");
> > > > > +g_autofree char *multi= virXMLPropString(node,
> > > > > "multifunction");
> > > >
> > > >
> > > > ... you've modified it so that local variables are being declared
> > > > *below* a
> > > > line of executable code rather than at the top of the block. Although I
> > > > don't see anything in the coding style document
> > > > (https://libvirt.org/coding-style.html) about it, and it may just be
> > > > leftover from a time when we supported a compiler that required all
> > > > declarations to be at top of scope, I think pretty much all of libvirts
> > > > code
> > > > declares all local variables at the top of the scope.
> > >
> > > We should really document it, and even enforce it at compile time.
> >
> > I think you are asking for enabling:
> >
> > -Wdeclaration-after-statement (C and Objective-C only)
> >Warn when a declaration is found after a statement in a
> > block. This construct, known from C++, was introduced
> >with ISO C99 and is by default allowed in GCC. It is not
> > supported by ISO C90.
>
> Yes, but
>
> > Will take a look.
>
> ..don't spend your time on enabling -Wdeclaration-after-statement unless
> you are strongly motivated to do a lot of tedious work. I took at stab at
> it about 12 months ago, but didn't complete it as there are a surprising
> amount of subtle edge cases to deal with, so I never got as far as sending
> a patch proposal.
>
> FWIW, WIP is here
>
>https://gitlab.com/berrange/libvirt/-/tree/c99-vars
>
> but this is based against ancient version, not current
> git master, so I doubt it will rebase cleanly at all
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>
Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
On Thu, Jul 09, 2020 at 02:40:34PM -0300, Nicolás Brignone wrote:
> On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé wrote:
> >
> > On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> > > On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > > > All pointers to virXMLPropString() use g_autofree.
> > >
> > >
> > > I changed the summary line like this, to be more precise:
> > >
> > >
> > > conf: use g_autofree for all pointers to virXMLPropString() in
> > > device_conf.c
> > >
> > >
> > > > All modified functions are similar, in all cases "cleanup" label is
> > > > removed,
> > > > along with all the "goto" calls.
> > >
> > >
> > > I've been advised in the recent past to put the g_autofree additions and
> > > corresponding removals of free functions into one patch, then do the
> > > removal
> > > of the extra labels (in favor of directly returning) in a separate patch
> > > to
> > > make it easier to hand-verify / review. Here's a couple recent examples:
> > >
> > >
> > > https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> > >
> > >
> > > In your case the changes are few enough that I'm okay with it a single
> > > patch, except...
> > >
> > >
> > > >
> > > > Signed-off-by: Nicolas Brignone
> > > > ---
> > > > src/conf/device_conf.c | 183 +
> > > > 1 file changed, 56 insertions(+), 127 deletions(-)
> > > >
> > > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > > > index 7d48a3f..9fa6141 100644
> > > > --- a/src/conf/device_conf.c
> > > > +++ b/src/conf/device_conf.c
> > > > @@ -208,45 +208,43 @@ int
> > > > virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > > virPCIDeviceAddressPtr addr)
> > > > {
> > > > -char *domain, *slot, *bus, *function, *multi;
> > > > xmlNodePtr cur;
> > > > xmlNodePtr zpci = NULL;
> > > > -int ret = -1;
> > > > memset(addr, 0, sizeof(*addr));
> > > > -domain = virXMLPropString(node, "domain");
> > > > -bus = virXMLPropString(node, "bus");
> > > > -slot = virXMLPropString(node, "slot");
> > > > -function = virXMLPropString(node, "function");
> > > > -multi= virXMLPropString(node, "multifunction");
> > > > +g_autofree char *domain = virXMLPropString(node, "domain");
> > > > +g_autofree char *bus = virXMLPropString(node, "bus");
> > > > +g_autofree char *slot = virXMLPropString(node, "slot");
> > > > +g_autofree char *function = virXMLPropString(node, "function");
> > > > +g_autofree char *multi= virXMLPropString(node,
> > > > "multifunction");
> > >
> > >
> > > ... you've modified it so that local variables are being declared *below*
> > > a
> > > line of executable code rather than at the top of the block. Although I
> > > don't see anything in the coding style document
> > > (https://libvirt.org/coding-style.html) about it, and it may just be
> > > leftover from a time when we supported a compiler that required all
> > > declarations to be at top of scope, I think pretty much all of libvirts
> > > code
> > > declares all local variables at the top of the scope.
> >
> > We should really document it, and even enforce it at compile time.
>
> I think you are asking for enabling:
>
> -Wdeclaration-after-statement (C and Objective-C only)
>Warn when a declaration is found after a statement in a
> block. This construct, known from C++, was introduced
>with ISO C99 and is by default allowed in GCC. It is not
> supported by ISO C90.
Yes, but
> Will take a look.
..don't spend your time on enabling -Wdeclaration-after-statement unless
you are strongly motivated to do a lot of tedious work. I took at stab at
it about 12 months ago, but didn't complete it as there are a surprising
amount of subtle edge cases to deal with, so I never got as far as sending
a patch proposal.
FWIW, WIP is here
https://gitlab.com/berrange/libvirt/-/tree/c99-vars
but this is based against ancient version, not current
git master, so I doubt it will rebase cleanly at all
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé wrote:
>
> On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> > On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > > All pointers to virXMLPropString() use g_autofree.
> >
> >
> > I changed the summary line like this, to be more precise:
> >
> >
> > conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
> >
> >
> > > All modified functions are similar, in all cases "cleanup" label is
> > > removed,
> > > along with all the "goto" calls.
> >
> >
> > I've been advised in the recent past to put the g_autofree additions and
> > corresponding removals of free functions into one patch, then do the removal
> > of the extra labels (in favor of directly returning) in a separate patch to
> > make it easier to hand-verify / review. Here's a couple recent examples:
> >
> >
> > https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> >
> >
> > In your case the changes are few enough that I'm okay with it a single
> > patch, except...
> >
> >
> > >
> > > Signed-off-by: Nicolas Brignone
> > > ---
> > > src/conf/device_conf.c | 183 +
> > > 1 file changed, 56 insertions(+), 127 deletions(-)
> > >
> > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > > index 7d48a3f..9fa6141 100644
> > > --- a/src/conf/device_conf.c
> > > +++ b/src/conf/device_conf.c
> > > @@ -208,45 +208,43 @@ int
> > > virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > virPCIDeviceAddressPtr addr)
> > > {
> > > -char *domain, *slot, *bus, *function, *multi;
> > > xmlNodePtr cur;
> > > xmlNodePtr zpci = NULL;
> > > -int ret = -1;
> > > memset(addr, 0, sizeof(*addr));
> > > -domain = virXMLPropString(node, "domain");
> > > -bus = virXMLPropString(node, "bus");
> > > -slot = virXMLPropString(node, "slot");
> > > -function = virXMLPropString(node, "function");
> > > -multi= virXMLPropString(node, "multifunction");
> > > +g_autofree char *domain = virXMLPropString(node, "domain");
> > > +g_autofree char *bus = virXMLPropString(node, "bus");
> > > +g_autofree char *slot = virXMLPropString(node, "slot");
> > > +g_autofree char *function = virXMLPropString(node, "function");
> > > +g_autofree char *multi= virXMLPropString(node, "multifunction");
> >
> >
> > ... you've modified it so that local variables are being declared *below* a
> > line of executable code rather than at the top of the block. Although I
> > don't see anything in the coding style document
> > (https://libvirt.org/coding-style.html) about it, and it may just be
> > leftover from a time when we supported a compiler that required all
> > declarations to be at top of scope, I think pretty much all of libvirts code
> > declares all local variables at the top of the scope.
>
> We should really document it, and even enforce it at compile time.
I think you are asking for enabling:
-Wdeclaration-after-statement (C and Objective-C only)
Warn when a declaration is found after a statement in a
block. This construct, known from C++, was introduced
with ISO C99 and is by default allowed in GCC. It is not
supported by ISO C90.
Will take a look.
> When you have variables in the middle of a function, and do a goto
> jump over them, they are in scope at the goto label target, but
> you may have jumped over their initialization. This makes it hard
> to rationalize about correctness of the code and has led to bugs
> involving uninitialized data before.
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>
Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
On Thu, Jul 9, 2020 at 12:15 AM Laine Stump wrote:
>
> On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > All pointers to virXMLPropString() use g_autofree.
>
>
> I changed the summary line like this, to be more precise:
>
>
> conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
>
>
> > All modified functions are similar, in all cases "cleanup" label is removed,
> > along with all the "goto" calls.
>
>
> I've been advised in the recent past to put the g_autofree additions and
> corresponding removals of free functions into one patch, then do the
> removal of the extra labels (in favor of directly returning) in a
> separate patch to make it easier to hand-verify / review. Here's a
Makes sense, absolutely!
> couple recent examples:
>
>
> https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
>
>
> In your case the changes are few enough that I'm okay with it a single
> patch, except...
>
>
> >
> > Signed-off-by: Nicolas Brignone
> > ---
> > src/conf/device_conf.c | 183 +
> > 1 file changed, 56 insertions(+), 127 deletions(-)
> >
> > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > index 7d48a3f..9fa6141 100644
> > --- a/src/conf/device_conf.c
> > +++ b/src/conf/device_conf.c
> > @@ -208,45 +208,43 @@ int
> > virPCIDeviceAddressParseXML(xmlNodePtr node,
> > virPCIDeviceAddressPtr addr)
> > {
> > -char *domain, *slot, *bus, *function, *multi;
> > xmlNodePtr cur;
> > xmlNodePtr zpci = NULL;
> > -int ret = -1;
> >
> > memset(addr, 0, sizeof(*addr));
> >
> > -domain = virXMLPropString(node, "domain");
> > -bus = virXMLPropString(node, "bus");
> > -slot = virXMLPropString(node, "slot");
> > -function = virXMLPropString(node, "function");
> > -multi= virXMLPropString(node, "multifunction");
> > +g_autofree char *domain = virXMLPropString(node, "domain");
> > +g_autofree char *bus = virXMLPropString(node, "bus");
> > +g_autofree char *slot = virXMLPropString(node, "slot");
> > +g_autofree char *function = virXMLPropString(node, "function");
> > +g_autofree char *multi= virXMLPropString(node, "multifunction");
>
>
> ... you've modified it so that local variables are being declared
> *below* a line of executable code rather than at the top of the block.
> Although I don't see anything in the coding style document
> (https://libvirt.org/coding-style.html) about it, and it may just be
> leftover from a time when we supported a compiler that required all
> declarations to be at top of scope, I think pretty much all of libvirts
> code declares all local variables at the top of the scope.
>
> That's simple enough for me to just fixup before pushing, so
Thanks for that!
>
>
> Reviewed-by: Laine Stump
>
>
> Congratulations on your first libvirt patch! :-)
Thanks to you for reviewing so quickly and for the precise feedback. Regards!
>
> >
> > if (domain &&
> > virStrToLong_uip(domain, NULL, 0, >domain) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("Cannot parse 'domain' attribute"));
> > -goto cleanup;
> > +return -1;
> > }
> >
> > if (bus &&
> > virStrToLong_uip(bus, NULL, 0, >bus) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("Cannot parse 'bus' attribute"));
> > -goto cleanup;
> > +return -1;
> > }
> >
> > if (slot &&
> > virStrToLong_uip(slot, NULL, 0, >slot) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("Cannot parse 'slot' attribute"));
> > -goto cleanup;
> > +return -1;
> > }
> >
> > if (function &&
> > virStrToLong_uip(function, NULL, 0, >function) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("Cannot parse 'function' attribute"));
> > -goto cleanup;
> > +return -1;
> > }
> >
> > if (multi &&
> > @@ -254,11 +252,11 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("Unknown value '%s' for
> > 'multifunction' attribute"),
> > multi);
> > -goto cleanup;
> > +return -1;
> >
> > }
> > if (!virPCIDeviceAddressIsEmpty(addr) &&
> > !virPCIDeviceAddressIsValid(addr, true))
> > -goto cleanup;
> > +return -1;
> >
> > cur = node->children;
> > while (cur) {
> > @@ -270,17 +268,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> > }
> >
> > if (zpci && virZPCIDeviceAddressParseXML(zpci, addr) < 0)
> > -goto cleanup;
> > +return -1;
> >
> > -ret = 0;
> > -
> > - cleanup:
> > -VIR_FREE(domain);
> > -VIR_FREE(bus);
> > -
Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > All pointers to virXMLPropString() use g_autofree.
>
>
> I changed the summary line like this, to be more precise:
>
>
> conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
>
>
> > All modified functions are similar, in all cases "cleanup" label is removed,
> > along with all the "goto" calls.
>
>
> I've been advised in the recent past to put the g_autofree additions and
> corresponding removals of free functions into one patch, then do the removal
> of the extra labels (in favor of directly returning) in a separate patch to
> make it easier to hand-verify / review. Here's a couple recent examples:
>
>
> https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
>
>
> In your case the changes are few enough that I'm okay with it a single
> patch, except...
>
>
> >
> > Signed-off-by: Nicolas Brignone
> > ---
> > src/conf/device_conf.c | 183 +
> > 1 file changed, 56 insertions(+), 127 deletions(-)
> >
> > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > index 7d48a3f..9fa6141 100644
> > --- a/src/conf/device_conf.c
> > +++ b/src/conf/device_conf.c
> > @@ -208,45 +208,43 @@ int
> > virPCIDeviceAddressParseXML(xmlNodePtr node,
> > virPCIDeviceAddressPtr addr)
> > {
> > -char *domain, *slot, *bus, *function, *multi;
> > xmlNodePtr cur;
> > xmlNodePtr zpci = NULL;
> > -int ret = -1;
> > memset(addr, 0, sizeof(*addr));
> > -domain = virXMLPropString(node, "domain");
> > -bus = virXMLPropString(node, "bus");
> > -slot = virXMLPropString(node, "slot");
> > -function = virXMLPropString(node, "function");
> > -multi= virXMLPropString(node, "multifunction");
> > +g_autofree char *domain = virXMLPropString(node, "domain");
> > +g_autofree char *bus = virXMLPropString(node, "bus");
> > +g_autofree char *slot = virXMLPropString(node, "slot");
> > +g_autofree char *function = virXMLPropString(node, "function");
> > +g_autofree char *multi= virXMLPropString(node, "multifunction");
>
>
> ... you've modified it so that local variables are being declared *below* a
> line of executable code rather than at the top of the block. Although I
> don't see anything in the coding style document
> (https://libvirt.org/coding-style.html) about it, and it may just be
> leftover from a time when we supported a compiler that required all
> declarations to be at top of scope, I think pretty much all of libvirts code
> declares all local variables at the top of the scope.
We should really document it, and even enforce it at compile time.
When you have variables in the middle of a function, and do a goto
jump over them, they are in scope at the goto label target, but
you may have jumped over their initialization. This makes it hard
to rationalize about correctness of the code and has led to bugs
involving uninitialized data before.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
On 7/8/20 4:19 PM, Nicolas Brignone wrote:
All pointers to virXMLPropString() use g_autofree.
I changed the summary line like this, to be more precise:
conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
All modified functions are similar, in all cases "cleanup" label is removed,
along with all the "goto" calls.
I've been advised in the recent past to put the g_autofree additions and
corresponding removals of free functions into one patch, then do the
removal of the extra labels (in favor of directly returning) in a
separate patch to make it easier to hand-verify / review. Here's a
couple recent examples:
https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
In your case the changes are few enough that I'm okay with it a single
patch, except...
Signed-off-by: Nicolas Brignone
---
src/conf/device_conf.c | 183 +
1 file changed, 56 insertions(+), 127 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 7d48a3f..9fa6141 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -208,45 +208,43 @@ int
virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
{
-char *domain, *slot, *bus, *function, *multi;
xmlNodePtr cur;
xmlNodePtr zpci = NULL;
-int ret = -1;
memset(addr, 0, sizeof(*addr));
-domain = virXMLPropString(node, "domain");
-bus = virXMLPropString(node, "bus");
-slot = virXMLPropString(node, "slot");
-function = virXMLPropString(node, "function");
-multi= virXMLPropString(node, "multifunction");
+g_autofree char *domain = virXMLPropString(node, "domain");
+g_autofree char *bus = virXMLPropString(node, "bus");
+g_autofree char *slot = virXMLPropString(node, "slot");
+g_autofree char *function = virXMLPropString(node, "function");
+g_autofree char *multi= virXMLPropString(node, "multifunction");
... you've modified it so that local variables are being declared
*below* a line of executable code rather than at the top of the block.
Although I don't see anything in the coding style document
(https://libvirt.org/coding-style.html) about it, and it may just be
leftover from a time when we supported a compiler that required all
declarations to be at top of scope, I think pretty much all of libvirts
code declares all local variables at the top of the scope.
That's simple enough for me to just fixup before pushing, so
Reviewed-by: Laine Stump
Congratulations on your first libvirt patch! :-)
if (domain &&
virStrToLong_uip(domain, NULL, 0, >domain) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot parse 'domain' attribute"));
-goto cleanup;
+return -1;
}
if (bus &&
virStrToLong_uip(bus, NULL, 0, >bus) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot parse 'bus' attribute"));
-goto cleanup;
+return -1;
}
if (slot &&
virStrToLong_uip(slot, NULL, 0, >slot) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot parse 'slot' attribute"));
-goto cleanup;
+return -1;
}
if (function &&
virStrToLong_uip(function, NULL, 0, >function) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot parse 'function' attribute"));
-goto cleanup;
+return -1;
}
if (multi &&
@@ -254,11 +252,11 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown value '%s' for 'multifunction'
attribute"),
multi);
-goto cleanup;
+return -1;
}
if (!virPCIDeviceAddressIsEmpty(addr) &&
!virPCIDeviceAddressIsValid(addr, true))
-goto cleanup;
+return -1;
cur = node->children;
while (cur) {
@@ -270,17 +268,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
}
if (zpci && virZPCIDeviceAddressParseXML(zpci, addr) < 0)
-goto cleanup;
+return -1;
-ret = 0;
-
- cleanup:
-VIR_FREE(domain);
-VIR_FREE(bus);
-VIR_FREE(slot);
-VIR_FREE(function);
-VIR_FREE(multi);
-return ret;
+return 0;
}
void
@@ -309,187 +299,149 @@ int
virDomainDeviceCCWAddressParseXML(xmlNodePtr node,
virDomainDeviceCCWAddressPtr addr)
{
-int ret = -1;
-char *cssid;
-char *ssid;
-char *devno;
-
memset(addr, 0, sizeof(*addr));
-cssid = virXMLPropString(node, "cssid");
-ssid = virXMLPropString(node, "ssid");
-devno = virXMLPropString(node, "devno");
+g_autofree char *cssid = virXMLPropString(node,
