Re: [Libguestfs] [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: 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. 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". 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