Re: [Xen-devel] [PATCH v4 7/7] tools: enable Cache QoS Monitoring feature for libxl/libxc

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mar, 2013-12-03 at 16:47 +0800, Dongxiao Xu wrote:
> Introduced two new xl commands to attach/detach CQM service for a guest
> $ xl pqos-attach cqm domid
> $ xl pqos-detach cqm domid
> 
> Introduce one new xl command to retrive guest CQM information
> $ xl pqos-list cqm (domid)
> 
> Signed-off-by: Jiongxi Li <jiongxi.li@xxxxxxxxx>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> ---
>  tools/libxc/xc_domain.c   |   47 +++++++++++++++
>  tools/libxc/xenctrl.h     |   11 ++++
>  tools/libxl/Makefile      |    3 +-
>  tools/libxl/libxl.h       |    5 ++
>  tools/libxl/libxl_pqos.c  |  108 +++++++++++++++++++++++++++++++++
>  tools/libxl/xl.h          |    3 +
>  tools/libxl/xl_cmdimpl.c  |  146 
> +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/xl_cmdtable.c |   15 +++++
>  8 files changed, 337 insertions(+), 1 deletion(-)
>  create mode 100644 tools/libxl/libxl_pqos.c
> 
Would it be possible to split this patch in 3, one for libxc, one for
libxl and one for xl?


> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index c7dceda..fdca92d 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -285,6 +285,7 @@
>  
>  #include <libxl_uuid.h>
>  #include <_libxl_list.h>
> +#include <xenctrl.h>
>
Is this really necessary? I think it shouldn't... <xenctrl.h> is already
included in "libxl_internal.h", which you are including yourself below,
so...

> diff --git a/tools/libxl/libxl_pqos.c b/tools/libxl/libxl_pqos.c
> new file mode 100644
> index 0000000..bf7593a
> --- /dev/null
> +++ b/tools/libxl/libxl_pqos.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (C) 2013      Intel Corporation
> + * Author Jiongxi Li <jiongxi.li@xxxxxxxxx>
> + * Author Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"

> +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, const char * qos_type)
> +{
> +    int rc;
> +    uint32_t flags = 0;
> +
> +    if (!strncmp(qos_type, "cqm", 3))
> +        flags |= XEN_DOMCTL_pqos_cqm;
> +    else {
> +        rc = -EINVAL;
> +        LIBXL__LOG(ctx, XTL_ERROR, "%s", msg[EINVAL]);
>
I think new code should use the LOG() / LOGE() variant of the logging
macros.

> +        return rc;
> +    }
>
libxl functions should return libxl error codes.

> +
> +    rc = xc_domain_pqos_attach(ctx->xch, domid, flags);
> +    if (rc < 0) {
> +        switch(errno) {
> +        case EINVAL:
> +        case ENODEV:
> +        case EEXIST:
> +        case EUSERS:
> +        case ESRCH:
> +            LIBXL__LOG(ctx, XTL_ERROR, "%s", msg[errno]);
> +            break;
> +        default:
> +            LIBXL__LOG(ctx, XTL_ERROR, "errno: %d", errno);
> +        }
> +    }
> +
Mmm... Isn't there a better place where to do this demultiplexing and
printing the correct message? Looking around, in libxl, what usually
happens is printing something generic enough, together with the errno
value/message (with LOGE), and then rely on lower layers to provide a
more detailed explanation of what happened.

This is certainly up to a maintainer to decide, but I think you at least
(you're doing this twice) should do it in a function rather than
duplicating the whole switch.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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