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

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

On 3/15/23 18:14, Eric Blake wrote:
> 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.
Thanks for the detailed explanation.

While I don't like this extra (orthogonal) complexity, I've made an
effort to collect some information.

(1) I couldn't figure out at what clang / gcc version the sentinel
attribute was introduced.

(2) The gcc manual says that __attribute__ ((__sentinel__)) is
equivalent to __attribute__ ((sentinel)); it's just that the former is
more suitable for public header files, to be included by client apps,
where "sentinel" could already exist as a macro, while __sentinel__
couldn't.

This is in fact (at least superficially) consistent with nbdkit's usage
of the attribute; we have "__sentinel__" in
"common/utils/utils.h" and
"tests/test.h" (header files -- albeit not public), and
"sentinel" in
"tests/test-layers.c".

So I'll squash "__attribute__ ((sentinel))" into the C code of
this patch.

(3) Whether or not we should add a wrapper macro. Libnbd is not really
consistent in this regard. The generator defines
LIBNBD_ATTRIBUTE_NONNULL and LIBNBD_ATTRIBUTE_ALLOC_DEALLOC -- those are
for the public "libnbd.h" header, so the wrapper macros are certainly
justified there.

The question is however what the libnbd-internal code does. And that's
*seemingly* inconsistent:

(3.a) the internal code consumes LIBNBD_ATTRIBUTE_NONNULL and
LIBNBD_ATTRIBUTE_ALLOC_DEALLOC liberally, from the public (generated)
library header -- likely because that's the convenient thing to do,

(3.b) in a single case, we have an internal-only wrapper:
NBD_ATTRIBUTE_PACKED in "lib/nbd-protocol.h" (whose definition
effectively enforces clang or gcc) -- and this seems to be shared with
nbdkit,

(3.c) we have a bunch of internal code that uses naked attributes, such
as "__nonnull__", "__unused__", "constructor",
"noreturn", "destructor",
"packed".

For (3.a) and (3.b), I can cook up a guiding principle -- both
"libnbd.h" and "nbd-protocol.h" look public, at least to an
extent,
while the stuff in (3.c) is totally internal.

So, I can equate wrapper macros to public headers, for now, and I won't
introduce a new macro just for this one application of "__attribute__
((sentinel))" in "lib/utils.c".

Laszlo