8:30 p.m.
On Thu, Mar 16, 2023 at 10:30:22AM +0100, Laszlo Ersek wrote:
>
...
> 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.
No problem - it also helps me to write it down in the archives ;)
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.
I didn't search either, but it's been a while. A quick grep of gnulib
finds:
m4/gnulib-common.m4:# define _GL_ATTR_sentinel _GL_GNUC_PREREQ (4, 0)
which is fairly old these days. (That file dates a lot of other
attributes as well...)
(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.
In general, gcc has __name__ aliases for ALL __attribute__((name))s,
precisely for public headers. So my personal rule of thumb is if it
is a .h to be installed, add wings; if it is local to the project, the
shorter version is fine.
(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,
Indeed.
(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,
nbdkit made the conscious decision to enforce clang or gcc for
__attribute__((cleanup)); as such, it can rely on that assumption in
most cases. But you are right about nbd-protocol.h being shared; this
may be one case where we could rework nbdkit's copy to not be so
compiler-specific if it lets libnbd compile on a wider array of
compilers. Or it may be proof that no one is really compiling libnbd
with anything other than clang/gcc, at which point libnbd using
__attribute__((cleanup)) is not going to cause anyone grief after all.
(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".
Fair enough. If it fails to compile, we can add a wrapper at that
time (the main reason for a wrapper is to allow building with a wider
array of compilers).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org