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

[Laszlo Ersek]

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