[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errn
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire |
Date: |
Fri, 08 May 2015 12:12:13 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Paolo Bonzini <address@hidden> writes:
> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.
>
> Luckily, most common error values are more or less universal: in
> particular, of all errno values <= 34 (up to ERANGE), they are all the
> same on supported platforms except for 11 (which is EAGAIN on Windows and
> Linux, but EDEADLK on Darwin and the *BSDs). So, in order to guarantee
> some portability, only keep a handful of possible error codes and squash
> everything else to EINVAL.
>
> This patch defines a limited set of errno values that are valid for the
> NBD protocol, and specifies recommendations for what error to return
> in specific corner cases. The set of errno values is roughly based on
> the errors listed in the read(2) and write(2) man pages, with some
> exceptions:
>
> - ENOMEM is added for servers that implement copy-on-write or other
> formats that require dynamic allocation.
>
> - EDQUOT is not part of the universal set of errors; it can be changed
> to ENOSPC on the wire format.
Makes sense.
> - EFBIG is part of the universal set of errors, but it is also changed
> to ENOSPC because it is pretty similar to ENOSPC or EDQUOT.
Perhaps debatable, but I defer to your judgement.
> Incoming values will in general match system errno values, but not
> on the Hurd which has different errno values (they have a "subsystem
> code" equal to 0x10 in bits 24-31). The Hurd is probably not something
> to which QEMU has been ported, but still do the right thing and
> reverse-map the NBD errno values to the system errno values.
>
> The corresponding patch to the NBD protocol description can be found at
> http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> v1->v2: use #defines and do not expect errnos to match
> NBD errnos for the sake of Hurd. Reduced the list
> of errno values even more. Better commit message
> explaining how the list was obtained.
>
> nbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/nbd.c b/nbd.c
> index cb1b9bb..57d71b2 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -86,6 +86,54 @@
> #define NBD_OPT_ABORT (2)
> #define NBD_OPT_LIST (3)
>
> +/* 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.
> + */
> +#define NBD_EPERM 1
> +#define NBD_EIO 5
> +#define NBD_ENOMEM 12
> +#define NBD_EINVAL 22
> +#define NBD_ENOSPC 28
> +
> +static int system_errno_to_nbd_errno(int err)
> +{
> + switch (err) {
> + case EPERM:
> + return NBD_EPERM;
> + case EIO:
> + return NBD_EIO;
> + case ENOMEM:
> + return NBD_ENOMEM;
> +#ifdef EDQUOT
> + case EDQUOT:
> +#endif
> + case EFBIG:
> + case ENOSPC:
> + return NBD_ENOSPC;
> + case EINVAL:
> + default:
> + return NBD_EINVAL;
> + }
> +}
> +
> +static int nbd_errno_to_system_errno(int err)
> +{
> + switch (err) {
> + case NBD_EPERM:
> + return EPERM;
> + case NBD_EIO:
> + return EIO;
> + case NBD_ENOMEM:
> + return ENOMEM;
> + case NBD_ENOSPC:
> + return ENOSPC;
> + case NBD_EINVAL:
> + default:
> + return EINVAL;
> + }
> +}
> +
If we reach default, something's amiss, isn't it? Worth logging
something then?
> /* Definitions for opaque data types */
>
> typedef struct NBDRequest NBDRequest;
> @@ -856,6 +904,8 @@ 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));
>
> + reply->error = nbd_errno_to_system_errno(reply->error);
> +
> TRACE("Got reply: "
> "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }",
> magic, reply->error, reply->handle);
> @@ -872,6 +922,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)
Reviewed-by: Markus Armbruster <address@hidden>