From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4 7/7] tools: enable Cache QoS Monitoring feature for libxl/libxc Date: Tue, 3 Dec 2013 13:17:01 +0100 Message-ID: <1386073021.5338.320.camel@Solace> References: <1386060479-17205-1-git-send-email-dongxiao.xu@intel.com> <1386060479-17205-8-git-send-email-dongxiao.xu@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4908593856069330774==" Return-path: In-Reply-To: <1386060479-17205-8-git-send-email-dongxiao.xu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dongxiao Xu Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org --===============4908593856069330774== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-rO7/WVY+IRAdwYscTHpx" --=-rO7/WVY+IRAdwYscTHpx Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 >=20 > Introduce one new xl command to retrive guest CQM information > $ xl pqos-list cqm (domid) >=20 > Signed-off-by: Jiongxi Li > Signed-off-by: Dongxiao Xu > --- > 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 >=20 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 @@ > =20 > #include > #include <_libxl_list.h> > +#include > Is this really necessary? I think it shouldn't... 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 > + * Author Dongxiao Xu > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU Lesser General Public License as publis= hed > + * 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_t= ype) > +{ > + int rc; > + uint32_t flags =3D 0; > + > + if (!strncmp(qos_type, "cqm", 3)) > + flags |=3D XEN_DOMCTL_pqos_cqm; > + else { > + rc =3D -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 =3D 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 --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-rO7/WVY+IRAdwYscTHpx Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iEYEABECAAYFAlKdy70ACgkQk4XaBE3IOsQlqwCcD8aHr4GR1V7tEsUmpdFhBlOL hfYAn0txIiYQxFgJV0A6m/Jhyj0JGWBU =fOgK -----END PGP SIGNATURE----- --=-rO7/WVY+IRAdwYscTHpx-- --===============4908593856069330774== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4908593856069330774==--