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 <nmbrignone(a)gmail.com&gt; > > --- > > 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 :|