From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35654) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yqelx-0002TA-91 for qemu-devel@nongnu.org; Fri, 08 May 2015 05:40:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yqelt-0002YY-Md for qemu-devel@nongnu.org; Fri, 08 May 2015 05:40:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34931) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yqelt-0002YT-Ek for qemu-devel@nongnu.org; Fri, 08 May 2015 05:40:41 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id DD51CAC7D5 for ; Fri, 8 May 2015 09:40:40 +0000 (UTC) Message-ID: <554C8494.7070609@redhat.com> Date: Fri, 08 May 2015 11:40:36 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1431012374-14113-1-git-send-email-pbonzini@redhat.com> <1431012374-14113-2-git-send-email-pbonzini@redhat.com> <87a8xfr2ig.fsf@blackfin.pond.sub.org> <554C740A.9050705@redhat.com> <87ioc3ifdu.fsf@blackfin.pond.sub.org> In-Reply-To: <87ioc3ifdu.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com On 08/05/2015 11:32, Markus Armbruster wrote: >>>> +/* NBD errors are based on errno numbers, so there is a 1:1 mapping, >>>> + * but only a limited set of errno values is specified in the protocol. >>>> + * Everything else is squashed to EINVAL. >>>> + */ >>> >>> Is the protocol defined anywhere? > > https://github.com/yoe/nbd/blob/master/doc/proto.txt > > All it has on the error code is this paragraph: > > The reply contains three fields: a 32 bit magic number ('magic'), a > 32 bit error code ('error'; 0, unless an error occurred in which > case it is the errno of the error on the server side), and the same > 64 bit handle that the corresponding request had in its 'handle' > field. In case the reply is sent in response to a read request and > the error field is 0 (zero), the reply header is immediately > followed by request.len bytes of data. > > Could you update it to document the errno compatibility issues, and > recommended practice (i.e. this patch's)? Yes, I've sent a patch yesterday. >>>> +static int system_errno_to_nbd_errno(int err) >>>> +{ >>>> + switch (err) { >>>> + case EPERM: >>>> + return 1; >>>> + case EIO: >>>> + return 5; >>>> + case ENXIO: >>>> + return 6; >>>> + case E2BIG: >>>> + return 7; >>>> + case ENOMEM: >>>> + return 12; >>>> + case EACCES: >>>> + return 13; >>>> + case EFBIG: >>>> + return 27; >>>> + case ENOSPC: >>>> + return 28; >>>> + case EROFS: >>>> + return 30; >>>> + case EINVAL: >>>> + default: >>>> + return 22; >>>> + } >>>> +} >>>> + >>> >>> This maps recognized OS errnos to NBD errnos. The latter are literals. >>> >>>> /* Definitions for opaque data types */ >>>> >>>> typedef struct NBDRequest NBDRequest; >>>> @@ -856,6 +887,20 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply) >>>> reply->error = be32_to_cpup((uint32_t*)(buf + 4)); >>>> reply->handle = be64_to_cpup((uint64_t*)(buf + 8)); >>>> >>>> + /* NBD errors should be universally equal to the corresponding >>>> + * errno values, check it here. >>>> + */ >>>> + QEMU_BUILD_BUG_ON(EPERM != 1); >>>> + QEMU_BUILD_BUG_ON(EIO != 5); >>>> + QEMU_BUILD_BUG_ON(ENXIO != 6); >>>> + QEMU_BUILD_BUG_ON(E2BIG != 7); >>>> + QEMU_BUILD_BUG_ON(ENOMEM != 12); >>>> + QEMU_BUILD_BUG_ON(EACCES != 13); >>>> + QEMU_BUILD_BUG_ON(EINVAL != 22); >>>> + QEMU_BUILD_BUG_ON(EFBIG != 27); >>>> + QEMU_BUILD_BUG_ON(ENOSPC != 28); >>>> + QEMU_BUILD_BUG_ON(EROFS != 30); >>>> + >>> >>> This checks that the mapping above is the identify function for all the >>> recognized NBD errnos. Why is that necessary? > > Still curious. Explain, and earn my R-by :) Because block/nbd.c expects host errnos, and I was too lazy to add a switch and a nbd_errno_to_system_errno function. But Eric pointed out that Hurd has weird errnos (the low bits match, but there's a 0x10 subsystem code in bits 24-31) so I'll add it. >>> Same literals as above. Violates DRY. I don't mind all that much, but >>> wonder whether we could at least do the checking next to >>> system_errno_to_nbd_errno(). > > Could we? Yes, s/could/should/. Also, should have added RFC to the patch. Paolo >>>> TRACE("Got reply: " >>>> "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }", >>>> magic, reply->error, reply->handle); >>>> @@ -872,6 +917,8 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply) >>>> uint8_t buf[NBD_REPLY_SIZE]; >>>> ssize_t ret; >>>> >>>> + reply->error = system_errno_to_nbd_errno(reply->error); >>>> + >>>> /* Reply >>>> [ 0 .. 3] magic (NBD_REPLY_MAGIC) >>>> [ 4 .. 7] error (0 == no error)