Re: [PATCH v2 05/10] xl_cmdimpl: improve return codes for memset commands

From: Dario Faggioli <dario.faggioli@citrix.com>
To: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
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
Subject: Re: [PATCH v2 05/10] xl_cmdimpl: improve return codes for memset commands
Date: Fri, 8 Apr 2016 11:41:29 +0200	[thread overview]
Message-ID: <1460108489.13871.74.camel@citrix.com> (raw)
In-Reply-To: <1460106276.5709.12.camel@localhost>

[-- Attachment #1.1: Type: text/plain, Size: 3565 bytes --]

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. 
>
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? 
> 
Well, this is something Wei and Ian have the final say on.

I'm actually fine with either approaches but, considering that:
 1. calling exit() in situations similar to this is, AFAICT, what xl
    does pretty much always;
 2. converting exit()-s to 'return 1' may (but that's on a case by
    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].
> 
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:

  return 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 
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
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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