[Libguestfs] [nbdkit PATCH] connections: Improve error responses
Richard W.M. Jones
rjones at redhat.com
Thu Nov 16 11:47:08 UTC 2017
On Wed, Nov 15, 2017 at 04:16:49PM -0600, Eric Blake wrote:
> We had several inconsistencies from the NBD spec when diagnosing
> bad client messages:
> - FLUSH is not generally forbidden on a read-only export (so failing
> with EPERM is wrong) [meanwhile, if we don't advertise flush because
> plugin_can_flush() fails, then rejecting with EINVAL is still okay]
> - returning EPERM (aka EROFS) for read-only exports should probably
> take precedence over anything else
> - out-of-bounds WRITE and WRITE_ZEROES should fail with ENOSPC,
> not EIO
> - out-of-bounds READ and TRIM should fail with EINVAL, not EIO
>
> We also had dead code: valid_range() and validate_request() cannot
> return -1. Make these functions return bool instead. And finally,
> fix a comment typo.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> src/connections.c | 53 ++++++++++++++++++++++++-----------------------------
> 1 file changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/src/connections.c b/src/connections.c
> index 8dc1925..264037d 100644
> --- a/src/connections.c
> +++ b/src/connections.c
> @@ -666,7 +666,7 @@ negotiate_handshake (struct connection *conn)
> return r;
> }
>
> -static int
> +static bool
> valid_range (struct connection *conn, uint64_t offset, uint32_t count)
> {
> uint64_t exportsize = conn->exportsize;
> @@ -674,27 +674,34 @@ valid_range (struct connection *conn, uint64_t offset, uint32_t count)
> return count > 0 && offset <= exportsize && offset + count <= exportsize;
> }
>
> -static int
> +static bool
> validate_request (struct connection *conn,
> uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t count,
> uint32_t *error)
> {
> int r;
>
> + /* Readonly connection? */
> + if (conn->readonly &&
> + (cmd == NBD_CMD_WRITE ||
> + cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) {
> + nbdkit_error ("invalid request: write request on readonly connection");
> + *error = EROFS;
> + return false;
> + }
> +
> /* Validate cmd, offset, count. */
> switch (cmd) {
> case NBD_CMD_READ:
> case NBD_CMD_WRITE:
> case NBD_CMD_TRIM:
> case NBD_CMD_WRITE_ZEROES:
> - r = valid_range (conn, offset, count);
> - if (r == -1)
> - return -1;
> - if (r == 0) {
> + if (!valid_range (conn, offset, count)) {
> /* XXX Allow writes to extend the disk? */
> nbdkit_error ("invalid request: offset and length are out of range");
> - *error = EIO;
> - return 0;
> + *error = (cmd == NBD_CMD_WRITE ||
> + cmd == NBD_CMD_WRITE_ZEROES) ? ENOSPC : EINVAL;
> + return false;
> }
> break;
>
> @@ -702,7 +709,7 @@ validate_request (struct connection *conn,
> if (offset != 0 || count != 0) {
> nbdkit_error ("invalid flush request: expecting offset and length == 0");
> *error = EINVAL;
> - return 0;
> + return false;
> }
> break;
>
> @@ -710,20 +717,20 @@ validate_request (struct connection *conn,
> nbdkit_error ("invalid request: unknown command (%" PRIu32 ") ignored",
> cmd);
> *error = EINVAL;
> - return 0;
> + return false;
> }
>
> /* Validate flags */
> if (flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
> nbdkit_error ("invalid request: unknown flag (0x%x)", flags);
> *error = EINVAL;
> - return 0;
> + return false;
> }
> if ((flags & NBD_CMD_FLAG_NO_HOLE) &&
> cmd != NBD_CMD_WRITE_ZEROES) {
> nbdkit_error ("invalid request: NO_HOLE flag needs WRITE_ZEROES request");
> *error = EINVAL;
> - return 0;
> + return false;
> }
>
> /* Refuse over-large read and write requests. */
> @@ -732,33 +739,24 @@ validate_request (struct connection *conn,
> nbdkit_error ("invalid request: data request is too large (%" PRIu32
> " > %d)", count, MAX_REQUEST_SIZE);
> *error = ENOMEM;
> - return 0;
> - }
> -
> - /* Readonly connection? */
> - if (conn->readonly &&
> - (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_FLUSH ||
> - cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) {
> - nbdkit_error ("invalid request: write request on readonly connection");
> - *error = EROFS;
> - return 0;
> + return false;
> }
>
> /* Flush allowed? */
> if (!conn->can_flush && cmd == NBD_CMD_FLUSH) {
> nbdkit_error ("invalid request: flush operation not supported");
> *error = EINVAL;
> - return 0;
> + return false;
> }
>
> /* Trim allowed? */
> if (!conn->can_trim && cmd == NBD_CMD_TRIM) {
> nbdkit_error ("invalid request: trim operation not supported");
> *error = EINVAL;
> - return 0;
> + return false;
> }
>
> - return 1; /* Commands validates. */
> + return true; /* Command validates. */
> }
>
> /* Grab the appropriate error value.
> @@ -970,10 +968,7 @@ recv_request_send_reply (struct connection *conn)
> }
>
> /* Validate the request. */
> - r = validate_request (conn, cmd, flags, offset, count, &error);
> - if (r == -1)
> - return -1;
> - if (r == 0) { /* request not valid */
> + if (!validate_request (conn, cmd, flags, offset, count, &error)) {
> if (cmd == NBD_CMD_WRITE &&
> skip_over_write_buffer (conn->sockin, count) < 0)
> return -1;
ACK.
I added you to the "collaborators" section of the project, so
hopefully you should be able to push this.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
More information about the Libguestfs
mailing list