Re: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
On Fri, Feb 18, 2011 at 01:22:41PM -0700, Eric Blake wrote: > On 02/18/2011 12:30 PM, Jim Meyering wrote: > >>> -if (doms->objs) > >>> -virHashFree(doms->objs, virDomainObjListDeallocator); > >>> +virHashFree(doms->objs, virDomainObjListDeallocator); > >> > >> I tried adding --name=virHashFree to the useless_free_options variable > >> in cfg.mk, to see if that would prevent regressions. However, it > >> appears that this two-argument free-like function is not picked up by > >> the heuristics in the useless-if-before-free script (it only works on > >> one-argument functions). > > > Right. useless-if-before-free deliberately detects only > > one-argument free-like functions. > > An even better idea - as long as we're improving hash.h, why not fix > things to pass the destructor in the Create call, rather than the Free > call. Then freeing is a one-argument operation, using the destructor > callback registered inside the virHash object, rather than a > two-argument operation requiring the caller to pass in the destructor. > At which point, we've defined away the problem of useless-if-before-free. Yes, that makes alot more sense as an API design Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
On 02/18/2011 12:30 PM, Jim Meyering wrote: >>> -if (doms->objs) >>> -virHashFree(doms->objs, virDomainObjListDeallocator); >>> +virHashFree(doms->objs, virDomainObjListDeallocator); >> >> I tried adding --name=virHashFree to the useless_free_options variable >> in cfg.mk, to see if that would prevent regressions. However, it >> appears that this two-argument free-like function is not picked up by >> the heuristics in the useless-if-before-free script (it only works on >> one-argument functions). > Right. useless-if-before-free deliberately detects only > one-argument free-like functions. An even better idea - as long as we're improving hash.h, why not fix things to pass the destructor in the Create call, rather than the Free call. Then freeing is a one-argument operation, using the destructor callback registered inside the virHash object, rather than a two-argument operation requiring the caller to pass in the destructor. At which point, we've defined away the problem of useless-if-before-free. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
Eric Blake wrote:
> [adding Jim to cc, as author of useless-if-before-free]
>
> On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
>> virHashFree follows the convention described in HACKING that
>> XXXFree() functions can be called with a NULL argument.
>> ---
>> src/conf/domain_conf.c |6 +---
>> src/datatypes.c | 51
>> +++---
>> src/qemu/qemu_process.c |4 +--
>> 3 files changed, 20 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 351daf7..e7c3409 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void
>> *payload, const char *name ATTRIBUT
>>
>> void virDomainObjListDeinit(virDomainObjListPtr doms)
>> {
>> -if (doms->objs)
>> -virHashFree(doms->objs, virDomainObjListDeallocator);
>> +virHashFree(doms->objs, virDomainObjListDeallocator);
>
> I tried adding --name=virHashFree to the useless_free_options variable
> in cfg.mk, to see if that would prevent regressions. However, it
> appears that this two-argument free-like function is not picked up by
> the heuristics in the useless-if-before-free script (it only works on
> one-argument functions).
>
> ACK to your patch as-is, so I pushed it. But I can't help but wonder if
> we could make this easier to enforce at 'make syntax-check' time by
> tweaking the perl script somehow.
Right. useless-if-before-free deliberately detects only
one-argument free-like functions.
I'm reluctant to add such a change to that script because
- there are comparatively few free-like functions with more than
one argument, so there's not much anticipated gain
- we don't know in general whether the free'd pointer arg is the
first or second, so...
- having to match against either the first or 2nd argument would
greatly complicate the already complicated regular expressions
If you're interested in pursuing it, I suggest switching tools
altogether and using spatch. That would make for a much cleaner
(albeit probably less efficient) script. It would also depend
on a less-ubiquitous tool than perl.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
[adding Jim to cc, as author of useless-if-before-free]
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
> virHashFree follows the convention described in HACKING that
> XXXFree() functions can be called with a NULL argument.
> ---
> src/conf/domain_conf.c |6 +---
> src/datatypes.c | 51 +++---
> src/qemu/qemu_process.c |4 +--
> 3 files changed, 20 insertions(+), 41 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 351daf7..e7c3409 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void *payload,
> const char *name ATTRIBUT
>
> void virDomainObjListDeinit(virDomainObjListPtr doms)
> {
> -if (doms->objs)
> -virHashFree(doms->objs, virDomainObjListDeallocator);
> +virHashFree(doms->objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable
in cfg.mk, to see if that would prevent regressions. However, it
appears that this two-argument free-like function is not picked up by
the heuristics in the useless-if-before-free script (it only works on
one-argument functions).
ACK to your patch as-is, so I pushed it. But I can't help but wonder if
we could make this easier to enforce at 'make syntax-check' time by
tweaking the perl script somehow.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
