5:17 p.m.
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