From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 1/9] xl: Improve return and exit codes of memory related functions. Date: Thu, 25 Feb 2016 12:10:36 +0100 Message-ID: <1456398636.6288.82.camel@citrix.com> References: <1456318407-3635-1-git-send-email-write.harmandeep@gmail.com> <1456318407-3635-2-git-send-email-write.harmandeep@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0402942892488346961==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.84) (envelope-from ) id 1aYtog-0004qb-Mu for xen-devel@lists.xenproject.org; Thu, 25 Feb 2016 11:10:42 +0000 In-Reply-To: <1456318407-3635-2-git-send-email-write.harmandeep@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Harmandeep Kaur , xen-devel@lists.xenproject.org Cc: wei.liu2@citrix.com, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org --===============0402942892488346961== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-Xyt2A6IKuArAFK7/CKCf" --=-Xyt2A6IKuArAFK7/CKCf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote: > Signed-off-by: Harmandeep Kaur > --- > =C2=A0tools/libxl/xl_cmdimpl.c | 12 ++++++------ > =C2=A01 file changed, 6 insertions(+), 6 deletions(-) >=20 > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 116363d..8f5a2f4 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -172,7 +172,7 @@ static uint32_t find_domain(const char *p) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D libxl_domain_qualifier_to_domid(ctx,= p, &domid); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (rc) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0fprintf(stderr, "%s= is an invalid domain identifier > (rc=3D%d)\n", p, rc); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0exit(2); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0exit(EXIT_FAILURE); > I'm not sure find_domain() is a "memory related functions", so I wouldn't change it in this patch. On the other hand, there is=C2=A0parse_mem_size_kb(), which returns -1 on failure, or the amount of memory, on success. That is ok, IMO, so, don't change it. However, I think you can take the chance of this patch to add just a couple of line of comments, above its signature, explaining that it does exactly that. Also, there is freemem(), which also does look "memory related" to me. And in its case, it indeed uses libxl's error codes while it should -- being an internal function-- just use the 0/1 convention. I think you can 'fix' it in this patch. > @@ -3275,7 +3275,7 @@ static int set_memory_max(uint32_t domid, const > char *mem) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0memorykb =3D parse_mem_size_kb(mem); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (memorykb =3D=3D -1) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0fprintf(stderr, "in= valid memory size: %s\n", mem); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0exit(3); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0exit(EXIT_FAILURE); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D libxl_domain_setmaxmem(ctx, domid, m= emorykb); > The statement right below this one is: =C2=A0return rc; set_memory_max() is an internal function so, again, it should retunr 0/1 to its caller, rather than a libxl error code. The rest of this patch looks ok to me. Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-Xyt2A6IKuArAFK7/CKCf 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 iEYEABECAAYFAlbO4SwACgkQk4XaBE3IOsQTUACfZrdlKgp/IMKFpjLgXgwQc4X9 OjUAoKA5Cxg1cPVulhHt+FNB8SMSw4/m =U0dS -----END PGP SIGNATURE----- --=-Xyt2A6IKuArAFK7/CKCf-- --===============0402942892488346961== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============0402942892488346961==--