Re: [Xen-devel] [PATCH 11/11] libxl: -Wunused-parameter

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> *** DO NOT APPLY ***
> 
> We have recently had a couple of bugs which basically involved
> ignoring the rc parameter to a callback function.  I thought I would
> try -Wunused-parameter.  Here are the results.
> 
> I found three further problems:
> 
>  * libxl_wait_for_free_memory takes a domid parameter but its
>    semantics don't seem to call for that.  This function is
>    going to have a big warning put on it for 4.2 and that
>    should happen soon.
> 
>  * qmp_synchronous_send has an `ask_timeout' parameter which is
>    ignored.
> 
>  * The autogenerated function libxl_event_init_type ignores the type
>    parameter.
> 
> Things I needed to do to get the rest of the code to compile:
> 
>  * Remove one harmless unused parameter from an internal function.
>    (Earlier in this series.)
> 
>  * Add an assert to make the error handling in the broken remus code
>    slightly less broken.  (Earlier in this series.)
> 
>  * Provide machinery in the Makefile for passing different CFLAGS to
>    libxl as opposed to xl and libxlu.  The flex- and bison-generated
>    files in libxlu can't be compiled with -Wunused-parameter.
> 
>  * Define a new helper macro
>         #define USE(var) ((void)(var))
>    and use it 43 times.  The pattern is something like
>         USE(egc);
>    in a function which takes egc but doesn't need it.  If the
>    parameter is later used, this is harmless.  In functions
>    which are placeholders the USE statement should be placed in the
>    middle of the function where the parameter would be used if the
>    function is changed later, so that the USE gets deleted by the
>    patch introducing the implementation.
> 
>  * Define a new helper macro for use only in other macros
>         #define MAYBE_UNUSED __attribute__((unused))
>    and use it in 10 different places.
> 
>  * Define new macros for helping declare common types of callback
>    functions.  For example:
> 
>         #define EV_XSWATCH_CALLBACK_PARAMS(egc, watch, wpath, epath)  \
>             libxl__egc *egc MAYBE_UNUSED,                             \
>             libxl__ev_xswatch *watch MAYBE_UNUSED,                    \
>             const char *wpath MAYBE_UNUSED,                           \
>             const char *epath MAYBE_UNUSED
> 
>    which is used like this:
> 
>         -static void some_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
>         -                          const char *wpath, const char *epath)
>         +static void some_callback(EV_XSWATCH_CALLBACK_PARAMS
>         +                          (egc, watch, wpath, epath))
>         {
>             ... now we use (or not) egc, watch, wpath, etc. or not as we like
> 
>    This somewhat resembles a Traditional K&R C typeless function
>    definition.  The types of the parameters are actually defined
>    for the compiler of course, along with the information that
>    the parameters might be unused.
> 
>    There are 4 macros of this kind with 22 call sites.
> 
> IMO on the cost (65 places in ordinary code where we have to write
> something somewhat ugly) is worth the benefit (finding, if we had
> deployed this right away, around 6 bugs).  But it's arguable.
> 
> *** DO NOT APPLY ***
> 
> Anyway, this patch must not be applied right now because it causes the
> build to fail.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> ---
>  tools/libxl/Makefile             |    4 +++-
>  tools/libxl/libxl.c              |   29 +++++++++++++++++++++++++----
>  tools/libxl/libxl_aoutils.c      |    8 ++++----
>  tools/libxl/libxl_blktap2.c      |    1 +
>  tools/libxl/libxl_bootloader.c   |    6 ++++++
>  tools/libxl/libxl_create.c       |    2 ++
>  tools/libxl/libxl_device.c       |    9 +++++----
>  tools/libxl/libxl_dm.c           |    4 ++++
>  tools/libxl/libxl_dom.c          |    8 ++++----
>  tools/libxl/libxl_event.c        |   22 +++++++++++++---------
>  tools/libxl/libxl_exec.c         |    8 ++++----
>  tools/libxl/libxl_fork.c         |    1 +
>  tools/libxl/libxl_internal.h     |   24 ++++++++++++++++++++++--
>  tools/libxl/libxl_pci.c          |    6 ++++++
>  tools/libxl/libxl_qmp.c          |   17 +++++++++--------
>  tools/libxl/libxl_save_callout.c |    4 ++--
>  tools/libxl/libxl_utils.c        |    2 ++
>  17 files changed, 113 insertions(+), 42 deletions(-)

I'm not entirely sure how I feel about this patch generally (it's quite
a bit of mess, but the bugs it would have found are real). It's also
quite a lot of churn for 4.2.

On the other hand we are likely to want to backport lots of libxl fixes
for 4.2.1 (I was actually considering an exception to the "no new
features" rule for 4.2.1 for xm parity causing patches) and having this
in 4.2 would make that cleaner.

I guess I come down (just) on the side of taking this, when it is baked.

> @@ -700,6 +702,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx, 
> libxl_domain_remus_info *info,
>      libxl__domain_suspend_state *dss;
>      int rc;
> 
> +    USE(recv_fd); /* TODO get rid of this and actually use it! */

You've only just introduced TODO REMUS...

> @@ -1019,6 +1019,7 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void 
> *for_libxl,
> 
>      assert(fd == ev->fd);
>      revents &= ev->events;
> +    USE(events); /* we use our own idea of what we asked for */

What is the point of this argument then?

Is getting an event we weren't expecting a log-worthy occurrence?

> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 9c92ae6..a094965 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -800,6 +800,10 @@ static int pci_ins_check(libxl__gc *gc, uint32_t domid, 
> const char *state, void
>  {
>      char *orig_state = priv;
> 
> +    USE(gc);
> +    USE(domid);
> +    USE(priv);

You actually use priv above.

Ian.


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