Re: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
On Mon, Feb 25, 2019 at 15:18:53 +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
> VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the
> resources for the given VIR_AUTOPTR variable. Given that the cleanup
> function is executed when the stack frame is being destroyed it does not
> make much sense to set the pointer to NULL.
Although correct, the current code is correct too and I really don't see any
benefit behind removing a single line where you reset a variable to NULL, if
anything, this adds more safety regardless of scenario (defensive programming
in general).
Well I'm not against defensive tactics in programming but resetting a
a pointer value to NULL when the pointer is leaving scope is almost as
useful as using a hard hat to prevent drowning in a pool.
There is no way to use the variable which would not be a completely
different class of bug. Also in no way does this keep the stack "tidy".
All stacked variables must still be considered uninitialized even if
we'd make sure that everything resets it's stack copies.
The only thing that comes into my mind where this would "save" us is if
you assign a pointer of a pointer defined by VIR_AUTOPTR to a variable
which also has a __attribute(cleanup... type handler which would
dereference it. That is also wrong on the basis that we should never
rely on the order those cleanup functions are executed even if the
standard does specify it.
Please don't mix this up with what VIR_FREE is doing. Resetting pointers
in VIR_FREE prevents bugs as the pointer is still a valid variable.
Originally, I wanted to argue, that this might introduce an issue in
loops if
you pass the temporary variable to a function by reference and then exiting on
failure would free some garbage, but that's irrelevant since Sukrit introduced
a syntax check to ensure, VIR_AUTO variables are always initialized.
>
> Making the inline function contain less code also decreases the
> possibility that for a given type the inlining will be declined by the
> compiler due to increasing code size.
I honestly doubt that re-setting a variable to NULL will have any impact on
this and it's such a trivial operation that the compiler might optimize this
for you in some way.
Well, I unfortunately can't seem to be able to reproduce this claim at
this point. The compiled code does have the extra instruction which
clears the variable.
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)