Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies

> > > @@ -364,6 +363,8 @@ STATE_MACHINE { > > > SET_NEXT_STATE (%.DEAD); > > > return 0; > > > } > > > + if (cmd->data_seen <= cmd->count) > > > + cmd->data_seen += length; > > > > This does not feel right. if you received more data, it should be counted, > > and if this causes data_seen to be bigger than cmd->count, isn't this a fatal > > error? > > cmd->count is at most 64M; it represents how much we asked the server > to provide. length was just validated (in the elided statement > between these two hunks) to be <= cmd->count (otherwise, the server is > known-buggy for replying with more than we asked, and we've already > moved to %.DEAD state). cmd->data_seen is a cumulative count of all > bytes seen in prior chunks, plus the current chunk. If we have not > yet passed cmd->count, then this chunk counts towards the cumulative > limit (and there is no overflow, since 64M*2 < UINT32_MAX). If we > have already passed cmd->count (in a previous chunk), then we don't > further increase cmd->count, but we already know that we will fail the > overall read later. In other words, we can stop incrementing > cmd->data_seen as soon as we know it exceeds cmd->count, and by > stopping early, we still detect server bugs without overflowing > uint32_t. But we can have this case: 1. ask for 32m 2. server sends 16m (data_seen increase to 16m) 3. server sends 16m (data_seen increase to 32m) 4. server sends 1m (data_seen does not increase)

5. entire request succeeds Shouldn't we fail if server sends unexpected data? If we detected that all data was received, and we get unexpected data, why not fail immediately? cmd->data_seen += length if (cmd->data_seen > cmd->count) switch to dead state?