Re: [Xen-devel] [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Nov 29, 2018, at 5:09 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
> 
> George Dunlap writes ("Re: [PATCH 5/9] libxl: Do root checks once in 
> libxl__domain_get_device_model_uid"):
>>> On Nov 28, 2018, at 4:39 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
>>> I know that in the hypervisor this kind of thing is tolerated (wrongly
>>> IMO) but can we please not have it here.
>> 
>> It is a bit strange having to work with one maintianer who thinks a handful 
>> of simple gotos is an issue, and another maintainer who thinks having switch 
>> case statements appear in the middle of if() { } blocks is perfectly normal. 
>> :-)
> 
> Sorry, I missed that.
> 
> I think maybe indeed a CODING_STYLE update is called for.

…not sure what you mean; I’m referring to a hypervisor maintainer who regularly 
generates code like:

switch() {
case A:
  if(blah) {
case B:
     foo();
  }
}

I’m not complaining, I’m just slightly amused at the difference. :-)

> 
>>> This may mean splitting stuff out into a sub-function.  That could be
>>> done some time between "Move dm user determination logic into a helper
>>> function" and this patch I guess.
>> 
>> I’m afraid you’re going to have to give me a bit more guidance here: It’s 
>> not clear to me what would be split into a sub-function, and how that would 
>> make the code easier to follow while avoiding unnecessary code duplication.
>> 
>> Do you propose replacing “goto root_check;” with “root_check(); goto out;” 
>> in all locations?  Or something else?
> 
> I am afraid that I have not followed all of the strands of code here.
> I am very tempted to reply simply with a reference to Dijkstra [1968],
> but:

I read that paper; it was nearly two decades ago now, but my memory was that 
most of his objections are made moot by languages such as C, particularly if 
your compiler is configured to throw an error if a label is unused.

> 
> I think some of your gotos could be refactored into early exits from a
> new subfunction.  In any case, I would like the code to be made out of
> block structured pieces such as for and while loops (perhaps with
> early exits like break and continue), if statements, switch
> statements, conditionals, and so on.

So the situation is, we have three “found a UID” cases (device_model_uid set, 
QEMU_USER_RANGE_BASE uid present, QEMU_USER_SHARED uid present).  In all three 
cases, we want to make sure the final UID is not root, but in two of them 
(device_model_uid and QEMU_USER_SHARED) we need to check user_base->pw_uid, and 
in the other one (QEMU_USER_RANGE_BASE), we need to check our calculated uid 
value (intended_uid).

Also, at some point in the development of the series, it wasn’t clear whether 
device_model_uid was allowed to be root (which is how patch 4 came about).

So it sounds like instead of this:

8<---
int a() {
  if (c1) {
    /* do something */
    goto check_1;
  }

  if (c2) {
    /* do something */
   goto out;
  }

  if (c3) {
    /* do something */
   goto check;
  }

  /* Fail */

check:
  /* do check, maybe fail */

out:
  /* Do common “out” things */
}
—>8

You want this:
8<—

int a() {
  if (c1) {
    /* do something */
    if(check())
      return error;
    goto out;
  }

  if (c2) {
    /* do something */
   goto out;
  } 

  if (c3) {
    /* do something */
    if(check())
      return error;
  }

  /* Fail */

out:
  /* Do common “out” things */
}

int check() {
  /* do check */
}
—>8

All I can say is, the second one seems less clear and more repetitive to me.  
It contains many more control flow changes for each check(), rather than one 
(since a function call is a type of goto), and you have to duplicate handling 
the error condition in each of the return sites.

But anyway, I’ll see what I can do about re-arranging things to have only a 
single `out:` label (getting rid of the style-non-conformant `return`s all over 
the function as well).  I think it should be possible without an extra function.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel