Re: [Libguestfs] [libnbd PATCH v3 08/22] block_status: Track 64-bit extents internally

Fair warning: this patch has made me both very excited and very uncomfortable; that's the reason my style might come across as needlessly "combative". I don't mean to be combative. It's just how my thoughts have come out, and (unfortunately) I need to go pick up my son now, and don't have time for further rounds of polishing my style. I apologize in advance. Again, it's a consequence of me caring a bit too much perhaps, and getting worked up. On 5/25/23 15:00, Eric Blake wrote: Yes. Per spec, negotiating NBD_OPT_EXTENDED_HEADERS is equivalent to the server sending NBD_REPLY_TYPE_BLOCK_STATUS_EXT, as opposed to NBD_REPLY_TYPE_BLOCK_STATUS. I agree that a 64-bit-only internal representation makes sense. Once corresponding code part (quoting out of order for discussion's sake) is: and that looks good to me. I absolutely don't follow this. The spec is clear. NBD_OPT_EXTENDED_HEADERS corresponds to NBD_REPLY_TYPE_BLOCK_STATUS_EXT, and (!NBD_OPT_EXTENDED_HEADERS) corresponds to NBD_REPLY_TYPE_BLOCK_STATUS. If NBD_OPT_EXTENDED_HEADERS is negotiated (which means both client and server agree to it), then the client interface is 64-bit clean, so there's no need to truncate anything. If NBD_OPT_EXTENDED_HEADERS is *not* negotiated, then our internal 64-bit clean representation will never be filled with information that doesn't fit into 32-bits (due to the reply type being restricted to NBD_REPLY_TYPE_BLOCK_STATUS), so there is again no need to (observably) truncate anything (technically there would be a uint64_t to uint32_t conversion, but it would be value-preserving). The spec also says that, if a client wants to query a particular metadata context that is known to use 64-bit values, then the client must first negotiate NBD_OPT_EXTENDED_HEADERS. Again, I don't follow. Whenever the client issues an NBD_CMD_BLOCK_STATUS command (which can only occur after negotiation): - the client must have set at least one metacontext with NBD_OPT_SET_META_CONTEXT during negotiation, - the client may or may not have agreed upon with the server on NBD_OPT_EXTENDED_HEADERS. Therefore, when the server receives the NBD_CMD_BLOCK_STATUS command, it can immediately decide whether there is *any* configured metacontext for which *lack* of extended headers could be too restrictive. If only 32-bit metacontexts have been requested, this is not a problem. If ext-hdrs have been negotiated, also not a problem. If there is at least one 64-bit metacontext (regarding *either* status flags *or* lengths) *and* ext-hdrs have not been negotiated, the server should immediately fail (reject) the request. Now, peeking a bit forward, I can see patch#10: "api: Add [aio_]nbd_block_status_64". That means we can reject a 32-bit-only nbd_block_status() API call on the client side immediately, if the client has negotiated ext-hdrs. That is, if the client is smart enough to ask for extended headers, it can be smart enough to use the 64-bit API, and then it can accept everything the server throws at it. If the client never even asks for extended headers, then it is permitted to stick with the 32-bit-only API, but then the server must never respond with NBD_REPLY_TYPE_BLOCK_STATUS_EXT [*], so actually 64-bit values are never going to be present in our internal representation. [*] If it still does, then the server is buggy, and we should realize that as soon as we parse the generic reply magic, or the specific reply type code. I simply fail to see the *possibility* truncating the internal 64-bit representation to 32-bit such that it would make a difference for the caller. Speaking in terms of code: Side question: what guarantees that the multiplication won't overflow? If we have a guarantee, we should document it, and (preferably) assert it. Yes, and we should absolutely enforce that. Don't we break off the connection (or at least abort the request) when we get the wrong reply magic from the server, or else if we do get the right reply magic, but an NBD_REPLY_TYPE_BLOCK_STATUS_EXT reply type code inside, without having negotiated extended headers? This compatibility goo creates a big maintenance and debugging burden. Back to the commit message: I'm not worried about the speed penalty, but at this point (and looking at the rest of the patch) I'm actually changing my mind, and questioning whether we are better off with the unified, 64-bit only internal representation. It's not just about converting back and forth (and hence code complexity): we're introducing further malloc()s, and those are all new failure points. Even if we do handle memory allocation failures everywhere correctly, they still introduce pontential *late* breaking points for the client. Right, and I happen to disagree with your implementation; more on that below. Now this could be a good approach in my opinion; however, I disagree with how it's being used (the in-place conversion below, which IMO arguably runs afoul of the effective type rules in C). This division will work as expected, but it's hard to see why. After my request under patch#1, for introducing bs_reply_length_ok(), and asserting it here, it will be easier to understand. I do dislike the open-coded "sizeof (uint32_t)" though, we should say "sizeof *h->bs_entries.narrow". So I find this way too clever; worse, it almost certainly breaks the effective type rules in C. First, I would be OK with storing pointers in the bs_entries union, if our only purpose with the union were to only handle one of those pointers at any given time. However, we're type punning the pointers back and forth, and relying on their representation being identical. (Pointer to structure vs. pointer to char vs. pointer to uint32_t.) Not catastrophic, but not nice (and IMO not even necessary, see below). Second, after reading the effective type rules very carefully, the code might break them minimally (I think) when "i" equals zero, and the "length" field is assigned. In that instance, you are reading the same memory location as a uint32_t and writing it as a uint64_t, and not going through a union. I'd prefer one of the two following options: - bs_entries should indeed be the union that you recommend, but used differently. First, "storage" should be allocated, and filled. When the reading completes, a new uint32_t array should be allocated, populated via byte-swapping, "storage" should be released, and the new (now byte-order-correct) array should be recorded in "narrow". Same approach for the future "normal" population. The advantages of this approach are: (a) not reading a pointer-to-type-X value as a pointer-to-type-Y value (reinterpreting the bit pattern) via the union, as we should always know what reply type we're dealing with, (b) any allocated array would only be accessed with a singular element type at any point in time. A kind of "double buffering". - Better yet: drop the "storage" field from the union, and only have "narrow" and "normal" in it. Treat them entirely separately. For the 32-bit case, simply rename the field references (from bs_normal to bs_normal.narrow), no need to change the existent logic. For the 64-bit case, duplicate and widen the 32-bit case (the byte swapping etc), using bs_entries.normal. [empty line here due to earlier out-of-order quoting] For better readability, "data" should be "user_data" IMO, and "cb" should be "shim". I disagree with this order of operations. (Note, I'm not saying there are leaks or double free's.) Consider the nbd_unlocked_aio_block_status() call site in nbd_aio_block_status(): If that call succeeds (returns a value different from (-1)), then we expect nbd_unlocked_aio_block_status() to have taken ownership of both "extent_callback" and "completion_callback", by emptying them. That way, the FREE_CALLBACK()s at the end of the function decay to NOPs. If the call fails OTOH, we expect the call to have left "extent_callback" and "completion_callback" untouched, and the FREE_CALLBACK()s at the end to have "fangs" -- perform actual freeing. Pre-patch, nbd_unlocked_aio_block_status() conforms to this interface contract quite well. It first goes through all possible local error branches, and if all of those pass, only then does it zero the callback structures (i.e., takes ownership of them). This patch changes that; we take ownershop of "extent_callback" (by copying it to "shim" and nulling it) before we check our local error conditions, such as structured_replies and metadata context validity. That breaks the interface contract, and so we cannot rely on nbd_aio_block_status() for freeing "extent_callback" when we fail in one of those local error conditions -- this is why we have to go through FREE_CALLBACK (cb.fn.extent) on each of those error paths, for freeing *both* "shim" and the "extent_callback" internals. And (most importantly to me) then we return to the caller, namely nbd_aio_block_status(), in such a way that (a) we fail, yet (b) we have already destroyed "extent_callback" (but not "completion_callback"). The caller copes with this (due to how FREE_CALLBACK() works), but it's unclean. If nbd_unlocked_aio_block_status() fails, it shouldn't have messed with "extent_callback". (I notice a *slight* inconsistency with this requirement of mine in the pre-patch code as well, BTW: pre-patch, we null both callback structures *before* calling nbd_internal_command_common(); if the latter fails, we get into the same situation. So I actually tend to disagree with that too.) Anyway, I would suggest (on top of your patch): Which, after squashing, would result in a patch like: But, to repeat myself -- I'm not convinced any more we benefit from the 64-bit-only internal representation. We should always be able to tell precisely which one of the 32-bit and the 64-bit internal representations we need. We should handle each separately, and then we don't need the conversions + shim. Laszlo