Re: [PATCH] libxc: zero-initialize structures in macros

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Fri, Sep 2, 2016 at 11:18 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 02/09/16 17:39, Tamas K Lengyel wrote:
>> While debugging applications built on top of libxc with Valgrind we get a lot
>> of complaining about relying on uninitialized values allocated in libxc.
>> While these warnings are safe to ignore, zero-initializing the structures
>> reduces Valgrind clutter a lot and aids in spotting real bugs.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxc/xc_private.h | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
>> index 75b761c..4e9073b 100644
>> --- a/tools/libxc/xc_private.h
>> +++ b/tools/libxc/xc_private.h
>> @@ -59,11 +59,11 @@ struct iovec {
>>  #include <sys/uio.h>
>>  #endif
>>
>> -#define DECLARE_DOMCTL struct xen_domctl domctl
>> -#define DECLARE_SYSCTL struct xen_sysctl sysctl
>> -#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op
>> -#define DECLARE_FLASK_OP struct xen_flask_op op
>> -#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op
>> +#define DECLARE_DOMCTL struct xen_domctl domctl = {0}
>> +#define DECLARE_SYSCTL struct xen_sysctl sysctl = {0}
>> +#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op = {0}
>> +#define DECLARE_FLASK_OP struct xen_flask_op op = {0}
>> +#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op = {0}
>
> I specifically took those out in the past, because it hides real
> problems from Valgrind.
>
> Instead, I would recommend removing these wrappers entirely.  They serve
> no useful purpose.
>
> Taking a random example of xc_get_pfn_type_batch(), it would be rather
> more efficient to write
>
> ...
>     DECLARE_HYPERCALL_BOUNCE(arr, sizeof(*arr) * num,
> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>     struct xen_domctl domctl = {
>         .cmd = XEN_DOMCTL_getpageframeinfo3,
>         .domain = dom,
>         .u.getpageframeinfo3.num = num,
>     };
> ...
>
> as it permits the compiler more freedom in how xen_domctl gets
> constructed, as well as being able to plainly see exactly what is done
> to the memory.
>

Yea I don't really see much point using these macros as they are
either and the one you propose certainly would make more sense.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel