* [PATCH 1/8] qemu-nbd: improve error message for dup2 error
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 20:46 ` Eric Blake
2023-09-06 9:32 ` [PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined Denis V. Lunev
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
This error is happened when we are not able to close the pipe to the
parent (to trace errors in the child process) and assign stderr to
/dev/null as required by the daemonizing convention.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Suggested-by: Eric Blake <eblake@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
qemu-nbd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index aaccaa3318..4575e4291e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -324,7 +324,7 @@ static void *nbd_client_thread(void *arg)
} else {
/* Close stderr so that the qemu-nbd process exits. */
if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
- error_report("Could not set stderr to /dev/null: %s",
+ error_report("Could not release pipe to parent: %s",
strerror(errno));
exit(EXIT_FAILURE);
}
@@ -1181,7 +1181,7 @@ int main(int argc, char **argv)
if (fork_process) {
if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
- error_report("Could not set stderr to /dev/null: %s",
+ error_report("Could not release pipe to parent: %s",
strerror(errno));
exit(EXIT_FAILURE);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/8] qemu-nbd: improve error message for dup2 error
2023-09-06 9:32 ` [PATCH 1/8] qemu-nbd: improve error message for dup2 error Denis V. Lunev
@ 2023-09-07 20:46 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 20:46 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:03AM +0200, Denis V. Lunev wrote:
> This error is happened when we are not able to close the pipe to the
s/is happened when/happens if/
> parent (to trace errors in the child process) and assign stderr to
> /dev/null as required by the daemonizing convention.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Suggested-by: Eric Blake <eblake@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qemu-nbd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index aaccaa3318..4575e4291e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -324,7 +324,7 @@ static void *nbd_client_thread(void *arg)
> } else {
> /* Close stderr so that the qemu-nbd process exits. */
> if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
> - error_report("Could not set stderr to /dev/null: %s",
> + error_report("Could not release pipe to parent: %s",
> strerror(errno));
> exit(EXIT_FAILURE);
> }
> @@ -1181,7 +1181,7 @@ int main(int argc, char **argv)
>
> if (fork_process) {
> if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
> - error_report("Could not set stderr to /dev/null: %s",
> + error_report("Could not release pipe to parent: %s",
> strerror(errno));
> exit(EXIT_FAILURE);
> }
> --
> 2.34.1
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
2023-09-06 9:32 ` [PATCH 1/8] qemu-nbd: improve error message for dup2 error Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 21:35 ` Eric Blake
2023-09-06 9:32 ` [PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts Denis V. Lunev
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
This patch also drops definition of some locals in main() to avoid
useless data copy.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
qemu-nbd.c | 60 ++++++++++++++++++++++++------------------------------
1 file changed, 27 insertions(+), 33 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4575e4291e..ebfae4d0b6 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -253,6 +253,12 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
}
+struct NbdClientOpts {
+ char *device;
+ bool fork_process;
+ bool verbose;
+};
+
#if HAVE_NBD_DEVICE
static void *show_parts(void *arg)
{
@@ -271,12 +277,6 @@ static void *show_parts(void *arg)
return NULL;
}
-struct NbdClientOpts {
- char *device;
- bool fork_process;
- bool verbose;
-};
-
static void *nbd_client_thread(void *arg)
{
struct NbdClientOpts *opts = arg;
@@ -519,7 +519,6 @@ int main(int argc, char **argv)
const char *bindto = NULL;
const char *port = NULL;
char *sockpath = NULL;
- char *device = NULL;
QemuOpts *sn_opts = NULL;
const char *sn_id_or_name = NULL;
const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:AB:L";
@@ -582,16 +581,16 @@ int main(int argc, char **argv)
const char *tlshostname = NULL;
bool imageOpts = false;
bool writethrough = false; /* Client will flush as needed. */
- bool verbose = false;
- bool fork_process = false;
bool list = false;
unsigned socket_activation;
const char *pid_file_name = NULL;
const char *selinux_label = NULL;
BlockExportOptions *export_opts;
-#if HAVE_NBD_DEVICE
- struct NbdClientOpts opts;
-#endif
+ struct NbdClientOpts opts = {
+ .fork_process = false,
+ .verbose = false,
+ .device = NULL,
+ };
#ifdef CONFIG_POSIX
os_setup_early_signal_handling();
@@ -719,7 +718,7 @@ int main(int argc, char **argv)
disconnect = true;
break;
case 'c':
- device = optarg;
+ opts.device = optarg;
break;
case 'e':
if (qemu_strtoi(optarg, NULL, 0, &shared) < 0 ||
@@ -750,7 +749,7 @@ int main(int argc, char **argv)
}
break;
case 'v':
- verbose = true;
+ opts.verbose = true;
break;
case 'V':
version(argv[0]);
@@ -782,7 +781,7 @@ int main(int argc, char **argv)
tlsauthz = optarg;
break;
case QEMU_NBD_OPT_FORK:
- fork_process = true;
+ opts.fork_process = true;
break;
case 'L':
list = true;
@@ -802,12 +801,12 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
if (export_name || export_description || dev_offset ||
- device || disconnect || fmt || sn_id_or_name || bitmaps ||
+ opts.device || disconnect || fmt || sn_id_or_name || bitmaps ||
alloc_depth || seen_aio || seen_discard || seen_cache) {
error_report("List mode is incompatible with per-device settings");
exit(EXIT_FAILURE);
}
- if (fork_process) {
+ if (opts.fork_process) {
error_report("List mode is incompatible with forking");
exit(EXIT_FAILURE);
}
@@ -832,7 +831,8 @@ int main(int argc, char **argv)
}
} else {
/* Using socket activation - check user didn't use -p etc. */
- const char *err_msg = socket_activation_validate_opts(device, sockpath,
+ const char *err_msg = socket_activation_validate_opts(opts.device,
+ sockpath,
bindto, port,
selinux_label,
list);
@@ -850,7 +850,7 @@ int main(int argc, char **argv)
}
if (tlscredsid) {
- if (device) {
+ if (opts.device) {
error_report("TLS is not supported with a host device");
exit(EXIT_FAILURE);
}
@@ -880,7 +880,7 @@ int main(int argc, char **argv)
if (selinux_label) {
#ifdef CONFIG_SELINUX
- if (sockpath == NULL && device == NULL) {
+ if (sockpath == NULL && opts.device == NULL) {
error_report("--selinux-label is not permitted without --socket");
exit(EXIT_FAILURE);
}
@@ -897,7 +897,7 @@ int main(int argc, char **argv)
}
#if !HAVE_NBD_DEVICE
- if (disconnect || device) {
+ if (disconnect || opts.device) {
error_report("Kernel /dev/nbdN support not available");
exit(EXIT_FAILURE);
}
@@ -919,7 +919,7 @@ int main(int argc, char **argv)
}
#endif
- if ((device && !verbose) || fork_process) {
+ if ((opts.device && !opts.verbose) || opts.fork_process) {
#ifndef WIN32
g_autoptr(GError) err = NULL;
int stderr_fd[2];
@@ -1002,9 +1002,9 @@ int main(int argc, char **argv)
#endif /* WIN32 */
}
- if (device != NULL && sockpath == NULL) {
+ if (opts.device != NULL && sockpath == NULL) {
sockpath = g_malloc(128);
- snprintf(sockpath, 128, SOCKET_PATH, basename(device));
+ snprintf(sockpath, 128, SOCKET_PATH, basename(opts.device));
}
server = qio_net_listener_new();
@@ -1145,15 +1145,9 @@ int main(int argc, char **argv)
blk_exp_add(export_opts, &error_fatal);
qapi_free_BlockExportOptions(export_opts);
- if (device) {
+ if (opts.device) {
#if HAVE_NBD_DEVICE
int ret;
- opts = (struct NbdClientOpts) {
- .device = device,
- .fork_process = fork_process,
- .verbose = verbose,
- };
-
ret = pthread_create(&client_thread, NULL, nbd_client_thread, &opts);
if (ret != 0) {
error_report("Failed to create client thread: %s", strerror(ret));
@@ -1179,7 +1173,7 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
- if (fork_process) {
+ if (opts.fork_process) {
if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
error_report("Could not release pipe to parent: %s",
strerror(errno));
@@ -1203,7 +1197,7 @@ int main(int argc, char **argv)
qemu_opts_del(sn_opts);
- if (device) {
+ if (opts.device) {
void *ret;
pthread_join(client_thread, &ret);
exit(ret != NULL);
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined
2023-09-06 9:32 ` [PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined Denis V. Lunev
@ 2023-09-07 21:35 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 21:35 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:04AM +0200, Denis V. Lunev wrote:
> This patch also drops definition of some locals in main() to avoid
> useless data copy.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qemu-nbd.c | 60 ++++++++++++++++++++++++------------------------------
> 1 file changed, 27 insertions(+), 33 deletions(-)
> @@ -519,7 +519,6 @@ int main(int argc, char **argv)
> const char *bindto = NULL;
> const char *port = NULL;
> char *sockpath = NULL;
> - char *device = NULL;
> QemuOpts *sn_opts = NULL;
> const char *sn_id_or_name = NULL;
> const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:AB:L";
> @@ -582,16 +581,16 @@ int main(int argc, char **argv)
> const char *tlshostname = NULL;
> bool imageOpts = false;
> bool writethrough = false; /* Client will flush as needed. */
> - bool verbose = false;
> - bool fork_process = false;
> bool list = false;
> unsigned socket_activation;
> const char *pid_file_name = NULL;
> const char *selinux_label = NULL;
> BlockExportOptions *export_opts;
> -#if HAVE_NBD_DEVICE
> - struct NbdClientOpts opts;
> -#endif
> + struct NbdClientOpts opts = {
> + .fork_process = false,
> + .verbose = false,
> + .device = NULL,
> + };
Could also do 'struct NbdClietnOpts opts = {};' since you happen to be
zero-initializing, but this may not remain the case if more fields get
added to the struct, so I'm fine leaving it as written.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
2023-09-06 9:32 ` [PATCH 1/8] qemu-nbd: improve error message for dup2 error Denis V. Lunev
2023-09-06 9:32 ` [PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 21:43 ` Eric Blake
2023-09-06 9:32 ` [PATCH 4/8] qemu-nbd: put saddr into " Denis V. Lunev
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
We pass other parameters into nbd_client_thread() in this way. This patch
makes the code more consistent.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
qemu-nbd.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index ebfae4d0b6..de6c2be590 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -73,7 +73,6 @@
#define MBR_SIZE 512
-static char *srcpath;
static SocketAddress *saddr;
static int persistent = 0;
static enum { RUNNING, TERMINATE, TERMINATED } state;
@@ -255,6 +254,7 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
struct NbdClientOpts {
char *device;
+ char *srcpath;
bool fork_process;
bool verbose;
};
@@ -320,7 +320,7 @@ static void *nbd_client_thread(void *arg)
if (opts->verbose && !opts->fork_process) {
fprintf(stderr, "NBD device %s is now connected to %s\n",
- opts->device, srcpath);
+ opts->device, opts->srcpath);
} else {
/* Close stderr so that the qemu-nbd process exits. */
if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
@@ -590,6 +590,7 @@ int main(int argc, char **argv)
.fork_process = false,
.verbose = false,
.device = NULL,
+ .srcpath = NULL,
};
#ifdef CONFIG_POSIX
@@ -1059,19 +1060,19 @@ int main(int argc, char **argv)
bdrv_init();
atexit(qemu_nbd_shutdown);
- srcpath = argv[optind];
+ opts.srcpath = argv[optind];
if (imageOpts) {
- QemuOpts *opts;
+ QemuOpts *o;
if (fmt) {
error_report("--image-opts and -f are mutually exclusive");
exit(EXIT_FAILURE);
}
- opts = qemu_opts_parse_noisily(&file_opts, srcpath, true);
- if (!opts) {
+ o = qemu_opts_parse_noisily(&file_opts, opts.srcpath, true);
+ if (!o) {
qemu_opts_reset(&file_opts);
exit(EXIT_FAILURE);
}
- options = qemu_opts_to_qdict(opts, NULL);
+ options = qemu_opts_to_qdict(o, NULL);
qemu_opts_reset(&file_opts);
blk = blk_new_open(NULL, NULL, options, flags, &local_err);
} else {
@@ -1079,7 +1080,7 @@ int main(int argc, char **argv)
options = qdict_new();
qdict_put_str(options, "driver", fmt);
}
- blk = blk_new_open(srcpath, NULL, options, flags, &local_err);
+ blk = blk_new_open(opts.srcpath, NULL, options, flags, &local_err);
}
if (!blk) {
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts
2023-09-06 9:32 ` [PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts Denis V. Lunev
@ 2023-09-07 21:43 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 21:43 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:05AM +0200, Denis V. Lunev wrote:
> We pass other parameters into nbd_client_thread() in this way. This patch
> makes the code more consistent.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qemu-nbd.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> @@ -1059,19 +1060,19 @@ int main(int argc, char **argv)
> bdrv_init();
> atexit(qemu_nbd_shutdown);
>
> - srcpath = argv[optind];
> + opts.srcpath = argv[optind];
> if (imageOpts) {
> - QemuOpts *opts;
> + QemuOpts *o;
> if (fmt) {
> error_report("--image-opts and -f are mutually exclusive");
> exit(EXIT_FAILURE);
> }
> - opts = qemu_opts_parse_noisily(&file_opts, srcpath, true);
> - if (!opts) {
> + o = qemu_opts_parse_noisily(&file_opts, opts.srcpath, true);
> + if (!o) {
Hmm - this would have been flagged by -Wshadow, and there are other
series working to clean up tree-wide issues that shadowing can cause.
Looking again, the shadowing was previously introduced before this
series, but only when HAVE_NBD_DEVICE was defined; then patch 2/8 made
the shadowing unconditional. Reworking the series to clean up the
shadowing earlier in 2/8 is just churn, so I don't mind that it took
us to this point to notice it; however, I'm inclined to add a note to
the commit message that it is a (happy) side-effect.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/8] qemu-nbd: put saddr into into struct NbdClientOpts
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
` (2 preceding siblings ...)
2023-09-06 9:32 ` [PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 21:48 ` Eric Blake
2023-09-06 9:32 ` [PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper Denis V. Lunev
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
We pass other parameters into nbd_client_thread() in this way. This patch
makes the code more consistent.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
qemu-nbd.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index de6c2be590..d0f8d8bad2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -73,7 +73,6 @@
#define MBR_SIZE 512
-static SocketAddress *saddr;
static int persistent = 0;
static enum { RUNNING, TERMINATE, TERMINATED } state;
static int shared = 1;
@@ -255,6 +254,7 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
struct NbdClientOpts {
char *device;
char *srcpath;
+ SocketAddress *saddr;
bool fork_process;
bool verbose;
};
@@ -289,7 +289,7 @@ static void *nbd_client_thread(void *arg)
sioc = qio_channel_socket_new();
if (qio_channel_socket_connect_sync(sioc,
- saddr,
+ opts->saddr,
&local_error) < 0) {
error_report_err(local_error);
goto out;
@@ -591,6 +591,7 @@ int main(int argc, char **argv)
.verbose = false,
.device = NULL,
.srcpath = NULL,
+ .saddr = NULL,
};
#ifdef CONFIG_POSIX
@@ -892,8 +893,8 @@ int main(int argc, char **argv)
}
if (list) {
- saddr = nbd_build_socket_address(sockpath, bindto, port);
- return qemu_nbd_client_list(saddr, tlscreds,
+ opts.saddr = nbd_build_socket_address(sockpath, bindto, port);
+ return qemu_nbd_client_list(opts.saddr, tlscreds,
tlshostname ? tlshostname : bindto);
}
@@ -1024,8 +1025,8 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
#endif
- saddr = nbd_build_socket_address(sockpath, bindto, port);
- if (qio_net_listener_open_sync(server, saddr, backlog,
+ opts.saddr = nbd_build_socket_address(sockpath, bindto, port);
+ if (qio_net_listener_open_sync(server, opts.saddr, backlog,
&local_err) < 0) {
object_unref(OBJECT(server));
error_report_err(local_err);
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/8] qemu-nbd: put saddr into into struct NbdClientOpts
2023-09-06 9:32 ` [PATCH 4/8] qemu-nbd: put saddr into " Denis V. Lunev
@ 2023-09-07 21:48 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 21:48 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:06AM +0200, Denis V. Lunev wrote:
> We pass other parameters into nbd_client_thread() in this way. This patch
> makes the code more consistent.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qemu-nbd.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
` (3 preceding siblings ...)
2023-09-06 9:32 ` [PATCH 4/8] qemu-nbd: put saddr into " Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 21:54 ` Eric Blake
2023-09-06 9:32 ` [PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
Move the code from main() and nbd_client_thread() into the specific
helper. This code is going to be grown.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
qemu-nbd.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d0f8d8bad2..9f28e3ebda 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -259,6 +259,16 @@ struct NbdClientOpts {
bool verbose;
};
+static void nbd_client_release_pipe(void)
+{
+ /* Close stderr so that the qemu-nbd process exits. */
+ if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
+ error_report("Could not release pipe to parent: %s",
+ strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+}
+
#if HAVE_NBD_DEVICE
static void *show_parts(void *arg)
{
@@ -322,12 +332,7 @@ static void *nbd_client_thread(void *arg)
fprintf(stderr, "NBD device %s is now connected to %s\n",
opts->device, opts->srcpath);
} else {
- /* Close stderr so that the qemu-nbd process exits. */
- if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
- error_report("Could not release pipe to parent: %s",
- strerror(errno));
- exit(EXIT_FAILURE);
- }
+ nbd_client_release_pipe();
}
if (nbd_client(fd) < 0) {
@@ -1176,11 +1181,7 @@ int main(int argc, char **argv)
}
if (opts.fork_process) {
- if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
- error_report("Could not release pipe to parent: %s",
- strerror(errno));
- exit(EXIT_FAILURE);
- }
+ nbd_client_release_pipe();
}
state = RUNNING;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper
2023-09-06 9:32 ` [PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper Denis V. Lunev
@ 2023-09-07 21:54 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 21:54 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:07AM +0200, Denis V. Lunev wrote:
> Move the code from main() and nbd_client_thread() into the specific
> helper. This code is going to be grown.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qemu-nbd.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
` (4 preceding siblings ...)
2023-09-06 9:32 ` [PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 21:59 ` Eric Blake
2023-09-06 9:32 ` [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man Denis V. Lunev
2023-09-06 9:32 ` [PATCH 8/8] qemu-nbd: fix formatting in main() Denis V. Lunev
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: den, Kevin Wolf, Eric Blake, Vladimir Sementsov-Ogievskiy,
Hanna Reitz, Mike Maslenkin
Closing stderr earlier is good for daemonized qemu-nbd under ssh
earlier, but breaks the case where -v is being used to track what is
happening in the server, as in iotest 233.
When we know we are verbose, we should preserve original stderr and
restore it once the setup stage is done. This commit restores the
original behavior with -v option. In this case original output
inside the test is kept intact.
Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Mike Maslenkin <mike.maslenkin@gmail.com>
Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh")
---
qemu-nbd.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9f28e3ebda..b9c74ce77c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -255,18 +255,23 @@ struct NbdClientOpts {
char *device;
char *srcpath;
SocketAddress *saddr;
+ int stderr;
bool fork_process;
bool verbose;
};
-static void nbd_client_release_pipe(void)
+static void nbd_client_release_pipe(int old_stderr)
{
/* Close stderr so that the qemu-nbd process exits. */
- if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
+ if (dup2(old_stderr, STDERR_FILENO) < 0) {
error_report("Could not release pipe to parent: %s",
strerror(errno));
exit(EXIT_FAILURE);
}
+ if (old_stderr != STDOUT_FILENO && close(old_stderr) < 0) {
+ error_report("Could not release qemu-nbd: %s", strerror(errno));
+ exit(EXIT_FAILURE);
+ }
}
#if HAVE_NBD_DEVICE
@@ -332,7 +337,7 @@ static void *nbd_client_thread(void *arg)
fprintf(stderr, "NBD device %s is now connected to %s\n",
opts->device, opts->srcpath);
} else {
- nbd_client_release_pipe();
+ nbd_client_release_pipe(opts->stderr);
}
if (nbd_client(fd) < 0) {
@@ -597,6 +602,7 @@ int main(int argc, char **argv)
.device = NULL,
.srcpath = NULL,
.saddr = NULL,
+ .stderr = STDOUT_FILENO,
};
#ifdef CONFIG_POSIX
@@ -951,6 +957,16 @@ int main(int argc, char **argv)
close(stderr_fd[0]);
+ /* Remember parent's stderr if we will be restoring it. */
+ if (opts.verbose /* fork_process is set */) {
+ opts.stderr = dup(STDERR_FILENO);
+ if (opts.stderr < 0) {
+ error_report("Could not dup original stderr: %s",
+ strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+ }
+
ret = qemu_daemon(1, 0);
saved_errno = errno; /* dup2 will overwrite error below */
@@ -1181,7 +1197,7 @@ int main(int argc, char **argv)
}
if (opts.fork_process) {
- nbd_client_release_pipe();
+ nbd_client_release_pipe(opts.stderr);
}
state = RUNNING;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output
2023-09-06 9:32 ` [PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
@ 2023-09-07 21:59 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 21:59 UTC (permalink / raw)
To: Denis V. Lunev
Cc: qemu-devel, qemu-block, Kevin Wolf, Vladimir Sementsov-Ogievskiy,
Hanna Reitz, Mike Maslenkin
On Wed, Sep 06, 2023 at 11:32:08AM +0200, Denis V. Lunev wrote:
> Closing stderr earlier is good for daemonized qemu-nbd under ssh
> earlier, but breaks the case where -v is being used to track what is
> happening in the server, as in iotest 233.
>
> When we know we are verbose, we should preserve original stderr and
> restore it once the setup stage is done. This commit restores the
> original behavior with -v option. In this case original output
> inside the test is kept intact.
>
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Mike Maslenkin <mike.maslenkin@gmail.com>
> Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh")
> ---
> qemu-nbd.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
Tested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
` (5 preceding siblings ...)
2023-09-06 9:32 ` [PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 22:01 ` Eric Blake
2023-09-06 9:32 ` [PATCH 8/8] qemu-nbd: fix formatting in main() Denis V. Lunev
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
docs/tools/qemu-nbd.rst | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index faf6349ea5..5c48ee7345 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -197,7 +197,9 @@ driver options if :option:`--image-opts` is specified.
.. option:: -v, --verbose
- Display extra debugging information.
+ Display extra debugging information. This option also keeps opened original
+ *STDERR* stream if ``qemu-nbd`` process is daemonized due to other options
+ like :option:`--fork` or :option:`-c`.
.. option:: -h, --help
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man
2023-09-06 9:32 ` [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man Denis V. Lunev
@ 2023-09-07 22:01 ` Eric Blake
2023-09-07 22:03 ` Denis V. Lunev
0 siblings, 1 reply; 18+ messages in thread
From: Eric Blake @ 2023-09-07 22:01 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:09AM +0200, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> docs/tools/qemu-nbd.rst | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
> index faf6349ea5..5c48ee7345 100644
> --- a/docs/tools/qemu-nbd.rst
> +++ b/docs/tools/qemu-nbd.rst
> @@ -197,7 +197,9 @@ driver options if :option:`--image-opts` is specified.
>
> .. option:: -v, --verbose
>
> - Display extra debugging information.
> + Display extra debugging information. This option also keeps opened original
> + *STDERR* stream if ``qemu-nbd`` process is daemonized due to other options
> + like :option:`--fork` or :option:`-c`.
As a native speaker, I find the following a bit easier to parse:
This option also keeps the original *STDERR* stream open if ...
I can make that touchup as part of queuing the series.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man
2023-09-07 22:01 ` Eric Blake
@ 2023-09-07 22:03 ` Denis V. Lunev
0 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-07 22:03 UTC (permalink / raw)
To: Eric Blake, Denis V. Lunev
Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On 9/8/23 00:01, Eric Blake wrote:
> On Wed, Sep 06, 2023 at 11:32:09AM +0200, Denis V. Lunev wrote:
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> docs/tools/qemu-nbd.rst | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
>> index faf6349ea5..5c48ee7345 100644
>> --- a/docs/tools/qemu-nbd.rst
>> +++ b/docs/tools/qemu-nbd.rst
>> @@ -197,7 +197,9 @@ driver options if :option:`--image-opts` is specified.
>>
>> .. option:: -v, --verbose
>>
>> - Display extra debugging information.
>> + Display extra debugging information. This option also keeps opened original
>> + *STDERR* stream if ``qemu-nbd`` process is daemonized due to other options
>> + like :option:`--fork` or :option:`-c`.
> As a native speaker, I find the following a bit easier to parse:
>
> This option also keeps the original *STDERR* stream open if ...
>
> I can make that touchup as part of queuing the series.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
That would be great, thanks!
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 8/8] qemu-nbd: fix formatting in main()
2023-09-06 9:32 [PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output Denis V. Lunev
` (6 preceding siblings ...)
2023-09-06 9:32 ` [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man Denis V. Lunev
@ 2023-09-06 9:32 ` Denis V. Lunev
2023-09-07 22:15 ` Eric Blake
7 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2023-09-06 9:32 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: den, Eric Blake, Vladimir Sementsov-Ogievskiy
Just a formatting, no functional changes.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
Do not really sure that this patch is mandatory, just stabs my eye. Feel free
to drop if this is too useless.
qemu-nbd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b9c74ce77c..8eb1d1f40b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -581,7 +581,8 @@ int main(int argc, char **argv)
pthread_t client_thread;
const char *fmt = NULL;
Error *local_err = NULL;
- BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
+ BlockdevDetectZeroesOptions detect_zeroes =
+ BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
QDict *options = NULL;
const char *export_name = NULL; /* defaults to "" later for server mode */
const char *export_description = NULL;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 8/8] qemu-nbd: fix formatting in main()
2023-09-06 9:32 ` [PATCH 8/8] qemu-nbd: fix formatting in main() Denis V. Lunev
@ 2023-09-07 22:15 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2023-09-07 22:15 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, qemu-block, Vladimir Sementsov-Ogievskiy
On Wed, Sep 06, 2023 at 11:32:10AM +0200, Denis V. Lunev wrote:
> Just a formatting, no functional changes.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> Do not really sure that this patch is mandatory, just stabs my eye. Feel free
> to drop if this is too useless.
>
> qemu-nbd.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b9c74ce77c..8eb1d1f40b 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -581,7 +581,8 @@ int main(int argc, char **argv)
> pthread_t client_thread;
> const char *fmt = NULL;
> Error *local_err = NULL;
> - BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
> + BlockdevDetectZeroesOptions detect_zeroes =
> + BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
check-patch allows code up to 90 columngs although it does advise
staying under 80. You fixed the long line by keeping the wrapped
portion right-flushed to 80 columns; I think more typical tree-wide is
to just indent by four spaces (at least, that's what emacs suggests I
do). But me changing what you wrote would a complete rewrite, so I'm
reluctant to include it in my upcoming pull request, although I'm not
ruling out a later cleanup (perhaps if it touches more than one
stylistic thing at once).
I'm queuing 1-7 through my NBD tree, and running another round of
iotests before sending the pull request this week.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 18+ messages in thread