From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 05/10] xl_cmdimpl: improve return codes for memset commands Date: Fri, 8 Apr 2016 11:41:29 +0200 Message-ID: <1460108489.13871.74.camel@citrix.com> References: <1459943163-18697-1-git-send-email-paulinaszubarczyk@gmail.com> <1459943163-18697-6-git-send-email-paulinaszubarczyk@gmail.com> <1460103999.13871.31.camel@citrix.com> <1460106276.5709.12.camel@localhost> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1081240201483835789==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aoSv5-0001Yw-9k for xen-devel@lists.xenproject.org; Fri, 08 Apr 2016 09:41:39 +0000 In-Reply-To: <1460106276.5709.12.camel@localhost> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Paulina Szubarczyk Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, George.Dunlap@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xenproject.org, roger.pau@citrix.com List-Id: xen-devel@lists.xenproject.org --===============1081240201483835789== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-24kM02HhZCtF7wyo8tFD" --=-24kM02HhZCtF7wyo8tFD Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-04-08 at 11:04 +0200, Paulina Szubarczyk wrote: > On Fri, 2016-04-08 at 10:26 +0200, Dario Faggioli wrote: > > Mmmm.. I see no reason why this can't remain exit(). In fact, it > > should > > be turned int exit(EXIT_FAILURE), and there's Harmandeep's series > > -- > > just resubmitted by me tonight-- outstanding that does that [1]. > There was a discussion [2] that the exit() could be replaced by > return 1 > in the v1 of this patch, that is way I did here and in the following > commits.=20 > Yeah, and in fact, sorry for not participating in that discussion... I wanted to, but was otherwise engaged at the time. > Should it be rather reverted?=C2=A0 >=20 Well, this is something Wei and Ian have the final say on. I'm actually fine with either approaches but, considering that: =C2=A01. calling exit() in situations similar to this is, AFAICT, what xl =C2=A0 =C2=A0 does pretty much always; =C2=A02. converting exit()-s to 'return 1' may (but that's on a case by =C2=A0 =C2=A0 case basis) require a bigger patch; Right now, given the status this is in xl, I would just focus on making sure that the program terminates with either EXIT_SUCCESS or EXIT_FAILURE; if that comes from an exit() being called in a non-top level functions, so be it... And, in fact, in this case, the quickest and cleanest way to make that happen, is doing what George's patch does, which is neither exit() nor 'return 1'. :-/ > > In any case, this patch is probably not necessary any longer, not > > because Harmandeep pending series, but because George take care of > > what > > I think you're trying to do in here in > > commit 0614c454209ac67016e2296577abfee9e9dcb012 already. > In that George's commit the set_memory_* functions return > EXIT_{FAILURE/SUCCESS}. Following the patches from Harmandeep's > series I > was going to changed them to return 1/0 and return > EXIT_{FAILURE/SUCCESS} in main_* functions, after asking about that > in > that thread [3], though, you said there are exceptions too that in > [1]. >=20 Yep, it's again, at least up to a cartain extent, a matter of taste and something that is hard to carve in stone, leaving no room for exceptions. For example, in principle, what you say you're planning to do is correct: internal functions should return 0/1, top level function (and main_foo() are top level functions) should either exit() or return EXIT_SUCCESS/EXIT_FAILURE. But. Although set_memory_max() certainly is an internal function, it is _only_ used from main_memmax() like this: =C2=A0=C2=A0return set_memory_max(domid, mem); and since main_memmax() is a top level function, if it were my call, I'd be more than ok with bending the above stated rule and allow set_memory_max() to return EXIT_SUCCESS/EXIT_FAILURE. IOW, since it's return code is always directly returned by a top level function, I think we can allow set_memory_max() (and functions used in similar ways) to return just like top level functions do. I know, it's tricky, because it's not just a matter of "blindly" applying a set of rules, you have to consider all the implication --and=20 there are many of them-- on a case by case basis. It's what (among other things), IMO, makes Open Source challenging, but also beautiful and rewarding. :-D Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-24kM02HhZCtF7wyo8tFD 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 v2 iEYEABECAAYFAlcHfMkACgkQk4XaBE3IOsS42wCgnPgPf3RvKpU3TUpyzCCJtr3Q TN8AnipPBRlGqR4hmCY5/IU3VTufJYgC =Frbb -----END PGP SIGNATURE----- --=-24kM02HhZCtF7wyo8tFD-- --===============1081240201483835789== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============1081240201483835789==--