[libvirt PATCH v5 9/9] lxcDomainDetachDeviceHostdevUSBLive: Use VIR_WITH_OBJECT_LOCK_GUARD

[Martin Kletzander]

On Wed, Feb 02, 2022 at 08:17:53AM +0100, Michal Prívozník wrote:
>On 2/2/22 01:06, Martin Kletzander wrote:
>> On Tue, Feb 01, 2022 at 05:23:33PM +0100, Tim Wiederhake wrote:
>>> On Tue, 2022-02-01 at 16:07 +0000, Daniel P. Berrangé wrote:
>>>> On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote:
>>>> > On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
>>>> > > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
>>>> > > ---
>>>> > > src/lxc/lxc_driver.c | 6 +++---
>>>> > > 1 file changed, 3 insertions(+), 3 deletions(-)
>>>> > >
>>>> > > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
>>>> > > index 7bc39120ee..42053de9c3 100644
>>>> > > --- a/src/lxc/lxc_driver.c
>>>> > > +++ b/src/lxc/lxc_driver.c
>>>> > > @@ -4045,9 +4045,9 @@
>>>> > > lxcDomainDetachDeviceHostdevUSBLive(virLXCDriver *driver,
>>>> > >         VIR_WARN("cannot deny device %s for domain %s: %s",
>>>> > >                  dst, vm->def->name, virGetLastErrorMessage());
>>>> > >
>>>> > > -    virObjectLock(hostdev_mgr->activeUSBHostdevs);
>>>> > > -    virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs, usb);
>>>> > > -    virObjectUnlock(hostdev_mgr->activeUSBHostdevs);
>>>> > > +    VIR_WITH_OBJECT_LOCK_GUARD(hostdev_mgr->activeUSBHostdevs) {
>>>> > > +        virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs,
>>>> > > usb);
>>>> > > +    }
>>>> >
>>>> > Even nicer you can omit the curly brackets and make this a +2/-3 ;)
>>>> > Otherwise I guess we'd need some kind of stylistic guide to keep us
>>>> > consistent in this regard right from the start.
>>>>
>>>> My preference is the keep the scope of the guarded section explicit
>>>> using {} even with a 1 line body.
>>>
>>> Same for me, fwiw.
>>>
>>
>> OK, I don't really have a horse in this race.  But in that case it would
>> be nice
>> to document it in our guidelines, maybe together with few words around
>> the new
>> macros.  In a later patch for example.
>
>Since you brought this up, I vaguely remember us discussing this
>earlier, though it was for one line body of if()-s. I too am in favor of
>having curly braces in that case, except maybe when the body is a goto
>or return statement. IOW:
>
>  if (cond)
>    return -1; // good
>
>  if (cond) {
>    func();  // good
>  }
>
>  if (cond) {
>     goto cleanup; // not a big fan
>  }
>
>OTOH, consistency matters more so I can live with the last example too.
>

It depends how much we like consistency, right?  I used to prefer curly
brackets around any sub-statement, even one-liners, but then I started
contributing to libvirt and had to adapt.  If we want to be 100%
consistent then it would make sense to just have them all the time even
though I do not prefer that style any more, but I would adapt.  Everyone
would, I think.

And that leads me to another idea which I mentioned in the past
already.  I don't want to repeat myself too much, but "since you bought
this up" and this is very related, here goes nothing.

All this talk about "which subjective way of writing the same code
should we choose" seems a bit of a moo point since even though we always
mention consistency we end up choosing what we like better.  But since
no decision will cater to everyone and everyone will, at some point,
have to adapt, maybe there is a better solution to the problem.  Maybe
the solution is even more consistency so that there are fewer
exceptions.  So what if we find another advantage/reason for deciding
on a way to format the code?

One possible advantage would be that if we choose to comply with a
format that can be easily enforced (something that indent / clang-format
/ whatever can do a good job of) we could erase lot of docs, lot of
syntax checking code, reduce the knowledge required for newcomers to
send proper code, make it easier to automatically fix code to comply to
the rules, and so on.  The only disadvantage would be that most of us
would have to adapt and my point is that we have to do that anyway (new
syntax, new "libvirt language", working on other projects) and more
consistency just makes it even easier IMHO, even if it takes a bit
longer.

Anyway, just something I felt like mentioning
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220202/990bccc1/attachment-0001.sig>