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

[Wei Liu]

On Fri, Sep 02, 2016 at 11:22:04AM -0600, Tamas K Lengyel wrote:
> On Fri, Sep 2, 2016 at 11:18 AM, Andrew Cooper
>  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 
> >> ---
> >> Cc: Ian Jackson 
> >> Cc: Wei Liu 
> >> ---
> >>  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 
> >>  #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.
> 

One reason I can think of why we would want those macros is that we
don't want to change all locations when the code fragment changes. But I
don't see how code segment is going to change for the macros under
discussion.

All in all, I don't object to eliminating those macros. Ian?

Wei.

> Tamas

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

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

[Tamas K Lengyel]

On Fri, Sep 2, 2016 at 11:18 AM, Andrew Cooper
 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 
>> ---
>> Cc: Ian Jackson 
>> Cc: Wei Liu 
>> ---
>>  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 
>>  #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

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

[Andrew Cooper]

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 
> ---
> Cc: Ian Jackson 
> Cc: Wei Liu 
> ---
>  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 
>  #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.

~Andrew

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

[Xen-devel] [PATCH] libxc: zero-initialize structures in macros

[Tamas K Lengyel]

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 
---
Cc: Ian Jackson 
Cc: Wei Liu 
---
 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 
 #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}
 
 #undef PAGE_SHIFT
 #undef PAGE_SIZE
-- 
2.9.3


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