[libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init

[Andrea Bolognani]

On Mon, 2018-09-03 at 17:13 +0200, Michal Privoznik wrote:
> On 08/31/2018 07:35 PM, John Ferlan wrote:
> > > +    if (!(ret->lockPlugin = virLockManagerPluginNew(lockManagerName,
> > > +                                                    NULL, NULL, 0))) {
> > > +        goto error;
> > > +    }
> > 
> > The { } aren't necessary, but understandable.
> 
> Andrea would disagree. In one other patch that I had on the list
> recently he made me to put the brackets there arguing that if() is not a
> single line or something.

The relevant chapter of our guidelines[1] states:

  Omit the curly braces around an if, while, for etc. body only
  when both that body and the condition itself occupy a single
  line. In every other case we require the braces. This ensures
  that it is trivially easy to identify a single-statement loop:
  each has only one line in its body.

  while (expr)             // single line body; {} is forbidden
      single_line_stmt();

  while (expr(arg1,
              arg2))      // indentation makes it obvious it is single line,
      single_line_stmt(); // {} is optional (not enforced either way)

  while (expr1 &&
         expr2) {         // multi-line, at same indentation, {} required
      single_line_stmt();
  }

In the recent patch you mention[2] we were squarely in Example
Three territory, so there was no question about whether or not
curly braces should be added.

This time around we may refer to Example Two, "not enforced
either way", but given just how much whitespace there is on
the second line I would argue the braces should definitely be
there to disambiguate the situation.

> On a side note, this points to a bug in our coding style. Either we
> should put brackets everywhere (even for true single line if()-s like
> those in the context above), or not be picky about "multiline" if()-s
> like the one we are talking about here.

Completely agree. We have way too many subtleties in our
guidelines, which leads to uneven adoption despite
syntax-check and people like me annoying contributors during
review :)

While switching to an all braces, all the time approach is
probably not realistic due to the sheer amount of changes
that would be required to make existing code compliant, we
could start requiring it for new code and go from there.


[1] https://libvirt.org/hacking.html#curly_braces
[2] https://www.redhat.com/archives/libvir-list/2018-August/msg01269.html
-- 
Andrea Bolognani / Red Hat / Virtualization