[libvirt PATCH] All pointers to virXMLPropString() use g_autofree.

[Nicolás Brignone]

Ack, Thanks!

On Thu, Jul 9, 2020 at 2:48 PM Daniel P. Berrangé <berrange at 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 at 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 at 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 :|
>