On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer wrote:
> > > @@ -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)
Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.
Then, later on when retiring the command, we note that 33m != 32m and
fail the read with EIO (if it has not already failed for other
reasons).
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?
Switching immediately to a dead state is also possible, but it's nice
to try and keep the connection alive as long as we can with a nice
diagnosis of a failed CMD_READ but still allow further commands,
rather than an abrupt disconnect that takes out all other use of the
server.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization:
qemu.org |
libvirt.org