On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé <berrange@xxxxxxxxxx> 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@xxxxxxxxx> > > > --- > > > 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 :| >
