| Message ID | 1475578981-24126-1-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 04, 2016 at 02:03:01PM +0300, Denis V. Lunev wrote: > From: Denis Plotnikov <dplotnikov@virtuozzo.com> > > The NBD server socket was created by qemu-nbd code. This could lead to the > race issue when the management layer started qemu-nbd server and allowed > a client to connect to the server, the client tried to connect to the > server but failed because qemu-nbd had not managed to open the listening > server socket by the time the client has finished its trying to connect. > > Creating a listening socket before starting of qemu-ndb and passing the > socket fd to be used as the server socket to qemu-nbd as an option solves > this issue. > > It also helps to resolve the situation when qemu-nbd started by some > management software should report the port number qemu-nbd bound to, to > another piece of software. Right now, there is no good way to do that > except to start the qemu-nbd to make sure that the certain port has been > actually aquired. Otherwise, it is definitely racy to try to define the > port first and then start the qemu-nbd, hoping that the port defined is > still available. Passing of the pre-created file descriptor resolves this > situation as well. > > As a plus, pre-creating of the server socket adds some degree of freedom in > setting of the socket properties. It is so, because once qemu-nbd gets > the socket fd, it starts using it as is, without any additional > modifications. > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Paolo Bonzini <pbonzini@redhat.com> > --- > qemu-nbd.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++-------- > qemu-nbd.texi | 6 ++++ > 2 files changed, 89 insertions(+), 12 deletions(-) > > Changes from v1: > - commit message improvements > - Eric's style nits applied It seems you've ignored the suggestion to implement the systemd socket activation protocol, which would avoid the need for any new command line parameters, and would let qemu-nbd "just work" with systemd units too. Regards, Daniel
On 10/04/2016 02:03 PM, Denis V. Lunev wrote: > From: Denis Plotnikov <dplotnikov@virtuozzo.com> > > The NBD server socket was created by qemu-nbd code. This could lead to the > race issue when the management layer started qemu-nbd server and allowed > a client to connect to the server, the client tried to connect to the > server but failed because qemu-nbd had not managed to open the listening > server socket by the time the client has finished its trying to connect. > > Creating a listening socket before starting of qemu-ndb and passing the > socket fd to be used as the server socket to qemu-nbd as an option solves > this issue. > > It also helps to resolve the situation when qemu-nbd started by some > management software should report the port number qemu-nbd bound to, to > another piece of software. Right now, there is no good way to do that > except to start the qemu-nbd to make sure that the certain port has been > actually aquired. Otherwise, it is definitely racy to try to define the > port first and then start the qemu-nbd, hoping that the port defined is > still available. Passing of the pre-created file descriptor resolves this > situation as well. > > As a plus, pre-creating of the server socket adds some degree of freedom in > setting of the socket properties. It is so, because once qemu-nbd gets > the socket fd, it starts using it as is, without any additional > modifications. > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Paolo Bonzini <pbonzini@redhat.com> > --- > qemu-nbd.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++-------- > qemu-nbd.texi | 6 ++++ > 2 files changed, 89 insertions(+), 12 deletions(-) > > Changes from v1: > - commit message improvements > - Eric's style nits applied > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 99297a5..3b6319e 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -48,9 +48,13 @@ > #define QEMU_NBD_OPT_OBJECT 260 > #define QEMU_NBD_OPT_TLSCREDS 261 > #define QEMU_NBD_OPT_IMAGE_OPTS 262 > +#define QEMU_NBD_OPT_SRV_SOCKET_FD 263 > > #define MBR_SIZE 512 > > +#define STR(s) _STR(s) > +#define _STR(s) #s > + > static NBDExport *exp; > static bool newproto; > static int verbose; > @@ -78,6 +82,8 @@ static void usage(const char *name) > " -b, --bind=IFACE interface to bind to (default `0.0.0.0')\n" > " -k, --socket=PATH path to the unix socket\n" > " (default '"SOCKET_PATH"')\n" > +" --server-sock-fd=NUM pre-opened listening server socket\n" > +" file descriptor\n" > " -e, --shared=NUM device can be shared by NUM clients (default '1')\n" > " -t, --persistent don't exit on the last connection\n" > " -v, --verbose display extra debugging information\n" > @@ -382,9 +388,8 @@ static void nbd_update_server_watch(void) > } > > > -static SocketAddress *nbd_build_socket_address(const char *sockpath, > - const char *bindto, > - const char *port) > +static SocketAddress *nbd_build_sock_fd(const char *sockpath, > + const char *bindto, const char *port) > { > SocketAddress *saddr; > > @@ -459,6 +464,41 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) > return creds; > } > > +static void setup_address_and_port(const char **address, const char **port) > +{ > + if (*address == NULL) { > + *address = "0.0.0.0"; > + } > + > + if (*port == NULL) { > + *port = STR(NBD_DEFAULT_PORT); > + } > +} > + > +/* > ++ * Check socket parameters compatibility when sock_fd is given > ++ */ > +static const char *sock_fd_validate_opts(char *device, char *sockpath, > + const char *address, const char *port) > +{ > + if (device != NULL) { > + return "NBD device can't be set when socket fd is specified"; > + } > + > + if (sockpath != NULL) { > + return "Unix socket can't be set when socket fd is specified"; > + } > + > + if (address != NULL) { > + return "The interface can't be set when socket fd is specified"; > + } > + > + if (port != NULL) { > + return "TCP port number can't be set when socket fd is specified"; > + } > + > + return NULL; > +} > > int main(int argc, char **argv) > { > @@ -467,7 +507,7 @@ int main(int argc, char **argv) > off_t dev_offset = 0; > uint16_t nbdflags = 0; > bool disconnect = false; > - const char *bindto = "0.0.0.0"; > + const char *bindto = NULL; > const char *port = NULL; > char *sockpath = NULL; > char *device = NULL; > @@ -503,6 +543,8 @@ int main(int argc, char **argv) > { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, > { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, > { "trace", required_argument, NULL, 'T' }, > + { "server-sock-fd", required_argument, NULL, > + QEMU_NBD_OPT_SRV_SOCKET_FD }, > { NULL, 0, NULL, 0 } > }; > int ch; > @@ -524,6 +566,7 @@ int main(int argc, char **argv) > bool imageOpts = false; > bool writethrough = true; > char *trace_file = NULL; > + int sock_fd = -1; > > /* The client thread uses SIGTERM to interrupt the server. A signal > * handler ensures that "qemu-nbd -v -c" exits with a nice status code. > @@ -714,6 +757,15 @@ int main(int argc, char **argv) > g_free(trace_file); > trace_file = trace_opt_parse(optarg); > break; > + case QEMU_NBD_OPT_SRV_SOCKET_FD: > + sock_fd = qemu_parse_fd(optarg); > + > + if (sock_fd < 0) { > + error_report("Invalid socket fd value: " > + "it must be a non-negative integer"); > + exit(EXIT_FAILURE); > + } > + break; > } > } > > @@ -735,6 +787,17 @@ int main(int argc, char **argv) > trace_init_file(trace_file); > qemu_set_log(LOG_TRACE); > > + if (sock_fd != -1) { > + const char *err_msg = sock_fd_validate_opts(device, sockpath, > + bindto, port); > + if (err_msg != NULL) { > + error_report("%s", err_msg); > + exit(EXIT_FAILURE); > + } > + } else { > + setup_address_and_port(&bindto, &port); > + } > + > if (tlscredsid) { > if (sockpath) { > error_report("TLS is only supported with IPv4/IPv6"); > @@ -838,7 +901,22 @@ int main(int argc, char **argv) > snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > } > > - saddr = nbd_build_socket_address(sockpath, bindto, port); > + if (sock_fd == -1) { > + server_ioc = qio_channel_socket_new(); > + saddr = nbd_build_sock_fd(sockpath, bindto, port); > + if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) { > + object_unref(OBJECT(server_ioc)); > + error_report_err(local_err); > + return 1; > + } > + } else { > + server_ioc = qio_channel_socket_new_fd(sock_fd, &local_err); > + if (server_ioc == NULL) { > + error_report("Failed to use the given server socket fd: %s", > + error_get_pretty(local_err)); > + exit(EXIT_FAILURE); > + } > + } > > if (qemu_init_main_loop(&local_err)) { > error_report_err(local_err); > @@ -921,13 +999,6 @@ int main(int argc, char **argv) > newproto = true; > } > > - server_ioc = qio_channel_socket_new(); > - if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) { > - object_unref(OBJECT(server_ioc)); > - error_report_err(local_err); > - return 1; > - } > - > if (device) { > int ret; > > diff --git a/qemu-nbd.texi b/qemu-nbd.texi > index 91ebf04..4c9ea00 100644 > --- a/qemu-nbd.texi > +++ b/qemu-nbd.texi > @@ -34,6 +34,12 @@ The offset into the image > The interface to bind to (default @samp{0.0.0.0}) > @item -k, --socket=@var{path} > Use a unix socket with path @var{path} > +@item --server-sock-fd=@var{fd_num} > +A server socket fd to be used for client connections. > +The socket fd must be configured and switched to listening > +state before passing to the program. The program uses > +the given socket fd as is, without any additional > +modifications. > @item --image-opts > Treat @var{filename} as a set of image options, instead of a plain > filename. If this flag is specified, the @var{-f} flag should pls disregard
On 10/04/2016 02:09 PM, Daniel P. Berrange wrote: > On Tue, Oct 04, 2016 at 02:03:01PM +0300, Denis V. Lunev wrote: >> From: Denis Plotnikov <dplotnikov@virtuozzo.com> >> >> The NBD server socket was created by qemu-nbd code. This could lead to the >> race issue when the management layer started qemu-nbd server and allowed >> a client to connect to the server, the client tried to connect to the >> server but failed because qemu-nbd had not managed to open the listening >> server socket by the time the client has finished its trying to connect. >> >> Creating a listening socket before starting of qemu-ndb and passing the >> socket fd to be used as the server socket to qemu-nbd as an option solves >> this issue. >> >> It also helps to resolve the situation when qemu-nbd started by some >> management software should report the port number qemu-nbd bound to, to >> another piece of software. Right now, there is no good way to do that >> except to start the qemu-nbd to make sure that the certain port has been >> actually aquired. Otherwise, it is definitely racy to try to define the >> port first and then start the qemu-nbd, hoping that the port defined is >> still available. Passing of the pre-created file descriptor resolves this >> situation as well. >> >> As a plus, pre-creating of the server socket adds some degree of freedom in >> setting of the socket properties. It is so, because once qemu-nbd gets >> the socket fd, it starts using it as is, without any additional >> modifications. >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Paolo Bonzini <pbonzini@redhat.com> >> --- >> qemu-nbd.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++-------- >> qemu-nbd.texi | 6 ++++ >> 2 files changed, 89 insertions(+), 12 deletions(-) >> >> Changes from v1: >> - commit message improvements >> - Eric's style nits applied > It seems you've ignored the suggestion to implement the systemd socket > activation protocol, which would avoid the need for any new command > line parameters, and would let qemu-nbd "just work" with systemd units > too. > > > Regards, > Daniel thank you very much. I have missed that letter. Pls disregard this sending. ya, we will start over ;) Den
diff --git a/qemu-nbd.c b/qemu-nbd.c index 99297a5..3b6319e 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -48,9 +48,13 @@ #define QEMU_NBD_OPT_OBJECT 260 #define QEMU_NBD_OPT_TLSCREDS 261 #define QEMU_NBD_OPT_IMAGE_OPTS 262 +#define QEMU_NBD_OPT_SRV_SOCKET_FD 263 #define MBR_SIZE 512 +#define STR(s) _STR(s) +#define _STR(s) #s + static NBDExport *exp; static bool newproto; static int verbose; @@ -78,6 +82,8 @@ static void usage(const char *name) " -b, --bind=IFACE interface to bind to (default `0.0.0.0')\n" " -k, --socket=PATH path to the unix socket\n" " (default '"SOCKET_PATH"')\n" +" --server-sock-fd=NUM pre-opened listening server socket\n" +" file descriptor\n" " -e, --shared=NUM device can be shared by NUM clients (default '1')\n" " -t, --persistent don't exit on the last connection\n" " -v, --verbose display extra debugging information\n" @@ -382,9 +388,8 @@ static void nbd_update_server_watch(void) } -static SocketAddress *nbd_build_socket_address(const char *sockpath, - const char *bindto, - const char *port) +static SocketAddress *nbd_build_sock_fd(const char *sockpath, + const char *bindto, const char *port) { SocketAddress *saddr; @@ -459,6 +464,41 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) return creds; } +static void setup_address_and_port(const char **address, const char **port) +{ + if (*address == NULL) { + *address = "0.0.0.0"; + } + + if (*port == NULL) { + *port = STR(NBD_DEFAULT_PORT); + } +} + +/* ++ * Check socket parameters compatibility when sock_fd is given ++ */ +static const char *sock_fd_validate_opts(char *device, char *sockpath, + const char *address, const char *port) +{ + if (device != NULL) { + return "NBD device can't be set when socket fd is specified"; + } + + if (sockpath != NULL) { + return "Unix socket can't be set when socket fd is specified"; + } + + if (address != NULL) { + return "The interface can't be set when socket fd is specified"; + } + + if (port != NULL) { + return "TCP port number can't be set when socket fd is specified"; + } + + return NULL; +} int main(int argc, char **argv) { @@ -467,7 +507,7 @@ int main(int argc, char **argv) off_t dev_offset = 0; uint16_t nbdflags = 0; bool disconnect = false; - const char *bindto = "0.0.0.0"; + const char *bindto = NULL; const char *port = NULL; char *sockpath = NULL; char *device = NULL; @@ -503,6 +543,8 @@ int main(int argc, char **argv) { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, { "trace", required_argument, NULL, 'T' }, + { "server-sock-fd", required_argument, NULL, + QEMU_NBD_OPT_SRV_SOCKET_FD }, { NULL, 0, NULL, 0 } }; int ch; @@ -524,6 +566,7 @@ int main(int argc, char **argv) bool imageOpts = false; bool writethrough = true; char *trace_file = NULL; + int sock_fd = -1; /* The client thread uses SIGTERM to interrupt the server. A signal * handler ensures that "qemu-nbd -v -c" exits with a nice status code. @@ -714,6 +757,15 @@ int main(int argc, char **argv) g_free(trace_file); trace_file = trace_opt_parse(optarg); break; + case QEMU_NBD_OPT_SRV_SOCKET_FD: + sock_fd = qemu_parse_fd(optarg); + + if (sock_fd < 0) { + error_report("Invalid socket fd value: " + "it must be a non-negative integer"); + exit(EXIT_FAILURE); + } + break; } } @@ -735,6 +787,17 @@ int main(int argc, char **argv) trace_init_file(trace_file); qemu_set_log(LOG_TRACE); + if (sock_fd != -1) { + const char *err_msg = sock_fd_validate_opts(device, sockpath, + bindto, port); + if (err_msg != NULL) { + error_report("%s", err_msg); + exit(EXIT_FAILURE); + } + } else { + setup_address_and_port(&bindto, &port); + } + if (tlscredsid) { if (sockpath) { error_report("TLS is only supported with IPv4/IPv6"); @@ -838,7 +901,22 @@ int main(int argc, char **argv) snprintf(sockpath, 128, SOCKET_PATH, basename(device)); } - saddr = nbd_build_socket_address(sockpath, bindto, port); + if (sock_fd == -1) { + server_ioc = qio_channel_socket_new(); + saddr = nbd_build_sock_fd(sockpath, bindto, port); + if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) { + object_unref(OBJECT(server_ioc)); + error_report_err(local_err); + return 1; + } + } else { + server_ioc = qio_channel_socket_new_fd(sock_fd, &local_err); + if (server_ioc == NULL) { + error_report("Failed to use the given server socket fd: %s", + error_get_pretty(local_err)); + exit(EXIT_FAILURE); + } + } if (qemu_init_main_loop(&local_err)) { error_report_err(local_err); @@ -921,13 +999,6 @@ int main(int argc, char **argv) newproto = true; } - server_ioc = qio_channel_socket_new(); - if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) { - object_unref(OBJECT(server_ioc)); - error_report_err(local_err); - return 1; - } - if (device) { int ret; diff --git a/qemu-nbd.texi b/qemu-nbd.texi index 91ebf04..4c9ea00 100644 --- a/qemu-nbd.texi +++ b/qemu-nbd.texi @@ -34,6 +34,12 @@ The offset into the image The interface to bind to (default @samp{0.0.0.0}) @item -k, --socket=@var{path} Use a unix socket with path @var{path} +@item --server-sock-fd=@var{fd_num} +A server socket fd to be used for client connections. +The socket fd must be configured and switched to listening +state before passing to the program. The program uses +the given socket fd as is, without any additional +modifications. @item --image-opts Treat @var{filename} as a set of image options, instead of a plain filename. If this flag is specified, the @var{-f} flag should