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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
