* [PATCH] libxc: zero-initialize structures in macros @ 2016-09-02 16:39 Tamas K Lengyel 2016-09-02 17:18 ` Andrew Cooper 0 siblings, 1 reply; 4+ messages in thread From: Tamas K Lengyel @ 2016-09-02 16:39 UTC (permalink / raw) To: xen-devel; +Cc: Tamas K Lengyel, Ian Jackson, Wei Liu 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} #undef PAGE_SHIFT #undef PAGE_SIZE -- 2.9.3 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libxc: zero-initialize structures in macros 2016-09-02 16:39 [PATCH] libxc: zero-initialize structures in macros Tamas K Lengyel @ 2016-09-02 17:18 ` Andrew Cooper 2016-09-02 17:22 ` Tamas K Lengyel 0 siblings, 1 reply; 4+ messages in thread From: Andrew Cooper @ 2016-09-02 17:18 UTC (permalink / raw) To: Tamas K Lengyel, xen-devel; +Cc: Wei Liu, Ian Jackson 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. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxc: zero-initialize structures in macros 2016-09-02 17:18 ` Andrew Cooper @ 2016-09-02 17:22 ` Tamas K Lengyel 2016-09-05 9:45 ` Wei Liu 0 siblings, 1 reply; 4+ messages in thread From: Tamas K Lengyel @ 2016-09-02 17:22 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel@lists.xenproject.org, Ian Jackson, Wei Liu 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxc: zero-initialize structures in macros 2016-09-02 17:22 ` Tamas K Lengyel @ 2016-09-05 9:45 ` Wei Liu 0 siblings, 0 replies; 4+ messages in thread From: Wei Liu @ 2016-09-05 9:45 UTC (permalink / raw) To: Tamas K Lengyel Cc: Andrew Cooper, Ian Jackson, Wei Liu, xen-devel@lists.xenproject.org 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 > <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. > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-05 9:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-02 16:39 [PATCH] libxc: zero-initialize structures in macros Tamas K Lengyel 2016-09-02 17:18 ` Andrew Cooper 2016-09-02 17:22 ` Tamas K Lengyel 2016-09-05 9:45 ` Wei Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).