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: > 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.