From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4 02/15] libxl: sanitize error handling in libxl_get_max_{cpus, nodes} Date: Tue, 3 Dec 2013 12:40:11 +0100 Message-ID: <1386070811.5338.300.camel@Solace> References: <20131122183332.11200.20231.stgit@Solace> <20131122185649.11200.73393.stgit@Solace> <1385559900.23112.199.camel@kazak.uk.xensource.com> <1386008488.5338.222.camel@Solace> <1386063680.16012.42.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3469497801017492403==" Return-path: In-Reply-To: <1386063680.16012.42.camel@kazak.uk.xensource.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: Ian Campbell Cc: Marcus Granado , Keir Fraser , Matt Wilson , Li Yechen , George Dunlap , Andrew Cooper , Juergen Gross , Ian Jackson , xen-devel@lists.xen.org, Jan Beulich , Justin Weaver , Elena Ufimtseva List-Id: xen-devel@lists.xenproject.org --===============3469497801017492403== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-6+iUTPl4urvVaG5GRFHJ" --=-6+iUTPl4urvVaG5GRFHJ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable `On mar, 2013-12-03 at 09:41 +0000, Ian Campbell wrote: > On Mon, 2013-12-02 at 19:21 +0100, Dario Faggioli wrote: > > I feel like it's more natural to do like this, rather than having a > > pre-patch just moving the code for no apparent reason... Isn't it? >=20 > Certainly making changes which are necessarily required by the code > motion is fine to do in a single step, although even then if you can > arrange to make the changes either before or after the move it is > better. >=20 > But is that what is happening here? >=20 > -static inline int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *c= pumap, > - int max_cpus) > -{ > - if (max_cpus < 0) > - return ERROR_INVAL; > - if (max_cpus =3D=3D 0) > - max_cpus =3D libxl_get_max_cpus(ctx); > - if (max_cpus =3D=3D 0) > - return ERROR_FAIL; > - > - return libxl_bitmap_alloc(ctx, cpumap, max_cpus); > -} >=20 > is becoming: >=20 > +int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *cpumap, int max= _cpus) > +{ > + GC_INIT(ctx); > + int rc =3D 0; > + > + if (max_cpus < 0) { > + rc =3D ERROR_INVAL; > + goto out; > + } > + if (max_cpus =3D=3D 0) > + max_cpus =3D libxl_get_max_cpus(ctx); > + if (max_cpus <=3D 0) { > + LOG(ERROR, "failed to retrieve the maximum number of cpus"); > + rc =3D ERROR_FAIL; > + goto out; > + } > + /* This can't fail: no need to check and log */ > + libxl_bitmap_alloc(ctx, cpumap, max_cpus); > + > + out: > + GC_FREE; > + return rc; > +} >=20 > which involves a lot more reworking than simply adding the GC.=20 > Well, is it? All I'm doing is adding the GC and one LOG(). In v5 it's 2 LOG()s. The rest of the rework is indeed related to the GC-ification, since I can't leave without calling GC_FREE anymore... > In any > case I don't see why the original function couldn't be moved as is and > then have the GC-ification added in the new location, I don't think the > move requires the change to using the GC In any way. >=20 Mmm... I'm not sure I'm fully understanding what you're trying to say. That function is in libxl_utils.h right now (IIRC, I put it there myself :-)) because it's a trivial wrapper of libxl_bitmap_alloc(). What is happening is that, as a result of changing the error handling in libxl_get_max_cpus(), and deciding to move logging for when it fails closer to it --which is what happens in this very patch-- I just don't think this is a trivial enough wrapper any longer. So, my view here is: if I modify the function, adding GC and the LOG()s, then it should also be moved, if not, it can stay where it is. What you seem to be suggesting is that I (either in this or a previous patch) move the function as is, for no apparent reason, and then (either in a following or this patch), introduce the GC and the LOG()s... Is that the case, or am I missing something? I see the point of not combining functional and not-functional changes, but that really looks odd to me, in this particular case. Anyway, if that's what you want, I certainly can do that... I'm not being graded against a maximum number of patches in a series, am I? ;-P > > (Anyway, feel free to look at v5 and reply there). >=20 > In general it would be better to reply to vN either up front or as you > do the rework, so we can resolve any misunderstanding rather than > waiting until after you've posted vN+1 and perhaps requiring a vN+2. >=20 Definitely, and I always do that. This round suffered from a combination of weird circumstances on my side. Sorry for that. 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) --=-6+iUTPl4urvVaG5GRFHJ 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) iEYEABECAAYFAlKdwxsACgkQk4XaBE3IOsSYlwCfXs/hZrfm8hPTUkjzxBNtpLcq WQwAniQXfo0qL06Mb48xxk3WGmGTD52o =b0KZ -----END PGP SIGNATURE----- --=-6+iUTPl4urvVaG5GRFHJ-- --===============3469497801017492403== 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 --===============3469497801017492403==--