[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of

From: Max Reitz
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols
Date: Fri, 08 May 2015 14:58:58 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 08.05.2015 12:08, Kevin Wolf wrote:
Am 07.05.2015 um 16:50 hat Paolo Bonzini geschrieben:
On 07/05/2015 16:34, Kevin Wolf wrote:
Am 07.05.2015 um 16:16 hat Paolo Bonzini geschrieben:

On 07/05/2015 16:07, Kevin Wolf wrote:
This is not right for two reasons: The first is that this is
BlockBackend code
I think it would take effect for the qemu-nbd case though.
Oh, you want to change the server code rather than the client?
Actually, considering all the information in this thread, I'm inclined
that we should change both sides. qemu-nbd because ENOSPC might be what
clients expect by analogy with Linux block devices, even if the
behaviour for accesses beyond the device size isn't specified in the NBD
protocol and the server might just do anything. As long as the behaviour
is undefined, it's nice to do what most people may expect.

It is practically defined by what the reference implementation does, and that is return EINVAL (as I said in the other thread), with the reasoning being that it's an invalid request. I concur, the client should simply not send a request beyond the export length, doing so is wrong. So it is the client that should catch this case and return ENOSPC.

And as the real fix change the nbd client, because even if new qemu-nbd
versions will be nice, we shouldn't rely on undefined behaviour. We know
that old qemu-nbd servers won't produce ENOSPC and I'm not sure what
other NBD servers do.

Return EINVAL.


Wait... Are you saying that NBD sends a (platform specific) errno value
over the network? :-/
Yes. :/  That said, at least the error codes that Linux places in
/usr/include/asm/errno-base.h seem to be pretty much standard---at least
Windows and most Unices share them---with the exception of EAGAIN.

I'll send a patch to NBD to standardize the set of error codes that it
Thanks, that will be helpful in the future.

Is this the right place to look up the spec?

If so, the commands seem to be hopelessly underspecified, especially
with respect to error conditions. And where it says something about
errors, it doesn't make sense: The server is forbidden to reply to a
NBD_CMD_FLUSH if it failed... (qemu-nbd ignores this, obviously)

In theory, what error code the NBD server needs to send should be
specified by the NBD protocol. Am I right to assume that it doesn't do

In any case, I'm not sure whether qemu's internal error code
should change just for NBD. Producing the right error code for the
protocol is the job of nbd_co_receive_request().
Ok, so it shouldn't reach blk_check_request at all.  But then, we should
aim at making blk_check_request's checks assertions.
Sounds fair as a goal, but I don't think all devices have such checks
yet. We've fixed the most common devices (IDE, scsi-disk and virtio-blk)
just a while ago.

and it wouldn't even take effect for the qcow2 case
where we're writing past EOF only on the protocol layer. The second is
that -ENOSPC is only for writes and not for reads.
This is right.

Reads in the kernel return 0, but in QEMU we do not want that.  The code
currently returns -EIO, but perhaps -EINVAL is a better match.  It also
happens to be what Linux returns for discards.
Perhaps it is, yes. It shouldn't make a difference for guests anyway.
(Unlike -ENOSPC for writes, which would trigger werror=enospc! That's
most likely not what we want.)
Yes, we want the check duplicated in all BlockBackend users.  Most of
them already do it, see the work that Markus did last year I think.
I wouldn't call it duplicated because the action to take is different
for each device, but yes, the check belongs there.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]