Libguestfs - Mar 2023 - [libnbd PATCH v4 1/3] lib/utils: introduce xwritel() as a more robust and convenient write()

On 3/15/23 15:01, Eric Blake wrote:
> [...]
Thanks for the thorough review; I'm glad all the fine points I sought to
put in the patch were received -- and well-received! :)

One question:
> The only change I recommend is the addition of the __attribute__; but
> with or without it, I'm happy with:
Do we have general rules on attribute usage in libnbd vs. nbdkit?

The __sentinel__ (aka sentinel) attribute is used in nbdkit, but not yet
in libnbd. Now, that could be happenstance, but it rhymes with another
(obscure?) discrepancy in attribute usage.

Namely, when I was comparing the common/ subdirectories of both
projects, I noticed that nbdkit used the cleanup attribute, and libnbd
didn't. First I thought it was a mistake / oversight, but then I found a
porting note from Rich, in libnbd commit f306e231d294 ("common/utils:
Add extensible string, based on vector", 2022-03-12):

    RWMJ: This removes the CLEANUP_FREE_STRING macro since libnbd does not
    use __attribute__((cleanup)).

and then again in f3828bfd42be ("common/utils: Add new string vector
types", 2022-03-12):

    RWMJ: Removed the CLEANUP_* macros.

So those comments (esp. the one on commit f306e231d294) at least confirm
that the difference is intentional. I still don't know the reason for
the difference. And now I wonder: does the same (unexplained) reason
underlie the "sentinel" attribute's absence too, in libnbd?

If there is a common reason for avoiding both "cleanup" and
"sentinel"
in libnbd, we should probably not start using "sentinel" now. If, on
the
other hand, "sentinel" is not covered by the same argument as
"cleanup"
(not to mention if there isn't an actual reason for avoiding
"cleanup"
in the first place!), then I can add the sentinel attribute when merging
this patch.
> Reviewed-by: Eric Blake <eblake at redhat.com>
Thanks!
Laszlo

On Wed, Mar 15, 2023 at 03:30:12PM +0100, Laszlo Ersek
wrote:
> On 3/15/23 15:01, Eric Blake wrote:
> 
> > [...]
> 
> Thanks for the thorough review; I'm glad all the fine points I sought
to
> put in the patch were received -- and well-received! :)
> 
> One question:
> 
> > The only change I recommend is the addition of the __attribute__; but
> > with or without it, I'm happy with:
> 
> Do we have general rules on attribute usage in libnbd vs. nbdkit?
> 
> The __sentinel__ (aka sentinel) attribute is used in nbdkit, but not yet
> in libnbd. Now, that could be happenstance, but it rhymes with another
> (obscure?) discrepancy in attribute usage.
I think it's happenstance; until today, libnbd did not yet have a
varargs function where annotating the need for a NULL terminator was
useful to let the compiler aid in flagging erroneous usage.
> 
> Namely, when I was comparing the common/ subdirectories of both
> projects, I noticed that nbdkit used the cleanup attribute, and libnbd
> didn't. First I thought it was a mistake / oversight, but then I found
a
> porting note from Rich, in libnbd commit f306e231d294 ("common/utils:
> Add extensible string, based on vector", 2022-03-12):
> 
>     RWMJ: This removes the CLEANUP_FREE_STRING macro since libnbd does not
>     use __attribute__((cleanup)).
> 
> and then again in f3828bfd42be ("common/utils: Add new string vector
> types", 2022-03-12):
> 
>     RWMJ: Removed the CLEANUP_* macros.
>
Most attributes are merely extensions that aid the compiler in aiding
you.  __attribute__((sentinel)) is squarely in this camp - compilers
that understand it can warn on questionable code, while you can
#define a wrapper to an empty string for all other compilers with no
change in program behavior.

But __attribute__((cleanup)) is a special beast - it affects runtime
behavior, and if you use it, you are REQUIRED to have compiler
support.  There is no way to write preprocessor macros that will
provide the same runtime functionality that cleanup implies for use by
a purely standards-compliant cc.  That said, it is still a localized
compile-time effect, and does not impact ABI - it is merely reducing
(a lot!) of boilerplate coding that would otherwise be needed without
the attribute in play.  I see no problem in mixing an executable that
uses it with a library that does not (nbdkit does just that - our
server uses cleanup, but can run a plugin compiled without cleanups
just fine), nor with mixing a library that uses it with an executable
does not (which could be the case if libnbd starts using it).

Rather, the drawback of using __attribute__((cleanup)) in libnbd is
that we would now REQUIRE libnbd to be compiled with gcc or clang.
Right now, I don't know if anyone is trying to use libnbd with an
alternative compiler (no one has submitted patches or bug reports for
using libnbd under MSVC, for example), so it may be a non-issue.  But
it's a one-way bridge - once we explicitly decide that we expect a
particular extension to the standards to even be able to use the
library, it becomes a lot harder to port the code to other platforms
without that specific compiler extension without replacing it back to
a lot of boilerplate code in its place.

At the time we copied the vector code from nbdkit over to libnbd, we
weren't sure what environments would try to use libnbd, so we
intentionally did not port attribute cleanup stuff to avoid crippling
an unknown user.  I'm not opposed to using the cleanup attribute, and
if we DO decide to use it, I'd love to go all in and utilize it
wherever it makes sense, which is more than just with of vectors.
Maybe the thing to do is have one major release where we announce our
intention to utilize the attribute in a future release, unless someone
speaks up with a reason why it would break with their preferred
toolchain; it delays the decision, and means we can't use it right
away, but at least would be a documented transition rather than a
blind "sorry you can't build anymore".
> So those comments (esp. the one on commit f306e231d294) at least confirm
> that the difference is intentional. I still don't know the reason for
> the difference. And now I wonder: does the same (unexplained) reason
> underlie the "sentinel" attribute's absence too, in libnbd?
> 
> If there is a common reason for avoiding both "cleanup" and
"sentinel"
> in libnbd, we should probably not start using "sentinel" now. If,
on the
> other hand, "sentinel" is not covered by the same argument as
"cleanup"
> (not to mention if there isn't an actual reason for avoiding
"cleanup"
> in the first place!), then I can add the sentinel attribute when merging
> this patch.
I think the argument for not backporting "cleanup" is much different
than the one for not having needed to use "sentinel" to date.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org