On Wed, Oct 26, 2022 at 05:17:59PM -0500, Eric Blake wrote:
> Instead of using magic numbers, track connection status via an enum.
> While at it, convert functions whose return value is not used to
> instead be void. No semantic change.
> ---
> server/internal.h | 16 +++++--
> server/connections.c | 29 ++++++------
> server/protocol.c | 107 ++++++++++++++++++++++---------------------
> server/public.c | 3 +-
> server/test-public.c | 4 +-
> 5 files changed, 84 insertions(+), 75 deletions(-)
>
> diff --git a/server/internal.h b/server/internal.h
> index ed52c1bf..fa658364 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2013-2021 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -232,13 +232,19 @@ struct context {
> int can_cache;
> };
>
> +typedef enum {
> + STATUS_DEAD, /* Connection is closed */
> + STATUS_CLIENT_DONE, /* Client has sent NBD_CMD_DISC */
> + STATUS_ACTIVE, /* Client can make requests */
> +} conn_status;
> +
> struct connection {
> pthread_mutex_t request_lock;
> pthread_mutex_t read_lock;
> pthread_mutex_t write_lock;
> pthread_mutex_t status_lock;
>
> - int status; /* 1 for more I/O with client, 0 for shutdown, -1 on error */
> + conn_status status;
> int status_pipe[2]; /* track status changes via poll when nworkers > 1 */
> void *crypto_session;
> int nworkers;
> @@ -264,8 +270,8 @@ struct connection {
> };
>
> extern void handle_single_connection (int sockin, int sockout);
> -extern int connection_get_status (void);
> -extern int connection_set_status (int value);
> +extern conn_status connection_get_status (void);
> +extern void connection_set_status (conn_status value);
>
> /* protocol-handshake.c */
> extern int protocol_handshake (void);
> @@ -280,7 +286,7 @@ extern int protocol_handshake_oldstyle (void);
> extern int protocol_handshake_newstyle (void);
>
> /* protocol.c */
> -extern int protocol_recv_request_send_reply (void);
> +extern void protocol_recv_request_send_reply (void);
>
> /* The context ID of base:allocation. As far as I can tell it doesn't
> * matter what this is as long as nbdkit always returns the same
> diff --git a/server/connections.c b/server/connections.c
> index 29e4cd58..27177d70 100644
> --- a/server/connections.c
> +++ b/server/connections.c
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2013-2021 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -64,11 +64,11 @@ static int raw_send_other (const void *buf, size_t len,
> int flags);
> #endif
> static void raw_close (void);
>
> -int
> +conn_status
> connection_get_status (void)
> {
> GET_CONN;
> - int r;
> + conn_status r;
>
> if (conn->nworkers &&
> pthread_mutex_lock (&conn->status_lock))
> @@ -80,11 +80,9 @@ connection_get_status (void)
> return r;
> }
>
> -/* Update the status if the new value is lower than the existing value.
> - * For convenience, return the incoming value.
> - */
> -int
> -connection_set_status (int value)
> +/* Update the status if the new value is lower than the existing value. */
> +void
> +connection_set_status (conn_status value)
> {
> GET_CONN;
>
> @@ -92,7 +90,7 @@ connection_set_status (int value)
> pthread_mutex_lock (&conn->status_lock))
> abort ();
> if (value < conn->status) {
> - if (conn->nworkers && conn->status > 0) {
> + if (conn->nworkers && conn->status > STATUS_CLIENT_DONE) {
> char c = 0;
>
> assert (conn->status_pipe[1] >= 0);
> @@ -104,7 +102,6 @@ connection_set_status (int value)
> if (conn->nworkers &&
> pthread_mutex_unlock (&conn->status_lock))
> abort ();
> - return value;
> }
>
> struct worker_data {
> @@ -125,7 +122,7 @@ connection_worker (void *data)
> threadlocal_set_conn (conn);
> free (worker);
>
> - while (!quit && connection_get_status () > 0)
> + while (!quit && connection_get_status () > STATUS_CLIENT_DONE)
> protocol_recv_request_send_reply ();
> debug ("exiting worker thread %s", threadlocal_get_name ());
> free (name);
> @@ -177,7 +174,7 @@ handle_single_connection (int sockin, int sockout)
> if (!nworkers) {
> /* No need for a separate thread. */
> debug ("handshake complete, processing requests serially");
> - while (!quit && connection_get_status () > 0)
> + while (!quit && connection_get_status () > STATUS_CLIENT_DONE)
> protocol_recv_request_send_reply ();
> }
> else {
> @@ -196,13 +193,13 @@ handle_single_connection (int sockin, int sockout)
>
> if (unlikely (!worker)) {
> perror ("malloc");
> - connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> goto wait;
> }
> if (unlikely (asprintf (&worker->name, "%s.%d", plugin_name, nworkers)
> < 0)) {
> perror ("asprintf");
> - connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> free (worker);
> goto wait;
> }
> @@ -212,7 +209,7 @@ handle_single_connection (int sockin, int sockout)
> if (unlikely (err)) {
> errno = err;
> perror ("pthread_create");
> - connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> free (worker);
> goto wait;
> }
> @@ -264,7 +261,7 @@ new_connection (int sockin, int sockout, int nworkers)
> goto error1;
> }
>
> - conn->status = 1;
> + conn->status = STATUS_ACTIVE;
> conn->nworkers = nworkers;
> if (nworkers) {
> #ifdef HAVE_PIPE2
> diff --git a/server/protocol.c b/server/protocol.c
> index 2ac77055..d1e01502 100644
> --- a/server/protocol.c
> +++ b/server/protocol.c
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2013-2021 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -362,7 +362,7 @@ nbd_errno (int error, uint16_t flags)
> }
> }
>
> -static int
> +static void
> send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags,
> const char *buf, uint32_t count,
> uint32_t error)
> @@ -380,7 +380,8 @@ send_simple_reply (uint64_t handle, uint16_t cmd,
> uint16_t flags,
> r = conn->send (&reply, sizeof reply, f);
> if (r == -1) {
> nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> /* Send the read data buffer. */
> @@ -388,14 +389,12 @@ send_simple_reply (uint64_t handle, uint16_t cmd,
> uint16_t flags,
> r = conn->send (buf, count, 0);
> if (r == -1) {
> nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> }
> }
> -
> - return 1; /* command processed ok */
> }
>
> -static int
> +static void
> send_structured_reply_read (uint64_t handle, uint16_t cmd,
> const char *buf, uint32_t count, uint64_t offset)
> {
> @@ -421,7 +420,8 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd,
> r = conn->send (&reply, sizeof reply, SEND_MORE);
> if (r == -1) {
> nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> /* Send the offset + read data buffer. */
> @@ -429,16 +429,15 @@ send_structured_reply_read (uint64_t handle, uint16_t
> cmd,
> r = conn->send (&offset_data, sizeof offset_data, SEND_MORE);
> if (r == -1) {
> nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> r = conn->send (buf, count, 0);
> if (r == -1) {
> nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> }
> -
> - return 1; /* command processed ok */
> }
>
> /* Convert a list of extents into NBD_REPLY_TYPE_BLOCK_STATUS blocks.
> @@ -523,7 +522,7 @@ extents_to_block_descriptors (struct nbdkit_extents
> *extents,
> return blocks;
> }
>
> -static int
> +static void
> send_structured_reply_block_status (uint64_t handle,
> uint16_t cmd, uint16_t flags,
> uint32_t count, uint64_t offset,
> @@ -543,8 +542,10 @@ send_structured_reply_block_status (uint64_t handle,
>
> blocks = extents_to_block_descriptors (extents, flags, count, offset,
> &nr_blocks);
> - if (blocks == NULL)
> - return connection_set_status (-1);
> + if (blocks == NULL) {
> + connection_set_status (STATUS_DEAD);
> + return;
> + }
>
> reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC);
> reply.handle = handle;
> @@ -556,7 +557,8 @@ send_structured_reply_block_status (uint64_t handle,
> r = conn->send (&reply, sizeof reply, SEND_MORE);
> if (r == -1) {
> nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> /* Send the base:allocation context ID. */
> @@ -564,7 +566,8 @@ send_structured_reply_block_status (uint64_t handle,
> r = conn->send (&context_id, sizeof context_id, SEND_MORE);
> if (r == -1) {
> nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> /* Send each block descriptor. */
> @@ -573,14 +576,12 @@ send_structured_reply_block_status (uint64_t handle,
> i == nr_blocks - 1 ? 0 : SEND_MORE);
> if (r == -1) {
> nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> }
> }
> -
> - return 1; /* command processed ok */
> }
>
> -static int
> +static void
> send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags,
> uint32_t error)
> {
> @@ -599,7 +600,8 @@ send_structured_reply_error (uint64_t handle, uint16_t
> cmd, uint16_t flags,
> r = conn->send (&reply, sizeof reply, SEND_MORE);
> if (r == -1) {
> nbdkit_error ("write error reply: %m");
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> /* Send the error. */
> @@ -608,18 +610,17 @@ send_structured_reply_error (uint64_t handle, uint16_t
> cmd, uint16_t flags,
> r = conn->send (&error_data, sizeof error_data, 0);
> if (r == -1) {
> nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> }
> /* No human readable error message at the moment. */
> -
> - return 1; /* command processed ok */
> }
>
> -int
> +void
> protocol_recv_request_send_reply (void)
> {
> GET_CONN;
> int r;
> + conn_status cs;
> struct nbd_request request;
> uint16_t cmd, flags;
> uint32_t magic, count, error = 0;
> @@ -630,24 +631,27 @@ protocol_recv_request_send_reply (void)
> /* Read the request packet. */
> {
> ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->read_lock);
> - r = connection_get_status ();
> - if (r <= 0)
> - return r;
> + cs = connection_get_status ();
> + if (cs <= STATUS_CLIENT_DONE)
> + return;
> r = conn->recv (&request, sizeof request);
> if (r == -1) {
> nbdkit_error ("read request: %m");
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
> if (r == 0) {
> debug ("client closed input socket, closing connection");
> - return connection_set_status (0); /* disconnect */
> + connection_set_status (STATUS_CLIENT_DONE); /* disconnect */
> + return;
> }
>
> magic = be32toh (request.magic);
> if (magic != NBD_REQUEST_MAGIC) {
> nbdkit_error ("invalid request: 'magic' field is incorrect (0x%x)",
> magic);
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
>
> flags = be16toh (request.flags);
> @@ -658,14 +662,17 @@ protocol_recv_request_send_reply (void)
>
> if (cmd == NBD_CMD_DISC) {
> debug ("client sent %s, closing connection", name_of_nbd_cmd (cmd));
> - return connection_set_status (0); /* disconnect */
> + connection_set_status (STATUS_CLIENT_DONE); /* disconnect */
> + return;
> }
>
> /* Validate the request. */
> if (!validate_request (cmd, flags, offset, count, &error)) {
> if (cmd == NBD_CMD_WRITE &&
> - skip_over_write_buffer (conn->sockin, count) < 0)
> - return connection_set_status (-1);
> + skip_over_write_buffer (conn->sockin, count) < 0) {
> + connection_set_status (STATUS_DEAD);
> + return;
> + }
> goto send_reply;
> }
>
> @@ -677,8 +684,10 @@ protocol_recv_request_send_reply (void)
> if (buf == NULL) {
> error = ENOMEM;
> if (cmd == NBD_CMD_WRITE &&
> - skip_over_write_buffer (conn->sockin, count) < 0)
> - return connection_set_status (-1);
> + skip_over_write_buffer (conn->sockin, count) < 0) {
> + connection_set_status (STATUS_DEAD);
> + return;
> + }
> goto send_reply;
> }
> }
> @@ -702,13 +711,14 @@ protocol_recv_request_send_reply (void)
> }
> if (r == -1) {
> nbdkit_error ("read data: %s: %m", name_of_nbd_cmd (cmd));
> - return connection_set_status (-1);
> + connection_set_status (STATUS_DEAD);
> + return;
> }
> }
> }
>
> /* Perform the request. Only this part happens inside the request lock. */
> - if (quit || !connection_get_status ()) {
> + if (quit || connection_get_status () == STATUS_CLIENT_DONE) {
> error = ESHUTDOWN;
> }
> else {
> @@ -720,8 +730,8 @@ protocol_recv_request_send_reply (void)
>
> /* Send the reply packet. */
> send_reply:
> - if (connection_get_status () < 0)
> - return -1;
> + if (connection_get_status () < STATUS_CLIENT_DONE)
> + return;
>
> if (error != 0) {
> /* Since we're about to send only the limited NBD_E* errno to the
> @@ -742,19 +752,14 @@ protocol_recv_request_send_reply (void)
> (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) {
> if (!error) {
> if (cmd == NBD_CMD_READ)
> - return send_structured_reply_read (request.handle, cmd,
> - buf, count, offset);
> + send_structured_reply_read (request.handle, cmd, buf, count, offset);
> else /* NBD_CMD_BLOCK_STATUS */
> - return send_structured_reply_block_status (request.handle,
> - cmd, flags,
> - count, offset,
> - extents);
> + send_structured_reply_block_status (request.handle, cmd, flags,
> + count, offset, extents);
> }
> else
> - return send_structured_reply_error (request.handle, cmd, flags,
> - error);
> + send_structured_reply_error (request.handle, cmd, flags, error);
> }
> else
> - return send_simple_reply (request.handle, cmd, flags, buf, count,
> - error);
> + send_simple_reply (request.handle, cmd, flags, buf, count, error);
> }
> diff --git a/server/public.c b/server/public.c
> index 472ca623..6a9840bb 100644
> --- a/server/public.c
> +++ b/server/public.c
> @@ -727,7 +727,8 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
> */
> bool has_quit = quit;
> assert (has_quit ||
> - (conn && conn->nworkers > 0 && connection_get_status () < 1) ||
> + (conn && conn->nworkers > 0 &&
> + connection_get_status () < STATUS_ACTIVE) ||
> (conn && (fds[2].revents & (POLLRDHUP | POLLHUP | POLLERR |
> POLLNVAL))));
> if (has_quit)
> diff --git a/server/test-public.c b/server/test-public.c
> index 1cb759d1..1d83354f 100644
> --- a/server/test-public.c
> +++ b/server/test-public.c
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2018-2021 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -83,7 +83,7 @@ threadlocal_get_context (void)
> abort ();
> }
>
> -int
> +conn_status
> connection_get_status (void)
> {
> abort ();
> --
> 2.37.3
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
