Re: [Libguestfs] [PATCH libnbd] generator: Pass LISTEN_FDNAMES=nbd with systemd socket activation

On 1/28/23 13:47, Richard W.M. Jones wrote: I really didn't want to obsess about this -- I spent like 10+ minutes on curbing my enthusiasm! :) --, but I believe there's a semantic bug in the patch, one that's directly related to my "hidden" thoughts. (1) In the last hunk, strlen() is applied to "LISTEN_FDS=". However, the zero-index element of the env array holds "LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx". In other words, the patch only gets lucky because "PID" and "FDS" are both three characters long. Relatedly, my hidden thought was that we shouldn't use so many naked string literals all over the code. May I take a stab at rewriting this? I feel that's the easiest way for me to express what I'd propose. Basically I'd propose: - an enum for listing the "keys" we need, - a static array of structure elements, for expressing the environment variables (name=value), *and* the prefix lengths, - a macro for populating an element of the array -- use "sizeof" for grabbing the prefix length (2) Pre-patch, at commit 5a02c7d2cc6a, the error handling tail of prepare_socket_activation_environment() is less than ideal, IMO. Namely, we have (excerpt) (2a) we free the vector's pointer field, but don't NULL it, nor do we zero the len or cap fields. We should call string_vector_reset() instead. (2b) Casting the address of the free() function to (void*) makes me uncomfortable. It is undefined behavior by ISO C. Now, I seem to remember that POSIX says in various places that pointers to functions and pointers to void have identical representation, and also that pointers to void and pointers to structures have identical representation. One of those locations is the dlsym() spec <https://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html>;. The other locations elude me, unfortunately. I think at least one of those "other" locations may be in one of the Conformance sections; Eric will know better. Regardless, casting "free" to a pointer-to-object, just because string_vector_iter() takes a (void(*)(char*)), and not a (void(*)(void*)), is questionable style, IMO. I've grepped the tree for this pattern: git grep -E '\(void ?\*\) ?free' and there are eleven hits. Furthermore, there are *no other* _vector_iter() calls -- and not just string_vector_iter() calls, but in general, _vector_iter() ones! -- than these eleven. I think it's time we designed either a general freeing iterator API for vector, or at least added a trivial (stop-gap) wrapper function like this: Comments please :) (3) At the last hunk, the code suggests we're between fork() and exec(). Per POSIX <https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html>;, there we can only call async-signal-safe functions: The list of async-signal-safe functions can be found at <https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html...;. snprintf() and sprintf() are not on that list, so it makes sense for nbd_internal_fork_safe_itoa() to exist. The remaining functions we call in this context also seem to be on the list... except for execvp(). execvp() scans PATH, and is not safe to use in this concept. I think we should call execve() instead. First, it is async-signal-safe. Second, it could take "env.ptr" directly; I do find the "environ" assignment a bit dubious, even if it happens to conform to POSIX. What image are we executing here, to begin with? Do we really depend on PATH searching? Or do we rely on execvp() transparently launching shell scripts? All that said, I think we can stick with this patch; the only "actual" problem I see with it is the "LISTEN_FDS" reference in the last hunk. Thanks, Laszlo