From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yqhrx-00070w-Ew for qemu-devel@nongnu.org; Fri, 08 May 2015 08:59:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yqhrw-0005he-0c for qemu-devel@nongnu.org; Fri, 08 May 2015 08:59:09 -0400 Message-ID: <554CB312.1020607@redhat.com> Date: Fri, 08 May 2015 14:58:58 +0200 From: Max Reitz MIME-Version: 1.0 References: <554A4E2D.4050300@redhat.com> <554B58A8.6000203@redhat.com> <20150507122911.GB4571@noname.redhat.com> <554B5ED3.4030405@redhat.com> <20150507132056.GC4571@noname.redhat.com> <554B6EBB.1010001@redhat.com> <20150507140716.GE4571@noname.redhat.com> <554B73C2.4030908@redhat.com> <20150507143418.GF4571@noname.redhat.com> <554B7BBC.2040508@redhat.com> <20150508100832.GC4318@noname.redhat.com> In-Reply-To: <20150508100832.GC4318@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] block: Warn about usage of growing formats over non-growable protocols List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , Paolo Bonzini Cc: Stefan Hajnoczi , qemu-block@nongnu.org, qemu-devel@nongnu.org, Stefan Hajnoczi 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? >> Yes. > 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. Max >>> 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 >> sends. > Thanks, that will be helpful in the future. > > Is this the right place to look up the spec? > http://sourceforge.net/p/nbd/code/ci/master/tree/doc/proto.txt > > 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 >>> that? >> Nope. >> >>> 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. > > Kevin