Re: [Libguestfs] [nbdkit PATCH 1/4] server: Switch connection status to enum

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(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs