Ack, Thanks! On Thu, Jul 9, 2020 at 2:48 PM Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com> 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 <nmbrignone@gmail.com> --- 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 :|