Re: [Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()

On 2/17/23 19:36, Eric Blake wrote: No, that's not the right explanation. Elsewhere in the code, we do not use *assignments* for prepping our vector structures with "empty_vector". Instead, everywhere else in the code, we use *initialization*. Those are different contexts. If you expand the "empty_vector" macro in the below: string_vector v1 = empty_vector; string_vector v2; v2 = empty_vector; we get: string_vector v1 = { .ptr = NULL, .len = 0, .cap = 0 }; string_vector v2; v2 = { .ptr = NULL, .len = 0, .cap = 0 }; For "v1", we have a definition that is also an initialization, so the initializer { .ptr = NULL, .len = 0, .cap = 0 } is fine there. For "v2", we have a definition that is not an initialization, and we have an additional *assignment*. For the right hand side of the assignment, the initializer { .ptr = NULL, .len = 0, .cap = 0 } is *not* fine. However, if we write: v2 = (string_vector){ .ptr = NULL, .len = 0, .cap = 0 }; then the RHS of the assignment becomes a C99 *compound literal*, which the initializer { .ptr = NULL, .len = 0, .cap = 0 } is a *proper part of*. The compound literal (string_vector){ .ptr = NULL, .len = 0, .cap = 0 }; is a nameless object with automatic storage duration. It's something like: string_vector v2; string_vector nameless_empty = { .ptr = NULL, .len = 0, .cap = 0 }; v2 = nameless_empty; ... I have a few more comments here: - I generally prefer to *avoid* initialization (especially initialization where the initializer is complex, like a function call!), and use assignments that are performed as late (i.e., as narrowly) as possible. In my opinion, this conveys information about data flow. (I don't like arguments such as "let's initialize this just to be sure" -- we should *know* when a variable is first consumed!) Anyway, in the present case, this argument of mine is not relevant, because the object in question (*env) already exists, we're not *defining* it in the first place (~ definition is such a declaration that allocates space), so we cannot initialize it either (in the C language sense); we can only assign another structure to it, or access its fields, or use memset(). C code that predates C99 would certainly use individual field accesses: env->ptr = NULL; env->len = 0; env->cap = 0; or call memset: memset (env, 0, sizeof *env); or use a small "initializer function" that would thinly wrap one of the above variants. However, in C99, we have compound literals, and that's a great way for reusing the "empty_vector" macro in a (structure) *assignment*. I agree, to an extent. The particular "malloc" string literal is somewhat inexact at this point, but semantically OK. It will become more inexact only later. However, my main point here is that this set_error() call does not belong in the callee. It belongs in the caller. First because the callee does lots of things (and will do more). Second, because the caller already has explicit set_error() calls on all other error branches. Yes, that's it, basically. Thanks, Laszlo