* [Qemu-devel] [PULL 01/28] checkpatch: Eliminate false positive in case of comma-space-square bracket
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 02/28] checkpatch: Eliminate false positive in case of space before square bracket in a definition Paolo Bonzini
` (27 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonid Bloch
From: Leonid Bloch <leonid@daynix.com>
Previously, an error was printed in cases such as:
{ [1] = 5, [2] = 6 }
The space passed OK after a curly brace, but not after a comma.
Now, a space before a square bracket is allowed, if a comma comes before
it.
Signed-off-by: Leonid Bloch <leonid@daynix.com>
Message-Id: <1446112118-12376-2-git-send-email-leonid@daynix.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/checkpatch.pl | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 257126f..e71c3d1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1715,11 +1715,13 @@ sub process {
# 1. with a type on the left -- int [] a;
# 2. at the beginning of a line for slice initialisers -- [0...10] = 5,
# 3. inside a curly brace -- = { [0...10] = 5 }
+# 4. after a comma -- [1] = 5, [2] = 6
while ($line =~ /(.*?\s)\[/g) {
my ($where, $prefix) = ($-[1], $1);
if ($prefix !~ /$Type\s+$/ &&
($where != 0 || $prefix !~ /^.\s+$/) &&
- $prefix !~ /{\s+$/) {
+ $prefix !~ /{\s+$/ &&
+ $prefix !~ /,\s+$/) {
ERROR("space prohibited before open square bracket '['\n" . $herecurr);
}
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 02/28] checkpatch: Eliminate false positive in case of space before square bracket in a definition
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 01/28] checkpatch: Eliminate false positive in case of comma-space-square bracket Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 03/28] Revert "qemu-char: Keep pty slave file descriptor open until the master is closed" Paolo Bonzini
` (26 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonid Bloch
From: Leonid Bloch <leonid@daynix.com>
Now, macro definition such as "#define abc(x) [x] = y" should pass
without an error.
Signed-off-by: Leonid Bloch <leonid@daynix.com>
Message-Id: <1446112118-12376-3-git-send-email-leonid@daynix.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/checkpatch.pl | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e71c3d1..c26f76e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1716,11 +1716,13 @@ sub process {
# 2. at the beginning of a line for slice initialisers -- [0...10] = 5,
# 3. inside a curly brace -- = { [0...10] = 5 }
# 4. after a comma -- [1] = 5, [2] = 6
+# 5. in a macro definition -- #define abc(x) [x] = y
while ($line =~ /(.*?\s)\[/g) {
my ($where, $prefix) = ($-[1], $1);
if ($prefix !~ /$Type\s+$/ &&
($where != 0 || $prefix !~ /^.\s+$/) &&
$prefix !~ /{\s+$/ &&
+ $prefix !~ /\#\s*define[^(]*\([^)]*\)\s+$/ &&
$prefix !~ /,\s+$/) {
ERROR("space prohibited before open square bracket '['\n" . $herecurr);
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 03/28] Revert "qemu-char: Keep pty slave file descriptor open until the master is closed"
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 01/28] checkpatch: Eliminate false positive in case of comma-space-square bracket Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 02/28] checkpatch: Eliminate false positive in case of space before square bracket in a definition Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 04/28] char: fix handling of QIO_CHANNEL_ERR_BLOCK Paolo Bonzini
` (25 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
This reverts commit 34689e206abddac87a5217d458534e24f2a05562.
Marc-André Lureau provided the following commentary: "It looks like if
a the slave is opened, then Linux will buffer the master writes, up to
a few kb and then throttle, so it's not entirely blocked but eventually
the guest VM dies. However, not having any slave open it will simply let
the write go and discard the data. At least, virt-install configures
a pty for the serial but viewers like virt-manager do not necessarily
open it. And, if there are no viewers, it will just hang. If qemu
starts reading all the data from the slave, I don't think interactions
with other slaves will work. I don't see much options but to close the
slave, thus reverting this patch."
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-char.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 1b7d5da..00caf65 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1171,7 +1171,6 @@ typedef struct {
int connected;
guint timer_tag;
guint open_tag;
- int slave_fd;
} PtyCharDriver;
static void pty_chr_update_read_handler_locked(CharDriverState *chr);
@@ -1348,7 +1347,6 @@ static void pty_chr_close(struct CharDriverState *chr)
qemu_mutex_lock(&chr->chr_write_lock);
pty_chr_state(chr, 0);
- close(s->slave_fd);
object_unref(OBJECT(s->ioc));
if (s->timer_tag) {
g_source_remove(s->timer_tag);
@@ -1376,6 +1374,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
return NULL;
}
+ close(slave_fd);
qemu_set_nonblock(master_fd);
chr = qemu_chr_alloc(common, errp);
@@ -1400,7 +1399,6 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
chr->explicit_be_open = true;
s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
- s->slave_fd = slave_fd;
s->timer_tag = 0;
return chr;
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 04/28] char: fix handling of QIO_CHANNEL_ERR_BLOCK
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (2 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 03/28] Revert "qemu-char: Keep pty slave file descriptor open until the master is closed" Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 05/28] build: Don't redefine 'inline' Paolo Bonzini
` (24 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
If io_channel_send_full gets QIO_CHANNEL_ERR_BLOCK it
and has already sent some of the data, it should return
that amount of data, not EAGAIN, as that would cause
the caller to re-try already sent data.
Unfortunately due to a previous rebase conflict resolution
error, the code for dealing with this was in the wrong
part of the conditional, and so mistakenly ran on other
I/O errors.
This be seen running
qemu-system-x86_64 -monitor stdio
and entering 'info mtree', when running on a slow console
(eg a slow remote ssh session). The monitor would get into
an indefinite loop writing the same data until it managed
to send it all without getting EAGAIN.
Reported-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455288410-27046-1-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-char.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 00caf65..ad11b75 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -896,13 +896,13 @@ static int io_channel_send_full(QIOChannel *ioc,
ioc, &iov, 1,
fds, nfds, NULL);
if (ret == QIO_CHANNEL_ERR_BLOCK) {
- errno = EAGAIN;
- return -1;
- } else if (ret < 0) {
if (offset) {
return offset;
}
+ errno = EAGAIN;
+ return -1;
+ } else if (ret < 0) {
errno = EINVAL;
return -1;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 05/28] build: Don't redefine 'inline'
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (3 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 04/28] char: fix handling of QIO_CHANNEL_ERR_BLOCK Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 06/28] vl: change QEMU state machine for system reset Paolo Bonzini
` (23 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: Eric Blake <eblake@redhat.com>
Actively redefining 'inline' is wrong for C++, where gcc has an
extension 'inline namespace' which fails to compile if the
keyword 'inline' is replaced by a macro expansion. This will
matter once we start to include "qemu/osdep.h" first from C++
files, depending also on whether the system headers are new
enough to be using the gcc extension.
But rather than just guard things by __cplusplus, let's look at
the overall picture. Commit df2542c737ea2 in 2007 defined 'inline'
to the gcc attribute __always_inline__, with the rationale "To
avoid discarded inlining bug". But compilers have improved since
then, and we are probably better off trusting the compiler rather
than trying to force its hand.
So just nuke our craziness.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455043788-28112-1-git-send-email-eblake@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qemu/compiler.h | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index d22eb01..c5fbe28 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -77,18 +77,6 @@
#define typeof_field(type, field) typeof(((type *)0)->field)
#define type_check(t1,t2) ((t1*)0 - (t2*)0)
-#ifndef always_inline
-#if !((__GNUC__ < 3) || defined(__APPLE__))
-#ifdef __OPTIMIZE__
-#undef inline
-#define inline __attribute__ (( always_inline )) __inline__
-#endif
-#endif
-#else
-#undef inline
-#define inline always_inline
-#endif
-
#define QEMU_BUILD_BUG_ON(x) \
typedef char glue(qemu_build_bug_on__,__LINE__)[(x)?-1:1] __attribute__((unused));
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 06/28] vl: change QEMU state machine for system reset
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (4 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 05/28] build: Don't redefine 'inline' Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 07/28] vl: fix migration from prelaunch state Paolo Bonzini
` (22 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Denis V. Lunev
From: "Denis V. Lunev" <den@openvz.org>
This patch implements proposal from Paolo to handle system reset when
the guest is not running.
"After a reset, main_loop_should_exit should actually transition
to VM_STATE_PRELAUNCH (*not* RUN_STATE_PAUSED) for *all* states except
RUN_STATE_INMIGRATE, RUN_STATE_SAVE_VM (which I think cannot happen
there) and (of course) RUN_STATE_RUNNING."
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1455369986-20353-1-git-send-email-den@openvz.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
vl.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 175ebcc..76dfb54 100644
--- a/vl.c
+++ b/vl.c
@@ -579,6 +579,7 @@ static const RunStateTransition runstate_transitions_def[] = {
/* from -> to */
{ RUN_STATE_DEBUG, RUN_STATE_RUNNING },
{ RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
{ RUN_STATE_INMIGRATE, RUN_STATE_INTERNAL_ERROR },
{ RUN_STATE_INMIGRATE, RUN_STATE_IO_ERROR },
@@ -592,15 +593,19 @@ static const RunStateTransition runstate_transitions_def[] = {
{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PRELAUNCH },
{ RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
{ RUN_STATE_IO_ERROR, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_IO_ERROR, RUN_STATE_PRELAUNCH },
{ RUN_STATE_PAUSED, RUN_STATE_RUNNING },
{ RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
{ RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
{ RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_POSTMIGRATE, RUN_STATE_PRELAUNCH },
{ RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
{ RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
@@ -608,8 +613,10 @@ static const RunStateTransition runstate_transitions_def[] = {
{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
+ { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
{ RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
+ { RUN_STATE_RESTORE_VM, RUN_STATE_PRELAUNCH },
{ RUN_STATE_RUNNING, RUN_STATE_DEBUG },
{ RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
@@ -626,17 +633,21 @@ static const RunStateTransition runstate_transitions_def[] = {
{ RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
{ RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_SHUTDOWN, RUN_STATE_PRELAUNCH },
{ RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
{ RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
{ RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
{ RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
{ RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
{ RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_WATCHDOG, RUN_STATE_PRELAUNCH },
{ RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_GUEST_PANICKED, RUN_STATE_PRELAUNCH },
{ RUN_STATE__MAX, RUN_STATE__MAX },
};
@@ -1881,8 +1892,9 @@ static bool main_loop_should_exit(void)
cpu_synchronize_all_states();
qemu_system_reset(VMRESET_REPORT);
resume_all_vcpus();
- if (runstate_needs_reset()) {
- runstate_set(RUN_STATE_PAUSED);
+ if (!runstate_check(RUN_STATE_RUNNING) &&
+ !runstate_check(RUN_STATE_INMIGRATE)) {
+ runstate_set(RUN_STATE_PRELAUNCH);
}
}
if (qemu_wakeup_requested()) {
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 07/28] vl: fix migration from prelaunch state
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (5 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 06/28] vl: change QEMU state machine for system reset Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 08/28] migration: fix incorrect memory_global_dirty_log_start outside BQL Paolo Bonzini
` (21 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
Reproducer is simply to migrate a virtual machine that was started with -S,
or that was already migrated.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
vl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/vl.c b/vl.c
index 76dfb54..2e0116e 100644
--- a/vl.c
+++ b/vl.c
@@ -590,6 +590,8 @@ static const RunStateTransition runstate_transitions_def[] = {
{ RUN_STATE_INMIGRATE, RUN_STATE_WATCHDOG },
{ RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },
{ RUN_STATE_INMIGRATE, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_INMIGRATE, RUN_STATE_PRELAUNCH },
+ { RUN_STATE_INMIGRATE, RUN_STATE_POSTMIGRATE },
{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 08/28] migration: fix incorrect memory_global_dirty_log_start outside BQL
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (6 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 07/28] vl: fix migration from prelaunch state Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 09/28] mptsas: add missing va_end Paolo Bonzini
` (20 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Dave Gilbert
This can cause various segmentation faults or aborts in qemu-iotests
test 091.
Fixes: 5b82b703b69acc67b78b98a5efc897a3912719eb
Cc: Dave Gilbert <dgilbert@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
migration/ram.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/migration/ram.c b/migration/ram.c
index 96c749f..704f6a9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1920,6 +1920,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
acct_clear();
}
+ /* For memory_global_dirty_log_start below. */
+ qemu_mutex_lock_iothread();
+
qemu_mutex_lock_ramlist();
rcu_read_lock();
bytes_transferred = 0;
@@ -1944,6 +1947,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
memory_global_dirty_log_start();
migration_bitmap_sync();
qemu_mutex_unlock_ramlist();
+ qemu_mutex_unlock_iothread();
qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 09/28] mptsas: add missing va_end
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (7 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 08/28] migration: fix incorrect memory_global_dirty_log_start outside BQL Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 10/28] mptsas: fix memory leak Paolo Bonzini
` (19 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
Reported by Coverity.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/mptconfig.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/scsi/mptconfig.c b/hw/scsi/mptconfig.c
index d049825..7071854 100644
--- a/hw/scsi/mptconfig.c
+++ b/hw/scsi/mptconfig.c
@@ -123,6 +123,7 @@ static size_t vpack(uint8_t **p_data, const char *fmt, va_list ap1)
va_copy(ap2, ap1);
size = vfill(NULL, 0, fmt, ap2);
*p_data = data = g_malloc(size);
+ va_end(ap2);
}
return vfill(data, size, fmt, ap1);
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 10/28] mptsas: fix memory leak
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (8 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 09/28] mptsas: add missing va_end Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 11/28] mptsas: fix wrong formula Paolo Bonzini
` (18 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
Reported by Coverity.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/mptsas.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 333cc1f..1ce3226 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -504,6 +504,7 @@ reply_maybe_async:
reply_async->IOCLogInfo = count;
return;
}
+ g_free(reply_async);
reply.TerminationCount = count;
break;
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 11/28] mptsas: fix wrong formula
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (9 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 10/28] mptsas: fix memory leak Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 12/28] ipmi: sensor number should not exceed MAX_SENSORS Paolo Bonzini
` (17 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
MPI_DOORBELL_WHO_INIT_SHIFT is being repeated twice. Reported
by Coverity.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/mptsas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 1ce3226..499c146 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -824,7 +824,7 @@ static uint32_t mptsas_doorbell_read(MPTSASState *s)
{
uint32_t ret;
- ret = (s->who_init << MPI_DOORBELL_WHO_INIT_SHIFT) & MPI_DOORBELL_WHO_INIT_SHIFT;
+ ret = (s->who_init << MPI_DOORBELL_WHO_INIT_SHIFT) & MPI_DOORBELL_WHO_INIT_MASK;
ret |= s->state;
switch (s->doorbell_state) {
case DOORBELL_NONE:
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 12/28] ipmi: sensor number should not exceed MAX_SENSORS
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (10 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 11/28] mptsas: fix wrong formula Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 13/28] qom: add helpers for UserCreatable object types Paolo Bonzini
` (16 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Cédric Le Goater
From: Cédric Le Goater <clg@fr.ibm.com>
Fix a number of off-by-ones, one of them spotted by Coverity.
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/ipmi/ipmi_bmc_sim.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index f8b2176..51d234a 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -534,7 +534,7 @@ static void ipmi_init_sensors_from_sdrs(IPMIBmcSim *s)
continue; /* Not a sensor SDR we set from */
}
- if (sdr->sensor_owner_number > MAX_SENSORS) {
+ if (sdr->sensor_owner_number >= MAX_SENSORS) {
continue;
}
sens = s->sensors + sdr->sensor_owner_number;
@@ -1448,7 +1448,7 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
IPMISensor *sens;
IPMI_CHECK_CMD_LEN(4);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
@@ -1500,7 +1500,7 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
IPMISensor *sens;
IPMI_CHECK_CMD_LEN(3);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
@@ -1521,7 +1521,7 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
IPMISensor *sens;
IPMI_CHECK_CMD_LEN(4);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
@@ -1543,7 +1543,7 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
IPMISensor *sens;
IPMI_CHECK_CMD_LEN(3);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
@@ -1565,7 +1565,7 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
IPMISensor *sens;
IPMI_CHECK_CMD_LEN(3);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
@@ -1588,7 +1588,7 @@ static void set_sensor_type(IPMIBmcSim *ibs,
IPMI_CHECK_CMD_LEN(5);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
@@ -1607,7 +1607,7 @@ static void get_sensor_type(IPMIBmcSim *ibs,
IPMI_CHECK_CMD_LEN(3);
- if ((cmd[2] > MAX_SENSORS) ||
+ if ((cmd[2] >= MAX_SENSORS) ||
!IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
return;
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 13/28] qom: add helpers for UserCreatable object types
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (11 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 12/28] ipmi: sensor number should not exceed MAX_SENSORS Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 14/28] qemu-nbd: add support for --object command line arg Paolo Bonzini
` (15 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
The QMP monitor code has two helper methods object_add
and qmp_object_del that are called from several places
in the code (QMP, HMP and main emulator startup).
The HMP and main emulator startup code also share
further logic that extracts the qom-type & id
values from a qdict.
We soon need to use this logic from qemu-img, qemu-io
and qemu-nbd too, but don't want those to depend on
the monitor, nor do we want to duplicate the code.
To avoid this, move some code out of qmp.c and hmp.c
adding new methods to qom/object_interfaces.c
- user_creatable_add - takes a QDict holding a full
object definition & instantiates it
- user_creatable_add_type - takes an ID, type name,
and QDict holding object properties & instantiates
it
- user_creatable_add_opts - takes a QemuOpts holding
a full object definition & instantiates it
- user_creatable_add_opts_foreach - variant on
user_creatable_add_opts which can be directly used
in conjunction with qemu_opts_foreach.
- user_creatable_del - takes an ID and deletes the
corresponding object
The existing code is updated to use these new methods.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-2-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp.c | 52 +++---------
include/monitor/monitor.h | 3 -
include/qom/object_interfaces.h | 92 +++++++++++++++++++++
qmp.c | 76 ++----------------
qom/object_interfaces.c | 174 ++++++++++++++++++++++++++++++++++++++++
vl.c | 68 ++--------------
6 files changed, 290 insertions(+), 175 deletions(-)
diff --git a/hmp.c b/hmp.c
index c6419da..41fb9ca 100644
--- a/hmp.c
+++ b/hmp.c
@@ -30,6 +30,7 @@
#include "qapi/string-output-visitor.h"
#include "qapi/util.h"
#include "qapi-visit.h"
+#include "qom/object_interfaces.h"
#include "ui/console.h"
#include "block/qapi.h"
#include "qemu-io.h"
@@ -1655,58 +1656,27 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
void hmp_object_add(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
- Error *err_end = NULL;
QemuOpts *opts;
- char *type = NULL;
- char *id = NULL;
OptsVisitor *ov;
- QDict *pdict;
- Visitor *v;
+ Object *obj = NULL;
opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
if (err) {
- goto out;
+ hmp_handle_error(mon, &err);
+ return;
}
ov = opts_visitor_new(opts);
- pdict = qdict_clone_shallow(qdict);
- v = opts_get_visitor(ov);
-
- visit_start_struct(v, NULL, NULL, 0, &err);
- if (err) {
- goto out_clean;
- }
-
- qdict_del(pdict, "qom-type");
- visit_type_str(v, "qom-type", &type, &err);
- if (err) {
- goto out_end;
- }
+ obj = user_creatable_add(qdict, opts_get_visitor(ov), &err);
+ opts_visitor_cleanup(ov);
+ qemu_opts_del(opts);
- qdict_del(pdict, "id");
- visit_type_str(v, "id", &id, &err);
if (err) {
- goto out_end;
+ hmp_handle_error(mon, &err);
}
-
- object_add(type, id, pdict, v, &err);
-
-out_end:
- visit_end_struct(v, &err_end);
- if (!err && err_end) {
- qmp_object_del(id, NULL);
+ if (obj) {
+ object_unref(obj);
}
- error_propagate(&err, err_end);
-out_clean:
- opts_visitor_cleanup(ov);
-
- QDECREF(pdict);
- qemu_opts_del(opts);
- g_free(id);
- g_free(type);
-
-out:
- hmp_handle_error(mon, &err);
}
void hmp_getfd(Monitor *mon, const QDict *qdict)
@@ -1934,7 +1904,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
const char *id = qdict_get_str(qdict, "id");
Error *err = NULL;
- qmp_object_del(id, &err);
+ user_creatable_del(id, &err);
hmp_handle_error(mon, &err);
}
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 91b95ae..aa0f373 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -43,9 +43,6 @@ void monitor_read_command(Monitor *mon, int show_prompt);
int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
void *opaque);
-void object_add(const char *type, const char *id, const QDict *qdict,
- Visitor *v, Error **errp);
-
AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
bool has_opaque, const char *opaque,
Error **errp);
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 283ae0d..d579746 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -2,6 +2,8 @@
#define OBJECT_INTERFACES_H
#include "qom/object.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/visitor.h"
#define TYPE_USER_CREATABLE "user-creatable"
@@ -72,4 +74,94 @@ void user_creatable_complete(Object *obj, Error **errp);
* from implements USER_CREATABLE interface.
*/
bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
+
+/**
+ * user_creatable_add:
+ * @qdict: the object definition
+ * @v: the visitor
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object whose type
+ * is defined in @qdict by the 'qom-type' field, placing it
+ * in the object composition tree with name provided by the
+ * 'id' field. The remaining fields in @qdict are used to
+ * initialize the object properties.
+ *
+ * Returns: the newly created object or NULL on error
+ */
+Object *user_creatable_add(const QDict *qdict,
+ Visitor *v, Error **errp);
+
+/**
+ * user_creatable_add_type:
+ * @type: the object type name
+ * @id: the unique ID for the object
+ * @qdict: the object properties
+ * @v: the visitor
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object @type, placing
+ * it in the object composition tree with name @id, initializing
+ * it with properties from @qdict
+ *
+ * Returns: the newly created object or NULL on error
+ */
+Object *user_creatable_add_type(const char *type, const char *id,
+ const QDict *qdict,
+ Visitor *v, Error **errp);
+
+/**
+ * user_creatable_add_opts:
+ * @opts: the object definition
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object whose type
+ * is defined in @opts by the 'qom-type' option, placing it
+ * in the object composition tree with name provided by the
+ * 'id' field. The remaining options in @opts are used to
+ * initialize the object properties.
+ *
+ * Returns: the newly created object or NULL on error
+ */
+Object *user_creatable_add_opts(QemuOpts *opts, Error **errp);
+
+
+/**
+ * user_creatable_add_opts_predicate:
+ * @type: the QOM type to be added
+ *
+ * A callback function to determine whether an object
+ * of type @type should be created. Instances of this
+ * callback should be passed to user_creatable_add_opts_foreach
+ */
+typedef bool (*user_creatable_add_opts_predicate)(const char *type);
+
+/**
+ * user_creatable_add_opts_foreach:
+ * @opaque: a user_creatable_add_opts_predicate callback or NULL
+ * @opts: options to create
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * An iterator callback to be used in conjunction with
+ * the qemu_opts_foreach() method for creating a list of
+ * objects from a set of QemuOpts
+ *
+ * The @opaque parameter can be passed a user_creatable_add_opts_predicate
+ * callback to filter which types of object are created during iteration.
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int user_creatable_add_opts_foreach(void *opaque,
+ QemuOpts *opts, Error **errp);
+
+/**
+ * user_creatable_del:
+ * @id: the unique ID for the object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Delete an instance of the user creatable object identified
+ * by @id.
+ */
+void user_creatable_del(const char *id, Error **errp);
+
#endif
diff --git a/qmp.c b/qmp.c
index 6ae7230..9a93d5e 100644
--- a/qmp.c
+++ b/qmp.c
@@ -633,65 +633,13 @@ void qmp_add_client(const char *protocol, const char *fdname,
close(fd);
}
-void object_add(const char *type, const char *id, const QDict *qdict,
- Visitor *v, Error **errp)
-{
- Object *obj;
- ObjectClass *klass;
- const QDictEntry *e;
- Error *local_err = NULL;
-
- klass = object_class_by_name(type);
- if (!klass) {
- error_setg(errp, "invalid object type: %s", type);
- return;
- }
-
- if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
- error_setg(errp, "object type '%s' isn't supported by object-add",
- type);
- return;
- }
-
- if (object_class_is_abstract(klass)) {
- error_setg(errp, "object type '%s' is abstract", type);
- return;
- }
-
- obj = object_new(type);
- if (qdict) {
- for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
- object_property_set(obj, v, e->key, &local_err);
- if (local_err) {
- goto out;
- }
- }
- }
-
- object_property_add_child(object_get_objects_root(),
- id, obj, &local_err);
- if (local_err) {
- goto out;
- }
-
- user_creatable_complete(obj, &local_err);
- if (local_err) {
- object_property_del(object_get_objects_root(),
- id, &error_abort);
- goto out;
- }
-out:
- if (local_err) {
- error_propagate(errp, local_err);
- }
- object_unref(obj);
-}
void qmp_object_add(const char *type, const char *id,
bool has_props, QObject *props, Error **errp)
{
const QDict *pdict = NULL;
QmpInputVisitor *qiv;
+ Object *obj;
if (props) {
pdict = qobject_to_qdict(props);
@@ -702,27 +650,17 @@ void qmp_object_add(const char *type, const char *id,
}
qiv = qmp_input_visitor_new(props);
- object_add(type, id, pdict, qmp_input_get_visitor(qiv), errp);
+ obj = user_creatable_add_type(type, id, pdict,
+ qmp_input_get_visitor(qiv), errp);
qmp_input_visitor_cleanup(qiv);
+ if (obj) {
+ object_unref(obj);
+ }
}
void qmp_object_del(const char *id, Error **errp)
{
- Object *container;
- Object *obj;
-
- container = object_get_objects_root();
- obj = object_resolve_path_component(container, id);
- if (!obj) {
- error_setg(errp, "object id not found");
- return;
- }
-
- if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) {
- error_setg(errp, "%s is in use, can not be deleted", id);
- return;
- }
- object_unparent(obj);
+ user_creatable_del(id, errp);
}
MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index f1218f0..c2f6e29 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -1,6 +1,9 @@
#include "qemu/osdep.h"
#include "qom/object_interfaces.h"
#include "qemu/module.h"
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qapi/opts-visitor.h"
void user_creatable_complete(Object *obj, Error **errp)
{
@@ -31,6 +34,177 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
}
}
+
+Object *user_creatable_add(const QDict *qdict,
+ Visitor *v, Error **errp)
+{
+ char *type = NULL;
+ char *id = NULL;
+ Object *obj = NULL;
+ Error *local_err = NULL, *end_err = NULL;
+ QDict *pdict;
+
+ pdict = qdict_clone_shallow(qdict);
+
+ visit_start_struct(v, NULL, NULL, 0, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "qom-type");
+ visit_type_str(v, "qom-type", &type, &local_err);
+ if (local_err) {
+ goto out_visit;
+ }
+
+ qdict_del(pdict, "id");
+ visit_type_str(v, "id", &id, &local_err);
+ if (local_err) {
+ goto out_visit;
+ }
+
+ obj = user_creatable_add_type(type, id, pdict, v, &local_err);
+ if (local_err) {
+ goto out_visit;
+ }
+
+ out_visit:
+ visit_end_struct(v, &end_err);
+ if (end_err) {
+ error_propagate(&local_err, end_err);
+ if (obj) {
+ user_creatable_del(id, NULL);
+ }
+ goto out;
+ }
+
+out:
+ QDECREF(pdict);
+ g_free(id);
+ g_free(type);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ object_unref(obj);
+ return NULL;
+ }
+ return obj;
+}
+
+
+Object *user_creatable_add_type(const char *type, const char *id,
+ const QDict *qdict,
+ Visitor *v, Error **errp)
+{
+ Object *obj;
+ ObjectClass *klass;
+ const QDictEntry *e;
+ Error *local_err = NULL;
+
+ klass = object_class_by_name(type);
+ if (!klass) {
+ error_setg(errp, "invalid object type: %s", type);
+ return NULL;
+ }
+
+ if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
+ error_setg(errp, "object type '%s' isn't supported by object-add",
+ type);
+ return NULL;
+ }
+
+ if (object_class_is_abstract(klass)) {
+ error_setg(errp, "object type '%s' is abstract", type);
+ return NULL;
+ }
+
+ obj = object_new(type);
+ if (qdict) {
+ for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+ object_property_set(obj, v, e->key, &local_err);
+ if (local_err) {
+ goto out;
+ }
+ }
+ }
+
+ object_property_add_child(object_get_objects_root(),
+ id, obj, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
+ user_creatable_complete(obj, &local_err);
+ if (local_err) {
+ object_property_del(object_get_objects_root(),
+ id, &error_abort);
+ goto out;
+ }
+out:
+ if (local_err) {
+ error_propagate(errp, local_err);
+ object_unref(obj);
+ return NULL;
+ }
+ return obj;
+}
+
+
+Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
+{
+ OptsVisitor *ov;
+ QDict *pdict;
+ Object *obj = NULL;
+
+ ov = opts_visitor_new(opts);
+ pdict = qemu_opts_to_qdict(opts, NULL);
+
+ obj = user_creatable_add(pdict, opts_get_visitor(ov), errp);
+ opts_visitor_cleanup(ov);
+ QDECREF(pdict);
+ return obj;
+}
+
+
+int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
+{
+ bool (*type_predicate)(const char *) = opaque;
+ Object *obj = NULL;
+ const char *type;
+
+ type = qemu_opt_get(opts, "qom-type");
+ if (type && type_predicate &&
+ !type_predicate(type)) {
+ return 0;
+ }
+
+ obj = user_creatable_add_opts(opts, errp);
+ if (!obj) {
+ return -1;
+ }
+ object_unref(obj);
+ return 0;
+}
+
+
+void user_creatable_del(const char *id, Error **errp)
+{
+ Object *container;
+ Object *obj;
+
+ container = object_get_objects_root();
+ obj = object_resolve_path_component(container, id);
+ if (!obj) {
+ error_setg(errp, "object '%s' not found", id);
+ return;
+ }
+
+ if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) {
+ error_setg(errp, "object '%s' is in use, can not be deleted", id);
+ return;
+ }
+ object_unparent(obj);
+}
+
static void register_types(void)
{
static const TypeInfo uc_interface_info = {
diff --git a/vl.c b/vl.c
index 2e0116e..18e6086 100644
--- a/vl.c
+++ b/vl.c
@@ -2830,64 +2830,6 @@ static bool object_create_delayed(const char *type)
}
-static int object_create(void *opaque, QemuOpts *opts, Error **errp)
-{
- Error *err = NULL;
- Error *err_end = NULL;
- char *type = NULL;
- char *id = NULL;
- OptsVisitor *ov;
- QDict *pdict;
- bool (*type_predicate)(const char *) = opaque;
- Visitor *v;
-
- ov = opts_visitor_new(opts);
- pdict = qemu_opts_to_qdict(opts, NULL);
- v = opts_get_visitor(ov);
-
- visit_start_struct(v, NULL, NULL, 0, &err);
- if (err) {
- goto out;
- }
-
- qdict_del(pdict, "qom-type");
- visit_type_str(v, "qom-type", &type, &err);
- if (err) {
- goto out;
- }
- if (!type_predicate(type)) {
- visit_end_struct(v, NULL);
- goto out;
- }
-
- qdict_del(pdict, "id");
- visit_type_str(v, "id", &id, &err);
- if (err) {
- goto out_end;
- }
-
- object_add(type, id, pdict, v, &err);
-
-out_end:
- visit_end_struct(v, &err_end);
- if (!err && err_end) {
- qmp_object_del(id, NULL);
- }
- error_propagate(&err, err_end);
-
-out:
- opts_visitor_cleanup(ov);
-
- QDECREF(pdict);
- g_free(id);
- g_free(type);
- if (err) {
- error_report_err(err);
- return -1;
- }
- return 0;
-}
-
static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
MachineClass *mc)
{
@@ -4313,8 +4255,9 @@ int main(int argc, char **argv, char **envp)
socket_init();
if (qemu_opts_foreach(qemu_find_opts("object"),
- object_create,
- object_create_initial, NULL)) {
+ user_creatable_add_opts_foreach,
+ object_create_initial, &err)) {
+ error_report_err(err);
exit(1);
}
@@ -4431,8 +4374,9 @@ int main(int argc, char **argv, char **envp)
}
if (qemu_opts_foreach(qemu_find_opts("object"),
- object_create,
- object_create_delayed, NULL)) {
+ user_creatable_add_opts_foreach,
+ object_create_delayed, &err)) {
+ error_report_err(err);
exit(1);
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 14/28] qemu-nbd: add support for --object command line arg
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (12 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 13/28] qom: add helpers for UserCreatable object types Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 15/28] nbd: convert block client to use I/O channels for connection setup Paolo Bonzini
` (14 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
Allow creation of user creatable object types with qemu-nbd
via a new --object command line arg. This will be used to supply
passwords and/or encryption keys to the various block driver
backends via the recently added 'secret' object type.
# printf letmein > mypasswd.txt
# qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
...other nbd args...
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-3-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-nbd.c | 34 ++++++++++++++++++++++++++++++++++
qemu-nbd.texi | 6 ++++++
2 files changed, 40 insertions(+)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d374015..130c306 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -24,9 +24,11 @@
#include "qemu/main-loop.h"
#include "qemu/sockets.h"
#include "qemu/error-report.h"
+#include "qemu/config-file.h"
#include "block/snapshot.h"
#include "qapi/util.h"
#include "qapi/qmp/qstring.h"
+#include "qom/object_interfaces.h"
#include <getopt.h>
#include <sys/socket.h>
@@ -41,6 +43,7 @@
#define QEMU_NBD_OPT_AIO 2
#define QEMU_NBD_OPT_DISCARD 3
#define QEMU_NBD_OPT_DETECT_ZEROES 4
+#define QEMU_NBD_OPT_OBJECT 5
static NBDExport *exp;
static int verbose;
@@ -74,6 +77,9 @@ static void usage(const char *name)
" -o, --offset=OFFSET offset into the image\n"
" -P, --partition=NUM only expose partition NUM\n"
"\n"
+"General purpose options:\n"
+" --object type,id=ID,... define an object such as 'secret' for providing\n"
+" passwords and/or encryption keys\n"
#ifdef __linux__
"Kernel NBD client support:\n"
" -c, --connect=DEV connect FILE to the local NBD device DEV\n"
@@ -371,6 +377,16 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
}
+static QemuOptsList qemu_object_opts = {
+ .name = "object",
+ .implied_opt_name = "qom-type",
+ .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+ .desc = {
+ { }
+ },
+};
+
+
int main(int argc, char **argv)
{
BlockBackend *blk;
@@ -408,6 +424,7 @@ int main(int argc, char **argv)
{ "format", 1, NULL, 'f' },
{ "persistent", 0, NULL, 't' },
{ "verbose", 0, NULL, 'v' },
+ { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
{ NULL, 0, NULL, 0 }
};
int ch;
@@ -433,6 +450,8 @@ int main(int argc, char **argv)
memset(&sa_sigterm, 0, sizeof(sa_sigterm));
sa_sigterm.sa_handler = termsig_handler;
sigaction(SIGTERM, &sa_sigterm, NULL);
+ module_call_init(MODULE_INIT_QOM);
+ qemu_add_opts(&qemu_object_opts);
qemu_init_exec_dir(argv[0]);
while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
@@ -588,6 +607,14 @@ int main(int argc, char **argv)
case '?':
error_report("Try `%s --help' for more information.", argv[0]);
exit(EXIT_FAILURE);
+ case QEMU_NBD_OPT_OBJECT: {
+ QemuOpts *opts;
+ opts = qemu_opts_parse_noisily(&qemu_object_opts,
+ optarg, true);
+ if (!opts) {
+ exit(EXIT_FAILURE);
+ }
+ } break;
}
}
@@ -597,6 +624,13 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
+ if (qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, &local_err)) {
+ error_report_err(local_err);
+ exit(EXIT_FAILURE);
+ }
+
if (disconnect) {
fd = open(argv[optind], O_RDWR);
if (fd < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 0027841..a56ebc3 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -18,6 +18,12 @@ Export a QEMU disk image using the NBD protocol.
@var{dev} is an NBD device.
@table @option
+@item --object type,id=@var{id},...props...
+Define a new instance of the @var{type} object class identified by @var{id}.
+See the @code{qemu(1)} manual page for full details of the properties
+supported. The common object type that it makes sense to define is the
+@code{secret} object, which is used to supply passwords and/or encryption
+keys.
@item -p, --port=@var{port}
The TCP port to listen on (default @samp{10809})
@item -o, --offset=@var{offset}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 15/28] nbd: convert block client to use I/O channels for connection setup
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (13 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 14/28] qemu-nbd: add support for --object command line arg Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 16/28] nbd: convert qemu-nbd server " Paolo Bonzini
` (13 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This converts the NBD block driver client to use the QIOChannelSocket
class for initial connection setup. The NbdClientSession struct has
two pointers, one to the master QIOChannelSocket providing the raw
data channel, and one to a QIOChannel which is the current channel
used for I/O. Initially the two point to the same object, but when
TLS support is added, they will point to different objects.
The qemu-img & qemu-io tools now need to use MODULE_INIT_QOM to
ensure the QIOChannel object classes are registered. The qemu-nbd
tool already did this.
In this initial conversion though, all I/O is still actually done
using the raw POSIX sockets APIs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-4-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Makefile | 6 ++---
block/nbd-client.c | 76 +++++++++++++++++++++++++++++++++---------------------
block/nbd-client.h | 8 ++++--
block/nbd.c | 39 ++++++++++++++--------------
qemu-img.c | 1 +
qemu-io.c | 1 +
tests/Makefile | 2 +-
7 files changed, 79 insertions(+), 54 deletions(-)
diff --git a/Makefile b/Makefile
index f9fae3a..16db398 100644
--- a/Makefile
+++ b/Makefile
@@ -234,9 +234,9 @@ util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
qemu-img.o: qemu-img-cmds.h
-qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
-qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
-qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 568c56c..c4379a9 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -28,7 +28,6 @@
#include "qemu/osdep.h"
#include "nbd-client.h"
-#include "qemu/sockets.h"
#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
#define INDEX_TO_HANDLE(bs, index) ((index) ^ ((uint64_t)(intptr_t)bs))
@@ -48,13 +47,21 @@ static void nbd_teardown_connection(BlockDriverState *bs)
{
NbdClientSession *client = nbd_get_client_session(bs);
+ if (!client->ioc) { /* Already closed */
+ return;
+ }
+
/* finish any pending coroutines */
- shutdown(client->sock, 2);
+ qio_channel_shutdown(client->ioc,
+ QIO_CHANNEL_SHUTDOWN_BOTH,
+ NULL);
nbd_recv_coroutines_enter_all(client);
nbd_client_detach_aio_context(bs);
- closesocket(client->sock);
- client->sock = -1;
+ object_unref(OBJECT(client->sioc));
+ client->sioc = NULL;
+ object_unref(OBJECT(client->ioc));
+ client->ioc = NULL;
}
static void nbd_reply_ready(void *opaque)
@@ -64,12 +71,16 @@ static void nbd_reply_ready(void *opaque)
uint64_t i;
int ret;
+ if (!s->ioc) { /* Already closed */
+ return;
+ }
+
if (s->reply.handle == 0) {
/* No reply already in flight. Fetch a header. It is possible
* that another thread has done the same thing in parallel, so
* the socket is not readable anymore.
*/
- ret = nbd_receive_reply(s->sock, &s->reply);
+ ret = nbd_receive_reply(s->sioc->fd, &s->reply);
if (ret == -EAGAIN) {
return;
}
@@ -122,30 +133,32 @@ static int nbd_co_send_request(BlockDriverState *bs,
assert(i < MAX_NBD_REQUESTS);
request->handle = INDEX_TO_HANDLE(s, i);
+
+ if (!s->ioc) {
+ qemu_co_mutex_unlock(&s->send_mutex);
+ return -EPIPE;
+ }
+
s->send_coroutine = qemu_coroutine_self();
aio_context = bdrv_get_aio_context(bs);
- aio_set_fd_handler(aio_context, s->sock, false,
+ aio_set_fd_handler(aio_context, s->sioc->fd, false,
nbd_reply_ready, nbd_restart_write, bs);
if (qiov) {
- if (!s->is_unix) {
- socket_set_cork(s->sock, 1);
- }
- rc = nbd_send_request(s->sock, request);
+ qio_channel_set_cork(s->ioc, true);
+ rc = nbd_send_request(s->sioc->fd, request);
if (rc >= 0) {
- ret = qemu_co_sendv(s->sock, qiov->iov, qiov->niov,
+ ret = qemu_co_sendv(s->sioc->fd, qiov->iov, qiov->niov,
offset, request->len);
if (ret != request->len) {
rc = -EIO;
}
}
- if (!s->is_unix) {
- socket_set_cork(s->sock, 0);
- }
+ qio_channel_set_cork(s->ioc, false);
} else {
- rc = nbd_send_request(s->sock, request);
+ rc = nbd_send_request(s->sioc->fd, request);
}
- aio_set_fd_handler(aio_context, s->sock, false,
+ aio_set_fd_handler(aio_context, s->sioc->fd, false,
nbd_reply_ready, NULL, bs);
s->send_coroutine = NULL;
qemu_co_mutex_unlock(&s->send_mutex);
@@ -162,11 +175,12 @@ static void nbd_co_receive_reply(NbdClientSession *s,
* peek at the next reply and avoid yielding if it's ours? */
qemu_coroutine_yield();
*reply = s->reply;
- if (reply->handle != request->handle) {
+ if (reply->handle != request->handle ||
+ !s->ioc) {
reply->error = EIO;
} else {
if (qiov && reply->error == 0) {
- ret = qemu_co_recvv(s->sock, qiov->iov, qiov->niov,
+ ret = qemu_co_recvv(s->sioc->fd, qiov->iov, qiov->niov,
offset, request->len);
if (ret != request->len) {
reply->error = EIO;
@@ -350,14 +364,14 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
void nbd_client_detach_aio_context(BlockDriverState *bs)
{
aio_set_fd_handler(bdrv_get_aio_context(bs),
- nbd_get_client_session(bs)->sock,
+ nbd_get_client_session(bs)->sioc->fd,
false, NULL, NULL, NULL);
}
void nbd_client_attach_aio_context(BlockDriverState *bs,
AioContext *new_context)
{
- aio_set_fd_handler(new_context, nbd_get_client_session(bs)->sock,
+ aio_set_fd_handler(new_context, nbd_get_client_session(bs)->sioc->fd,
false, nbd_reply_ready, NULL, bs);
}
@@ -370,39 +384,43 @@ void nbd_client_close(BlockDriverState *bs)
.len = 0
};
- if (client->sock == -1) {
+ if (client->ioc == NULL) {
return;
}
- nbd_send_request(client->sock, &request);
+ nbd_send_request(client->sioc->fd, &request);
nbd_teardown_connection(bs);
}
-int nbd_client_init(BlockDriverState *bs, int sock, const char *export,
- Error **errp)
+int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
+ const char *export, Error **errp)
{
NbdClientSession *client = nbd_get_client_session(bs);
int ret;
/* NBD handshake */
logout("session init %s\n", export);
- qemu_set_block(sock);
- ret = nbd_receive_negotiate(sock, export,
+ qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
+
+ ret = nbd_receive_negotiate(sioc->fd, export,
&client->nbdflags, &client->size, errp);
if (ret < 0) {
logout("Failed to negotiate with the NBD server\n");
- closesocket(sock);
return ret;
}
qemu_co_mutex_init(&client->send_mutex);
qemu_co_mutex_init(&client->free_sema);
- client->sock = sock;
+ client->sioc = sioc;
+ object_ref(OBJECT(client->sioc));
+ client->ioc = QIO_CHANNEL(sioc);
+ object_ref(OBJECT(client->ioc));
/* Now that we're connected, set the socket to be non-blocking and
* kick the reply mechanism. */
- qemu_set_nonblock(sock);
+ qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
+
nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
logout("Established connection with NBD server\n");
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e841340..e8b3283 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -4,6 +4,7 @@
#include "qemu-common.h"
#include "block/nbd.h"
#include "block/block_int.h"
+#include "io/channel-socket.h"
/* #define DEBUG_NBD */
@@ -17,7 +18,8 @@
#define MAX_NBD_REQUESTS 16
typedef struct NbdClientSession {
- int sock;
+ QIOChannelSocket *sioc; /* The master data channel */
+ QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
uint32_t nbdflags;
off_t size;
@@ -34,7 +36,9 @@ typedef struct NbdClientSession {
NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
-int nbd_client_init(BlockDriverState *bs, int sock, const char *export_name,
+int nbd_client_init(BlockDriverState *bs,
+ QIOChannelSocket *sock,
+ const char *export_name,
Error **errp);
void nbd_client_close(BlockDriverState *bs);
diff --git a/block/nbd.c b/block/nbd.c
index 1a90bc7..d7116e2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -31,7 +31,6 @@
#include "qemu/uri.h"
#include "block/block_int.h"
#include "qemu/module.h"
-#include "qemu/sockets.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qjson.h"
#include "qapi/qmp/qint.h"
@@ -238,25 +237,25 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
return &s->client;
}
-static int nbd_establish_connection(BlockDriverState *bs,
- SocketAddress *saddr,
- Error **errp)
+static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+ Error **errp)
{
- BDRVNBDState *s = bs->opaque;
- int sock;
+ QIOChannelSocket *sioc;
+ Error *local_err = NULL;
- sock = socket_connect(saddr, errp, NULL, NULL);
+ sioc = qio_channel_socket_new();
- if (sock < 0) {
- logout("Failed to establish connection to NBD server\n");
- return -EIO;
+ qio_channel_socket_connect_sync(sioc,
+ saddr,
+ &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return NULL;
}
- if (!s->client.is_unix) {
- socket_set_nodelay(sock);
- }
+ qio_channel_set_delay(QIO_CHANNEL(sioc), false);
- return sock;
+ return sioc;
}
static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
@@ -264,7 +263,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
{
BDRVNBDState *s = bs->opaque;
char *export = NULL;
- int result, sock;
+ int result;
+ QIOChannelSocket *sioc;
SocketAddress *saddr;
/* Pop the config into our state object. Exit if invalid. */
@@ -276,15 +276,16 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
/* establish TCP connection, return error if it fails
* TODO: Configurable retry-until-timeout behaviour.
*/
- sock = nbd_establish_connection(bs, saddr, errp);
+ sioc = nbd_establish_connection(saddr, errp);
qapi_free_SocketAddress(saddr);
- if (sock < 0) {
+ if (!sioc) {
g_free(export);
- return sock;
+ return -ECONNREFUSED;
}
/* NBD handshake */
- result = nbd_client_init(bs, sock, export, errp);
+ result = nbd_client_init(bs, sioc, export, errp);
+ object_unref(OBJECT(sioc));
g_free(export);
return result;
}
diff --git a/qemu-img.c b/qemu-img.c
index 163d8c1..7030107 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3104,6 +3104,7 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
+ module_call_init(MODULE_INIT_QOM);
bdrv_init();
if (argc < 2) {
error_exit("Not enough arguments");
diff --git a/qemu-io.c b/qemu-io.c
index 83c48f4..6c0c028 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -394,6 +394,7 @@ int main(int argc, char **argv)
progname = basename(argv[0]);
qemu_init_exec_dir(argv[0]);
+ module_call_init(MODULE_INIT_QOM);
bdrv_init();
while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
diff --git a/tests/Makefile b/tests/Makefile
index 650e654..c1c605f 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -390,8 +390,8 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
tests/test-qapi-event.o tests/test-qmp-introspect.o \
$(test-qom-obj-y)
test-crypto-obj-y = $(crypto-obj-y) $(test-qom-obj-y)
-test-block-obj-y = $(block-obj-y) $(test-crypto-obj-y)
test-io-obj-y = $(io-obj-y) $(test-crypto-obj-y)
+test-block-obj-y = $(block-obj-y) $(test-io-obj-y)
tests/check-qint$(EXESUF): tests/check-qint.o $(test-util-obj-y)
tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 16/28] nbd: convert qemu-nbd server to use I/O channels for connection setup
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (14 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 15/28] nbd: convert block client to use I/O channels for connection setup Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 17/28] nbd: convert blockdev NBD " Paolo Bonzini
` (12 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This converts the qemu-nbd server to use the QIOChannelSocket
class for initial listener socket setup and accepting of client
connections. Actual I/O is still being performed against the
socket file descriptor using the POSIX socket APIs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-5-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-nbd.c | 91 ++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 53 insertions(+), 38 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 130c306..bc309e0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -22,19 +22,17 @@
#include "block/block_int.h"
#include "block/nbd.h"
#include "qemu/main-loop.h"
-#include "qemu/sockets.h"
#include "qemu/error-report.h"
#include "qemu/config-file.h"
#include "block/snapshot.h"
#include "qapi/util.h"
#include "qapi/qmp/qstring.h"
#include "qom/object_interfaces.h"
+#include "io/channel-socket.h"
#include <getopt.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <netinet/tcp.h>
-#include <arpa/inet.h>
+#include <sys/types.h>
+#include <signal.h>
#include <libgen.h>
#include <pthread.h>
@@ -53,7 +51,8 @@ static int persistent = 0;
static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
static int shared = 1;
static int nb_fds;
-static int server_fd;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
static void usage(const char *name)
{
@@ -236,19 +235,21 @@ static void *nbd_client_thread(void *arg)
char *device = arg;
off_t size;
uint32_t nbdflags;
- int fd, sock;
+ QIOChannelSocket *sioc;
+ int fd;
int ret;
pthread_t show_parts_thread;
Error *local_error = NULL;
-
- sock = socket_connect(saddr, &local_error, NULL, NULL);
- if (sock < 0) {
+ sioc = qio_channel_socket_new();
+ if (qio_channel_socket_connect_sync(sioc,
+ saddr,
+ &local_error) < 0) {
error_report_err(local_error);
goto out;
}
- ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
+ ret = nbd_receive_negotiate(sioc->fd, NULL, &nbdflags,
&size, &local_error);
if (ret < 0) {
if (local_error) {
@@ -264,7 +265,7 @@ static void *nbd_client_thread(void *arg)
goto out_socket;
}
- ret = nbd_init(fd, sock, nbdflags, size);
+ ret = nbd_init(fd, sioc->fd, nbdflags, size);
if (ret < 0) {
goto out_fd;
}
@@ -285,13 +286,14 @@ static void *nbd_client_thread(void *arg)
goto out_fd;
}
close(fd);
+ object_unref(OBJECT(sioc));
kill(getpid(), SIGTERM);
return (void *) EXIT_SUCCESS;
out_fd:
close(fd);
out_socket:
- closesocket(sock);
+ object_unref(OBJECT(sioc));
out:
kill(getpid(), SIGTERM);
return (void *) EXIT_FAILURE;
@@ -308,7 +310,7 @@ static void nbd_export_closed(NBDExport *exp)
state = TERMINATED;
}
-static void nbd_update_server_fd_handler(int fd);
+static void nbd_update_server_watch(void);
static void nbd_client_closed(NBDClient *client)
{
@@ -316,37 +318,51 @@ static void nbd_client_closed(NBDClient *client)
if (nb_fds == 0 && !persistent && state == RUNNING) {
state = TERMINATE;
}
- nbd_update_server_fd_handler(server_fd);
+ nbd_update_server_watch();
nbd_client_put(client);
}
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
{
- struct sockaddr_in addr;
- socklen_t addr_len = sizeof(addr);
+ QIOChannelSocket *cioc;
+ int fd;
- int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
- if (fd < 0) {
- perror("accept");
- return;
+ cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+ NULL);
+ if (!cioc) {
+ return TRUE;
}
if (state >= TERMINATE) {
- close(fd);
- return;
+ object_unref(OBJECT(cioc));
+ return TRUE;
}
nb_fds++;
- nbd_update_server_fd_handler(server_fd);
- nbd_client_new(exp, fd, nbd_client_closed);
+ nbd_update_server_watch();
+ fd = dup(cioc->fd);
+ if (fd >= 0) {
+ nbd_client_new(exp, fd, nbd_client_closed);
+ }
+ object_unref(OBJECT(cioc));
+
+ return TRUE;
}
-static void nbd_update_server_fd_handler(int fd)
+static void nbd_update_server_watch(void)
{
if (nbd_can_accept()) {
- qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
+ if (server_watch == -1) {
+ server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+ G_IO_IN,
+ nbd_accept,
+ NULL, NULL);
+ }
} else {
- qemu_set_fd_handler(fd, NULL, NULL, NULL);
+ if (server_watch != -1) {
+ g_source_remove(server_watch);
+ server_watch = -1;
+ }
}
}
@@ -433,7 +449,6 @@ int main(int argc, char **argv)
int flags = BDRV_O_RDWR;
int partition = -1;
int ret = 0;
- int fd;
bool seen_cache = false;
bool seen_discard = false;
bool seen_aio = false;
@@ -632,15 +647,15 @@ int main(int argc, char **argv)
}
if (disconnect) {
- fd = open(argv[optind], O_RDWR);
- if (fd < 0) {
+ int nbdfd = open(argv[optind], O_RDWR);
+ if (nbdfd < 0) {
error_report("Cannot open %s: %s", argv[optind],
strerror(errno));
exit(EXIT_FAILURE);
}
- nbd_disconnect(fd);
+ nbd_disconnect(nbdfd);
- close(fd);
+ close(nbdfd);
printf("%s disconnected\n", argv[optind]);
@@ -773,8 +788,9 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
- fd = socket_listen(saddr, &local_err);
- if (fd < 0) {
+ 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;
}
@@ -792,8 +808,7 @@ int main(int argc, char **argv)
memset(&client_thread, 0, sizeof(client_thread));
}
- server_fd = fd;
- nbd_update_server_fd_handler(fd);
+ nbd_update_server_watch();
/* now when the initialization is (almost) complete, chdir("/")
* to free any busy filesystems */
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 17/28] nbd: convert blockdev NBD server to use I/O channels for connection setup
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (15 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 16/28] nbd: convert qemu-nbd server " Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 18/28] nbd: convert to using I/O channels for actual socket I/O Paolo Bonzini
` (11 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This converts the blockdev NBD server to use the QIOChannelSocket
class for initial listener socket setup and accepting of client
connections. Actual I/O is still being performed against the
socket file descriptor using the POSIX socket APIs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-6-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
blockdev-nbd.c | 49 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 34 insertions(+), 15 deletions(-)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index efc31a4..9baf883 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -18,32 +18,48 @@
#include "qmp-commands.h"
#include "trace.h"
#include "block/nbd.h"
-#include "qemu/sockets.h"
+#include "io/channel-socket.h"
-static int server_fd = -1;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
+ gpointer opaque)
{
- struct sockaddr_in addr;
- socklen_t addr_len = sizeof(addr);
+ QIOChannelSocket *cioc;
+ int fd;
- int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
+ cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+ NULL);
+ if (!cioc) {
+ return TRUE;
+ }
+
+ fd = dup(cioc->fd);
if (fd >= 0) {
nbd_client_new(NULL, fd, nbd_client_put);
}
+ object_unref(OBJECT(cioc));
+ return TRUE;
}
void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
{
- if (server_fd != -1) {
+ if (server_ioc) {
error_setg(errp, "NBD server already running");
return;
}
- server_fd = socket_listen(addr, errp);
- if (server_fd != -1) {
- qemu_set_fd_handler(server_fd, nbd_accept, NULL, NULL);
+ server_ioc = qio_channel_socket_new();
+ if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+ return;
}
+
+ server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+ G_IO_IN,
+ nbd_accept,
+ NULL,
+ NULL);
}
void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
@@ -52,7 +68,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
BlockBackend *blk;
NBDExport *exp;
- if (server_fd == -1) {
+ if (!server_ioc) {
error_setg(errp, "NBD server not running");
return;
}
@@ -98,9 +114,12 @@ void qmp_nbd_server_stop(Error **errp)
{
nbd_export_close_all();
- if (server_fd != -1) {
- qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
- close(server_fd);
- server_fd = -1;
+ if (server_watch != -1) {
+ g_source_remove(server_watch);
+ server_watch = -1;
+ }
+ if (server_ioc) {
+ object_unref(OBJECT(server_ioc));
+ server_ioc = NULL;
}
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 18/28] nbd: convert to using I/O channels for actual socket I/O
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (16 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 17/28] nbd: convert blockdev NBD " Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 19/28] nbd: invert client logic for negotiating protocol version Paolo Bonzini
` (10 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
Now that all callers are converted to use I/O channels for
initial connection setup, it is possible to switch the core
NBD protocol handling core over to use QIOChannel APIs for
actual sockets I/O.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-7-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd-client.c | 19 +++----
blockdev-nbd.c | 6 +--
include/block/nbd.h | 20 ++++---
nbd/client.c | 40 +++++++-------
nbd/common.c | 68 ++++++++++++++----------
nbd/nbd-internal.h | 18 +++----
nbd/server.c | 150 +++++++++++++++++++++++++++++-----------------------
qemu-nbd.c | 10 ++--
8 files changed, 180 insertions(+), 151 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index c4379a9..75bb2d9 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -80,7 +80,7 @@ static void nbd_reply_ready(void *opaque)
* that another thread has done the same thing in parallel, so
* the socket is not readable anymore.
*/
- ret = nbd_receive_reply(s->sioc->fd, &s->reply);
+ ret = nbd_receive_reply(s->ioc, &s->reply);
if (ret == -EAGAIN) {
return;
}
@@ -131,6 +131,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
}
}
+ g_assert(qemu_in_coroutine());
assert(i < MAX_NBD_REQUESTS);
request->handle = INDEX_TO_HANDLE(s, i);
@@ -146,17 +147,17 @@ static int nbd_co_send_request(BlockDriverState *bs,
nbd_reply_ready, nbd_restart_write, bs);
if (qiov) {
qio_channel_set_cork(s->ioc, true);
- rc = nbd_send_request(s->sioc->fd, request);
+ rc = nbd_send_request(s->ioc, request);
if (rc >= 0) {
- ret = qemu_co_sendv(s->sioc->fd, qiov->iov, qiov->niov,
- offset, request->len);
+ ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+ offset, request->len, 0);
if (ret != request->len) {
rc = -EIO;
}
}
qio_channel_set_cork(s->ioc, false);
} else {
- rc = nbd_send_request(s->sioc->fd, request);
+ rc = nbd_send_request(s->ioc, request);
}
aio_set_fd_handler(aio_context, s->sioc->fd, false,
nbd_reply_ready, NULL, bs);
@@ -180,8 +181,8 @@ static void nbd_co_receive_reply(NbdClientSession *s,
reply->error = EIO;
} else {
if (qiov && reply->error == 0) {
- ret = qemu_co_recvv(s->sioc->fd, qiov->iov, qiov->niov,
- offset, request->len);
+ ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+ offset, request->len, 1);
if (ret != request->len) {
reply->error = EIO;
}
@@ -388,7 +389,7 @@ void nbd_client_close(BlockDriverState *bs)
return;
}
- nbd_send_request(client->sioc->fd, &request);
+ nbd_send_request(client->ioc, &request);
nbd_teardown_connection(bs);
}
@@ -403,7 +404,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
logout("session init %s\n", export);
qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
- ret = nbd_receive_negotiate(sioc->fd, export,
+ ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
&client->nbdflags, &client->size, errp);
if (ret < 0) {
logout("Failed to negotiate with the NBD server\n");
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 9baf883..2148753 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -27,7 +27,6 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
gpointer opaque)
{
QIOChannelSocket *cioc;
- int fd;
cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
NULL);
@@ -35,10 +34,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
return TRUE;
}
- fd = dup(cioc->fd);
- if (fd >= 0) {
- nbd_client_new(NULL, fd, nbd_client_put);
- }
+ nbd_client_new(NULL, cioc, nbd_client_put);
object_unref(OBJECT(cioc));
return TRUE;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 7eccb41..1080ef8 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -23,6 +23,7 @@
#include "qemu-common.h"
#include "qemu/option.h"
+#include "io/channel-socket.h"
struct nbd_request {
uint32_t magic;
@@ -73,12 +74,17 @@ enum {
/* Maximum size of a single READ/WRITE data buffer */
#define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
-ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
+ssize_t nbd_wr_syncv(QIOChannel *ioc,
+ struct iovec *iov,
+ size_t niov,
+ size_t offset,
+ size_t length,
+ bool do_read);
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
off_t *size, Error **errp);
-int nbd_init(int fd, int csock, uint32_t flags, off_t size);
-ssize_t nbd_send_request(int csock, struct nbd_request *request);
-ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
+ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
+ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply);
int nbd_client(int fd);
int nbd_disconnect(int fd);
@@ -98,7 +104,9 @@ NBDExport *nbd_export_find(const char *name);
void nbd_export_set_name(NBDExport *exp, const char *name);
void nbd_export_close_all(void);
-void nbd_client_new(NBDExport *exp, int csock, void (*close_fn)(NBDClient *));
+void nbd_client_new(NBDExport *exp,
+ QIOChannelSocket *sioc,
+ void (*close)(NBDClient *));
void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
diff --git a/nbd/client.c b/nbd/client.c
index f07cb48..f6dd0a3 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -71,7 +71,7 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
*/
-int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
off_t *size, Error **errp)
{
char buf[256];
@@ -83,7 +83,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
rc = -EINVAL;
- if (read_sync(csock, buf, 8) != 8) {
+ if (read_sync(ioc, buf, 8) != 8) {
error_setg(errp, "Failed to read data");
goto fail;
}
@@ -109,7 +109,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
goto fail;
}
- if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
error_setg(errp, "Failed to read magic");
goto fail;
}
@@ -130,35 +130,35 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
}
goto fail;
}
- if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
error_setg(errp, "Failed to read server flags");
goto fail;
}
*flags = be16_to_cpu(tmp) << 16;
/* reserved for future use */
- if (write_sync(csock, &reserved, sizeof(reserved)) !=
+ if (write_sync(ioc, &reserved, sizeof(reserved)) !=
sizeof(reserved)) {
error_setg(errp, "Failed to read reserved field");
goto fail;
}
/* write the export name */
magic = cpu_to_be64(magic);
- if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
error_setg(errp, "Failed to send export name magic");
goto fail;
}
opt = cpu_to_be32(NBD_OPT_EXPORT_NAME);
- if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
+ if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
error_setg(errp, "Failed to send export name option number");
goto fail;
}
namesize = cpu_to_be32(strlen(name));
- if (write_sync(csock, &namesize, sizeof(namesize)) !=
+ if (write_sync(ioc, &namesize, sizeof(namesize)) !=
sizeof(namesize)) {
error_setg(errp, "Failed to send export name length");
goto fail;
}
- if (write_sync(csock, (char*)name, strlen(name)) != strlen(name)) {
+ if (write_sync(ioc, (char *)name, strlen(name)) != strlen(name)) {
error_setg(errp, "Failed to send export name");
goto fail;
}
@@ -175,7 +175,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
}
}
- if (read_sync(csock, &s, sizeof(s)) != sizeof(s)) {
+ if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
error_setg(errp, "Failed to read export length");
goto fail;
}
@@ -183,19 +183,19 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
TRACE("Size is %" PRIu64, *size);
if (!name) {
- if (read_sync(csock, flags, sizeof(*flags)) != sizeof(*flags)) {
+ if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
*flags = be32_to_cpup(flags);
} else {
- if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
*flags |= be16_to_cpu(tmp);
}
- if (read_sync(csock, &buf, 124) != 124) {
+ if (read_sync(ioc, &buf, 124) != 124) {
error_setg(errp, "Failed to read reserved block");
goto fail;
}
@@ -206,11 +206,11 @@ fail:
}
#ifdef __linux__
-int nbd_init(int fd, int csock, uint32_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size)
{
TRACE("Setting NBD socket");
- if (ioctl(fd, NBD_SET_SOCK, csock) < 0) {
+ if (ioctl(fd, NBD_SET_SOCK, sioc->fd) < 0) {
int serrno = errno;
LOG("Failed to set NBD socket");
return -serrno;
@@ -283,7 +283,7 @@ int nbd_client(int fd)
return ret;
}
#else
-int nbd_init(int fd, int csock, uint32_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *ioc, uint32_t flags, off_t size)
{
return -ENOTSUP;
}
@@ -294,7 +294,7 @@ int nbd_client(int fd)
}
#endif
-ssize_t nbd_send_request(int csock, struct nbd_request *request)
+ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
{
uint8_t buf[NBD_REQUEST_SIZE];
ssize_t ret;
@@ -309,7 +309,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
"{ .from = %" PRIu64", .len = %u, .handle = %" PRIu64", .type=%i}",
request->from, request->len, request->handle, request->type);
- ret = write_sync(csock, buf, sizeof(buf));
+ ret = write_sync(ioc, buf, sizeof(buf));
if (ret < 0) {
return ret;
}
@@ -321,13 +321,13 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
return 0;
}
-ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
+ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply)
{
uint8_t buf[NBD_REPLY_SIZE];
uint32_t magic;
ssize_t ret;
- ret = read_sync(csock, buf, sizeof(buf));
+ ret = read_sync(ioc, buf, sizeof(buf));
if (ret < 0) {
return ret;
}
diff --git a/nbd/common.c b/nbd/common.c
index 178d4a7..2b19c95 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -19,47 +19,59 @@
#include "qemu/osdep.h"
#include "nbd-internal.h"
-ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read)
+ssize_t nbd_wr_syncv(QIOChannel *ioc,
+ struct iovec *iov,
+ size_t niov,
+ size_t offset,
+ size_t length,
+ bool do_read)
{
- size_t offset = 0;
- int err;
+ ssize_t done = 0;
+ Error *local_err = NULL;
+ struct iovec *local_iov = g_new(struct iovec, niov);
+ struct iovec *local_iov_head = local_iov;
+ unsigned int nlocal_iov = niov;
- if (qemu_in_coroutine()) {
- if (do_read) {
- return qemu_co_recv(fd, buffer, size);
- } else {
- return qemu_co_send(fd, buffer, size);
- }
- }
+ nlocal_iov = iov_copy(local_iov, nlocal_iov,
+ iov, niov,
+ offset, length);
- while (offset < size) {
+ while (nlocal_iov > 0) {
ssize_t len;
-
if (do_read) {
- len = qemu_recv(fd, buffer + offset, size - offset, 0);
+ len = qio_channel_readv(ioc, local_iov, nlocal_iov, &local_err);
} else {
- len = send(fd, buffer + offset, size - offset, 0);
+ len = qio_channel_writev(ioc, local_iov, nlocal_iov, &local_err);
}
-
- if (len < 0) {
- err = socket_error();
-
- /* recoverable error */
- if (err == EINTR || (offset > 0 && (err == EAGAIN || err == EWOULDBLOCK))) {
- continue;
+ if (len == QIO_CHANNEL_ERR_BLOCK) {
+ if (qemu_in_coroutine()) {
+ /* XXX figure out if we can create a variant on
+ * qio_channel_yield() that works with AIO contexts
+ * and consider using that in this branch */
+ qemu_coroutine_yield();
+ } else {
+ qio_channel_wait(ioc,
+ do_read ? G_IO_IN : G_IO_OUT);
}
-
- /* unrecoverable error */
- return -err;
+ continue;
+ }
+ if (len < 0) {
+ TRACE("I/O error: %s", error_get_pretty(local_err));
+ error_free(local_err);
+ /* XXX handle Error objects */
+ done = -EIO;
+ goto cleanup;
}
- /* eof */
- if (len == 0) {
+ if (do_read && len == 0) {
break;
}
- offset += len;
+ iov_discard_front(&local_iov, &nlocal_iov, len);
+ done += len;
}
- return offset;
+ cleanup:
+ g_free(local_iov_head);
+ return done;
}
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index c0a6575..9960034 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -13,6 +13,7 @@
#include "sysemu/block-backend.h"
#include "qemu/coroutine.h"
+#include "qemu/iov.h"
#include <errno.h>
#include <string.h>
@@ -29,7 +30,6 @@
#include <linux/fs.h>
#endif
-#include "qemu/sockets.h"
#include "qemu/queue.h"
#include "qemu/main-loop.h"
@@ -90,24 +90,22 @@
#define NBD_EINVAL 22
#define NBD_ENOSPC 28
-static inline ssize_t read_sync(int fd, void *buffer, size_t size)
+static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
{
+ struct iovec iov = { .iov_base = buffer, .iov_len = size };
/* Sockets are kept in blocking mode in the negotiation phase. After
* that, a non-readable socket simply means that another thread stole
* our request/reply. Synchronization is done with recv_coroutine, so
* that this is coroutine-safe.
*/
- return nbd_wr_sync(fd, buffer, size, true);
+ return nbd_wr_syncv(ioc, &iov, 1, 0, size, true);
}
-static inline ssize_t write_sync(int fd, void *buffer, size_t size)
+static inline ssize_t write_sync(QIOChannel *ioc, void *buffer, size_t size)
{
- int ret;
- do {
- /* For writes, we do expect the socket to be writable. */
- ret = nbd_wr_sync(fd, buffer, size, false);
- } while (ret == -EAGAIN);
- return ret;
+ struct iovec iov = { .iov_base = buffer, .iov_len = size };
+
+ return nbd_wr_syncv(ioc, &iov, 1, 0, size, false);
}
#endif
diff --git a/nbd/server.c b/nbd/server.c
index dc1d66f..15aa03d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -76,7 +76,8 @@ struct NBDClient {
void (*close)(NBDClient *client);
NBDExport *exp;
- int sock;
+ QIOChannelSocket *sioc; /* The underlying data channel */
+ QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
Coroutine *recv_coroutine;
@@ -96,45 +97,56 @@ static void nbd_set_handlers(NBDClient *client);
static void nbd_unset_handlers(NBDClient *client);
static void nbd_update_can_read(NBDClient *client);
-static void nbd_negotiate_continue(void *opaque)
+static gboolean nbd_negotiate_continue(QIOChannel *ioc,
+ GIOCondition condition,
+ void *opaque)
{
qemu_coroutine_enter(opaque, NULL);
+ return TRUE;
}
-static ssize_t nbd_negotiate_read(int fd, void *buffer, size_t size)
+static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
{
ssize_t ret;
+ guint watch;
assert(qemu_in_coroutine());
/* Negotiation are always in main loop. */
- qemu_set_fd_handler(fd, nbd_negotiate_continue, NULL,
- qemu_coroutine_self());
- ret = read_sync(fd, buffer, size);
- qemu_set_fd_handler(fd, NULL, NULL, NULL);
+ watch = qio_channel_add_watch(ioc,
+ G_IO_IN,
+ nbd_negotiate_continue,
+ qemu_coroutine_self(),
+ NULL);
+ ret = read_sync(ioc, buffer, size);
+ g_source_remove(watch);
return ret;
}
-static ssize_t nbd_negotiate_write(int fd, void *buffer, size_t size)
+static ssize_t nbd_negotiate_write(QIOChannel *ioc, void *buffer, size_t size)
{
ssize_t ret;
+ guint watch;
assert(qemu_in_coroutine());
/* Negotiation are always in main loop. */
- qemu_set_fd_handler(fd, NULL, nbd_negotiate_continue,
- qemu_coroutine_self());
- ret = write_sync(fd, buffer, size);
- qemu_set_fd_handler(fd, NULL, NULL, NULL);
+ watch = qio_channel_add_watch(ioc,
+ G_IO_OUT,
+ nbd_negotiate_continue,
+ qemu_coroutine_self(),
+ NULL);
+ ret = write_sync(ioc, buffer, size);
+ g_source_remove(watch);
return ret;
}
-static ssize_t nbd_negotiate_drop_sync(int fd, size_t size)
+static ssize_t nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
{
ssize_t ret, dropped = size;
uint8_t *buffer = g_malloc(MIN(65536, size));
while (size > 0) {
- ret = nbd_negotiate_read(fd, buffer, MIN(65536, size));
+ ret = nbd_negotiate_read(ioc, buffer, MIN(65536, size));
if (ret < 0) {
g_free(buffer);
return ret;
@@ -175,66 +187,66 @@ static ssize_t nbd_negotiate_drop_sync(int fd, size_t size)
*/
-static int nbd_negotiate_send_rep(int csock, uint32_t type, uint32_t opt)
+static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
{
uint64_t magic;
uint32_t len;
magic = cpu_to_be64(NBD_REP_MAGIC);
- if (nbd_negotiate_write(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
LOG("write failed (rep magic)");
return -EINVAL;
}
opt = cpu_to_be32(opt);
- if (nbd_negotiate_write(csock, &opt, sizeof(opt)) != sizeof(opt)) {
+ if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
LOG("write failed (rep opt)");
return -EINVAL;
}
type = cpu_to_be32(type);
- if (nbd_negotiate_write(csock, &type, sizeof(type)) != sizeof(type)) {
+ if (nbd_negotiate_write(ioc, &type, sizeof(type)) != sizeof(type)) {
LOG("write failed (rep type)");
return -EINVAL;
}
len = cpu_to_be32(0);
- if (nbd_negotiate_write(csock, &len, sizeof(len)) != sizeof(len)) {
+ if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
LOG("write failed (rep data length)");
return -EINVAL;
}
return 0;
}
-static int nbd_negotiate_send_rep_list(int csock, NBDExport *exp)
+static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
{
uint64_t magic, name_len;
uint32_t opt, type, len;
name_len = strlen(exp->name);
magic = cpu_to_be64(NBD_REP_MAGIC);
- if (nbd_negotiate_write(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
LOG("write failed (magic)");
return -EINVAL;
}
opt = cpu_to_be32(NBD_OPT_LIST);
- if (nbd_negotiate_write(csock, &opt, sizeof(opt)) != sizeof(opt)) {
+ if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
LOG("write failed (opt)");
return -EINVAL;
}
type = cpu_to_be32(NBD_REP_SERVER);
- if (nbd_negotiate_write(csock, &type, sizeof(type)) != sizeof(type)) {
+ if (nbd_negotiate_write(ioc, &type, sizeof(type)) != sizeof(type)) {
LOG("write failed (reply type)");
return -EINVAL;
}
len = cpu_to_be32(name_len + sizeof(len));
- if (nbd_negotiate_write(csock, &len, sizeof(len)) != sizeof(len)) {
+ if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
LOG("write failed (length)");
return -EINVAL;
}
len = cpu_to_be32(name_len);
- if (nbd_negotiate_write(csock, &len, sizeof(len)) != sizeof(len)) {
+ if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
LOG("write failed (length)");
return -EINVAL;
}
- if (nbd_negotiate_write(csock, exp->name, name_len) != name_len) {
+ if (nbd_negotiate_write(ioc, exp->name, name_len) != name_len) {
LOG("write failed (buffer)");
return -EINVAL;
}
@@ -243,30 +255,29 @@ static int nbd_negotiate_send_rep_list(int csock, NBDExport *exp)
static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
{
- int csock;
NBDExport *exp;
- csock = client->sock;
if (length) {
- if (nbd_negotiate_drop_sync(csock, length) != length) {
+ if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
return -EIO;
}
- return nbd_negotiate_send_rep(csock, NBD_REP_ERR_INVALID, NBD_OPT_LIST);
+ return nbd_negotiate_send_rep(client->ioc,
+ NBD_REP_ERR_INVALID, NBD_OPT_LIST);
}
/* For each export, send a NBD_REP_SERVER reply. */
QTAILQ_FOREACH(exp, &exports, next) {
- if (nbd_negotiate_send_rep_list(csock, exp)) {
+ if (nbd_negotiate_send_rep_list(client->ioc, exp)) {
return -EINVAL;
}
}
/* Finish with a NBD_REP_ACK. */
- return nbd_negotiate_send_rep(csock, NBD_REP_ACK, NBD_OPT_LIST);
+ return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST);
}
static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
{
- int rc = -EINVAL, csock = client->sock;
+ int rc = -EINVAL;
char name[256];
/* Client sends:
@@ -277,7 +288,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
LOG("Bad length received");
goto fail;
}
- if (nbd_negotiate_read(csock, name, length) != length) {
+ if (nbd_negotiate_read(client->ioc, name, length) != length) {
LOG("read failed");
goto fail;
}
@@ -298,7 +309,6 @@ fail:
static int nbd_negotiate_options(NBDClient *client)
{
- int csock = client->sock;
uint32_t flags;
/* Client sends:
@@ -315,7 +325,8 @@ static int nbd_negotiate_options(NBDClient *client)
... Rest of request
*/
- if (nbd_negotiate_read(csock, &flags, sizeof(flags)) != sizeof(flags)) {
+ if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) !=
+ sizeof(flags)) {
LOG("read failed");
return -EIO;
}
@@ -331,7 +342,8 @@ static int nbd_negotiate_options(NBDClient *client)
uint32_t tmp, length;
uint64_t magic;
- if (nbd_negotiate_read(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+ if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) !=
+ sizeof(magic)) {
LOG("read failed");
return -EINVAL;
}
@@ -341,13 +353,13 @@ static int nbd_negotiate_options(NBDClient *client)
return -EINVAL;
}
- if (nbd_negotiate_read(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ if (nbd_negotiate_read(client->ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
LOG("read failed");
return -EINVAL;
}
- if (nbd_negotiate_read(csock, &length,
- sizeof(length)) != sizeof(length)) {
+ if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) !=
+ sizeof(length)) {
LOG("read failed");
return -EINVAL;
}
@@ -371,7 +383,7 @@ static int nbd_negotiate_options(NBDClient *client)
default:
tmp = be32_to_cpu(tmp);
LOG("Unsupported option 0x%x", tmp);
- nbd_negotiate_send_rep(client->sock, NBD_REP_ERR_UNSUP, tmp);
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP, tmp);
return -EINVAL;
}
}
@@ -385,7 +397,6 @@ typedef struct {
static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
{
NBDClient *client = data->client;
- int csock = client->sock;
char buf[8 + 8 + 8 + 128];
int rc;
const int myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
@@ -410,6 +421,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
[28 .. 151] reserved (0)
*/
+ qio_channel_set_blocking(client->ioc, false, NULL);
rc = -EINVAL;
TRACE("Beginning negotiation.");
@@ -426,12 +438,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
}
if (client->exp) {
- if (nbd_negotiate_write(csock, buf, sizeof(buf)) != sizeof(buf)) {
+ if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) != sizeof(buf)) {
LOG("write failed");
goto fail;
}
} else {
- if (nbd_negotiate_write(csock, buf, 18) != 18) {
+ if (nbd_negotiate_write(client->ioc, buf, 18) != 18) {
LOG("write failed");
goto fail;
}
@@ -444,8 +456,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
assert ((client->exp->nbdflags & ~65535) == 0);
stq_be_p(buf + 18, client->exp->size);
stw_be_p(buf + 26, client->exp->nbdflags | myflags);
- if (nbd_negotiate_write(csock, buf + 18,
- sizeof(buf) - 18) != sizeof(buf) - 18) {
+ if (nbd_negotiate_write(client->ioc, buf + 18, sizeof(buf) - 18) !=
+ sizeof(buf) - 18) {
LOG("write failed");
goto fail;
}
@@ -475,13 +487,13 @@ int nbd_disconnect(int fd)
}
#endif
-static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
+static ssize_t nbd_receive_request(QIOChannel *ioc, struct nbd_request *request)
{
uint8_t buf[NBD_REQUEST_SIZE];
uint32_t magic;
ssize_t ret;
- ret = read_sync(csock, buf, sizeof(buf));
+ ret = read_sync(ioc, buf, sizeof(buf));
if (ret < 0) {
return ret;
}
@@ -516,7 +528,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
return 0;
}
-static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
+static ssize_t nbd_send_reply(QIOChannel *ioc, struct nbd_reply *reply)
{
uint8_t buf[NBD_REPLY_SIZE];
ssize_t ret;
@@ -534,7 +546,7 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
TRACE("Sending response to client");
- ret = write_sync(csock, buf, sizeof(buf));
+ ret = write_sync(ioc, buf, sizeof(buf));
if (ret < 0) {
return ret;
}
@@ -562,8 +574,8 @@ void nbd_client_put(NBDClient *client)
assert(client->closing);
nbd_unset_handlers(client);
- close(client->sock);
- client->sock = -1;
+ object_unref(OBJECT(client->sioc));
+ object_unref(OBJECT(client->ioc));
if (client->exp) {
QTAILQ_REMOVE(&client->exp->clients, client, next);
nbd_export_put(client->exp);
@@ -583,7 +595,8 @@ static void client_close(NBDClient *client)
/* Force requests to finish. They will drop their own references,
* then we'll close the socket and free the NBDClient.
*/
- shutdown(client->sock, 2);
+ qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH,
+ NULL);
/* Also tell the client, so that they release their reference. */
if (client->close) {
@@ -789,25 +802,25 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct nbd_reply *reply,
int len)
{
NBDClient *client = req->client;
- int csock = client->sock;
ssize_t rc, ret;
+ g_assert(qemu_in_coroutine());
qemu_co_mutex_lock(&client->send_lock);
client->send_coroutine = qemu_coroutine_self();
nbd_set_handlers(client);
if (!len) {
- rc = nbd_send_reply(csock, reply);
+ rc = nbd_send_reply(client->ioc, reply);
} else {
- socket_set_cork(csock, 1);
- rc = nbd_send_reply(csock, reply);
+ qio_channel_set_cork(client->ioc, true);
+ rc = nbd_send_reply(client->ioc, reply);
if (rc >= 0) {
- ret = qemu_co_send(csock, req->data, len);
+ ret = write_sync(client->ioc, req->data, len);
if (ret != len) {
rc = -EIO;
}
}
- socket_set_cork(csock, 0);
+ qio_channel_set_cork(client->ioc, false);
}
client->send_coroutine = NULL;
@@ -819,14 +832,14 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct nbd_reply *reply,
static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *request)
{
NBDClient *client = req->client;
- int csock = client->sock;
uint32_t command;
ssize_t rc;
+ g_assert(qemu_in_coroutine());
client->recv_coroutine = qemu_coroutine_self();
nbd_update_can_read(client);
- rc = nbd_receive_request(csock, request);
+ rc = nbd_receive_request(client->ioc, request);
if (rc < 0) {
if (rc != -EAGAIN) {
rc = -EIO;
@@ -861,7 +874,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
if (command == NBD_CMD_WRITE) {
TRACE("Reading %u byte(s)", request->len);
- if (qemu_co_recv(csock, req->data, request->len) != request->len) {
+ if (read_sync(client->ioc, req->data, request->len) != request->len) {
LOG("reading from socket failed");
rc = -EIO;
goto out;
@@ -1056,7 +1069,7 @@ static void nbd_restart_write(void *opaque)
static void nbd_set_handlers(NBDClient *client)
{
if (client->exp && client->exp->ctx) {
- aio_set_fd_handler(client->exp->ctx, client->sock,
+ aio_set_fd_handler(client->exp->ctx, client->sioc->fd,
true,
client->can_read ? nbd_read : NULL,
client->send_coroutine ? nbd_restart_write : NULL,
@@ -1067,7 +1080,7 @@ static void nbd_set_handlers(NBDClient *client)
static void nbd_unset_handlers(NBDClient *client)
{
if (client->exp && client->exp->ctx) {
- aio_set_fd_handler(client->exp->ctx, client->sock,
+ aio_set_fd_handler(client->exp->ctx, client->sioc->fd,
true, NULL, NULL, NULL);
}
}
@@ -1109,7 +1122,9 @@ out:
g_free(data);
}
-void nbd_client_new(NBDExport *exp, int csock, void (*close_fn)(NBDClient *))
+void nbd_client_new(NBDExport *exp,
+ QIOChannelSocket *sioc,
+ void (*close_fn)(NBDClient *))
{
NBDClient *client;
NBDClientNewData *data = g_new(NBDClientNewData, 1);
@@ -1117,7 +1132,10 @@ void nbd_client_new(NBDExport *exp, int csock, void (*close_fn)(NBDClient *))
client = g_malloc0(sizeof(NBDClient));
client->refcount = 1;
client->exp = exp;
- client->sock = csock;
+ client->sioc = sioc;
+ object_ref(OBJECT(client->sioc));
+ client->ioc = QIO_CHANNEL(sioc);
+ object_ref(OBJECT(client->ioc));
client->can_read = true;
client->close = close_fn;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index bc309e0..5e54290 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -249,7 +249,7 @@ static void *nbd_client_thread(void *arg)
goto out;
}
- ret = nbd_receive_negotiate(sioc->fd, NULL, &nbdflags,
+ ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
&size, &local_error);
if (ret < 0) {
if (local_error) {
@@ -265,7 +265,7 @@ static void *nbd_client_thread(void *arg)
goto out_socket;
}
- ret = nbd_init(fd, sioc->fd, nbdflags, size);
+ ret = nbd_init(fd, sioc, nbdflags, size);
if (ret < 0) {
goto out_fd;
}
@@ -325,7 +325,6 @@ static void nbd_client_closed(NBDClient *client)
static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
{
QIOChannelSocket *cioc;
- int fd;
cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
NULL);
@@ -340,10 +339,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
nb_fds++;
nbd_update_server_watch();
- fd = dup(cioc->fd);
- if (fd >= 0) {
- nbd_client_new(exp, fd, nbd_client_closed);
- }
+ nbd_client_new(exp, cioc, nbd_client_closed);
object_unref(OBJECT(cioc));
return TRUE;
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 19/28] nbd: invert client logic for negotiating protocol version
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (17 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 18/28] nbd: convert to using I/O channels for actual socket I/O Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 20/28] nbd: make server compliant with fixed newstyle spec Paolo Bonzini
` (9 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
The nbd_receive_negotiate() method takes different code
paths based on whether 'name == NULL', and then checks
the expected protocol version in each branch.
This patch inverts the logic, so that it takes different
code paths based on what protocol version it receives and
then checks if name is NULL or not as needed.
This facilitates later code which allows the client to
be capable of using the new style protocol regardless
of whether an export name is listed or not.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-8-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/client.c | 60 +++++++++++++++++++++++++++++-------------------------------
1 file changed, 29 insertions(+), 31 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index f6dd0a3..5064788 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -116,20 +116,11 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
magic = be64_to_cpu(magic);
TRACE("Magic is 0x%" PRIx64, magic);
- if (name) {
+ if (magic == NBD_OPTS_MAGIC) {
uint32_t reserved = 0;
uint32_t opt;
uint32_t namesize;
- TRACE("Checking magic (opts_magic)");
- if (magic != NBD_OPTS_MAGIC) {
- if (magic == NBD_CLIENT_MAGIC) {
- error_setg(errp, "Server does not support export names");
- } else {
- error_setg(errp, "Bad magic received");
- }
- goto fail;
- }
if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
error_setg(errp, "Failed to read server flags");
goto fail;
@@ -142,6 +133,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
goto fail;
}
/* write the export name */
+ if (!name) {
+ error_setg(errp, "Server requires an export name");
+ goto fail;
+ }
magic = cpu_to_be64(magic);
if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
error_setg(errp, "Failed to send export name magic");
@@ -162,39 +157,42 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
error_setg(errp, "Failed to send export name");
goto fail;
}
- } else {
- TRACE("Checking magic (cli_magic)");
- if (magic != NBD_CLIENT_MAGIC) {
- if (magic == NBD_OPTS_MAGIC) {
- error_setg(errp, "Server requires an export name");
- } else {
- error_setg(errp, "Bad magic received");
- }
+ if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+ error_setg(errp, "Failed to read export length");
goto fail;
}
- }
+ *size = be64_to_cpu(s);
+ TRACE("Size is %" PRIu64, *size);
- if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
- error_setg(errp, "Failed to read export length");
- goto fail;
- }
- *size = be64_to_cpu(s);
- TRACE("Size is %" PRIu64, *size);
+ if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ error_setg(errp, "Failed to read export flags");
+ goto fail;
+ }
+ *flags |= be16_to_cpu(tmp);
+ } else if (magic == NBD_CLIENT_MAGIC) {
+ if (name) {
+ error_setg(errp, "Server does not support export names");
+ goto fail;
+ }
+
+ if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+ error_setg(errp, "Failed to read export length");
+ goto fail;
+ }
+ *size = be64_to_cpu(s);
+ TRACE("Size is %" PRIu64, *size);
- if (!name) {
if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
*flags = be32_to_cpup(flags);
} else {
- if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
- error_setg(errp, "Failed to read export flags");
- goto fail;
- }
- *flags |= be16_to_cpu(tmp);
+ error_setg(errp, "Bad magic received");
+ goto fail;
}
+
if (read_sync(ioc, &buf, 124) != 124) {
error_setg(errp, "Failed to read reserved block");
goto fail;
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 20/28] nbd: make server compliant with fixed newstyle spec
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (18 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 19/28] nbd: invert client logic for negotiating protocol version Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 21/28] nbd: make client request fixed new style if advertised Paolo Bonzini
` (8 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
If the client does not request the fixed new style protocol,
then we should only accept NBD_OPT_EXPORT_NAME. All other
options are only valid when fixed new style has been activated.
The qemu-nbd client doesn't currently request fixed new style
protocol, but this change won't break qemu-nbd, because it
fortunately only ever uses NBD_OPT_EXPORT_NAME, so was never
triggering the non-compliant server behaviour.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-9-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/server.c | 69 ++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 46 insertions(+), 23 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 15aa03d..074a1e6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -310,6 +310,7 @@ fail:
static int nbd_negotiate_options(NBDClient *client)
{
uint32_t flags;
+ bool fixedNewstyle = false;
/* Client sends:
[ 0 .. 3] client flags
@@ -332,14 +333,19 @@ static int nbd_negotiate_options(NBDClient *client)
}
TRACE("Checking client flags");
be32_to_cpus(&flags);
- if (flags != 0 && flags != NBD_FLAG_C_FIXED_NEWSTYLE) {
- LOG("Bad client flags received");
+ if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
+ TRACE("Support supports fixed newstyle handshake");
+ fixedNewstyle = true;
+ flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
+ }
+ if (flags != 0) {
+ TRACE("Unknown client flags 0x%x received", flags);
return -EIO;
}
while (1) {
int ret;
- uint32_t tmp, length;
+ uint32_t clientflags, length;
uint64_t magic;
if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) !=
@@ -353,10 +359,12 @@ static int nbd_negotiate_options(NBDClient *client)
return -EINVAL;
}
- if (nbd_negotiate_read(client->ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ if (nbd_negotiate_read(client->ioc, &clientflags,
+ sizeof(clientflags)) != sizeof(clientflags)) {
LOG("read failed");
return -EINVAL;
}
+ clientflags = be32_to_cpu(clientflags);
if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) !=
sizeof(length)) {
@@ -365,26 +373,41 @@ static int nbd_negotiate_options(NBDClient *client)
}
length = be32_to_cpu(length);
- TRACE("Checking option");
- switch (be32_to_cpu(tmp)) {
- case NBD_OPT_LIST:
- ret = nbd_negotiate_handle_list(client, length);
- if (ret < 0) {
- return ret;
+ TRACE("Checking option 0x%x", clientflags);
+ if (fixedNewstyle) {
+ switch (clientflags) {
+ case NBD_OPT_LIST:
+ ret = nbd_negotiate_handle_list(client, length);
+ if (ret < 0) {
+ return ret;
+ }
+ break;
+
+ case NBD_OPT_ABORT:
+ return -EINVAL;
+
+ case NBD_OPT_EXPORT_NAME:
+ return nbd_negotiate_handle_export_name(client, length);
+
+ default:
+ TRACE("Unsupported option 0x%x", clientflags);
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP,
+ clientflags);
+ return -EINVAL;
+ }
+ } else {
+ /*
+ * If broken new-style we should drop the connection
+ * for anything except NBD_OPT_EXPORT_NAME
+ */
+ switch (clientflags) {
+ case NBD_OPT_EXPORT_NAME:
+ return nbd_negotiate_handle_export_name(client, length);
+
+ default:
+ TRACE("Unsupported option 0x%x", clientflags);
+ return -EINVAL;
}
- break;
-
- case NBD_OPT_ABORT:
- return -EINVAL;
-
- case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client, length);
-
- default:
- tmp = be32_to_cpu(tmp);
- LOG("Unsupported option 0x%x", tmp);
- nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP, tmp);
- return -EINVAL;
}
}
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 21/28] nbd: make client request fixed new style if advertised
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (19 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 20/28] nbd: make server compliant with fixed newstyle spec Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 22/28] nbd: allow setting of an export name for qemu-nbd server Paolo Bonzini
` (7 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
If the server advertises support for the fixed new style
negotiation, the client should in turn enable new style.
This will allow the client to negotiate further NBD
options besides the export name.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-10-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/client.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 5064788..88f2ada 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -76,7 +76,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
{
char buf[256];
uint64_t magic, s;
- uint16_t tmp;
int rc;
TRACE("Receiving negotiation.");
@@ -117,19 +116,26 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
TRACE("Magic is 0x%" PRIx64, magic);
if (magic == NBD_OPTS_MAGIC) {
- uint32_t reserved = 0;
+ uint32_t clientflags = 0;
uint32_t opt;
uint32_t namesize;
+ uint16_t globalflags;
+ uint16_t exportflags;
- if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
+ sizeof(globalflags)) {
error_setg(errp, "Failed to read server flags");
goto fail;
}
- *flags = be16_to_cpu(tmp) << 16;
- /* reserved for future use */
- if (write_sync(ioc, &reserved, sizeof(reserved)) !=
- sizeof(reserved)) {
- error_setg(errp, "Failed to read reserved field");
+ *flags = be16_to_cpu(globalflags) << 16;
+ if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+ TRACE("Server supports fixed new style");
+ clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
+ }
+ /* client requested flags */
+ if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
+ sizeof(clientflags)) {
+ error_setg(errp, "Failed to send clientflags field");
goto fail;
}
/* write the export name */
@@ -165,11 +171,12 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
*size = be64_to_cpu(s);
TRACE("Size is %" PRIu64, *size);
- if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+ if (read_sync(ioc, &exportflags, sizeof(exportflags)) !=
+ sizeof(exportflags)) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
- *flags |= be16_to_cpu(tmp);
+ *flags |= be16_to_cpu(exportflags);
} else if (magic == NBD_CLIENT_MAGIC) {
if (name) {
error_setg(errp, "Server does not support export names");
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 22/28] nbd: allow setting of an export name for qemu-nbd server
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (20 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 21/28] nbd: make client request fixed new style if advertised Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol Paolo Bonzini
` (6 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
The qemu-nbd server currently always uses the old style protocol
since it never sets any export name. This is a problem because
future TLS support will require use of the new style protocol
negotiation.
This adds "--exportname NAME" / "-x NAME" arguments to qemu-nbd
which allow the user to set an explicit export name. When an
export name is set the server will always use the new style
NBD protocol.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-11-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-nbd.c | 14 ++++++++++++--
qemu-nbd.texi | 3 +++
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 5e54290..9710a26 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -44,6 +44,7 @@
#define QEMU_NBD_OPT_OBJECT 5
static NBDExport *exp;
+static bool newproto;
static int verbose;
static char *srcpath;
static SocketAddress *saddr;
@@ -339,7 +340,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
nb_fds++;
nbd_update_server_watch();
- nbd_client_new(exp, cioc, nbd_client_closed);
+ nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed);
object_unref(OBJECT(cioc));
return TRUE;
@@ -413,7 +414,7 @@ int main(int argc, char **argv)
off_t fd_size;
QemuOpts *sn_opts = NULL;
const char *sn_id_or_name = NULL;
- const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
+ const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
struct option lopt[] = {
{ "help", 0, NULL, 'h' },
{ "version", 0, NULL, 'V' },
@@ -437,6 +438,7 @@ int main(int argc, char **argv)
{ "persistent", 0, NULL, 't' },
{ "verbose", 0, NULL, 'v' },
{ "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
+ { "export-name", 1, NULL, 'x' },
{ NULL, 0, NULL, 0 }
};
int ch;
@@ -453,6 +455,7 @@ int main(int argc, char **argv)
Error *local_err = NULL;
BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
QDict *options = NULL;
+ const char *export_name = NULL;
/* The client thread uses SIGTERM to interrupt the server. A signal
* handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -604,6 +607,9 @@ int main(int argc, char **argv)
case 't':
persistent = 1;
break;
+ case 'x':
+ export_name = optarg;
+ break;
case 'v':
verbose = 1;
break;
@@ -783,6 +789,10 @@ int main(int argc, char **argv)
error_report_err(local_err);
exit(EXIT_FAILURE);
}
+ if (export_name) {
+ nbd_export_set_name(exp, export_name);
+ newproto = true;
+ }
server_ioc = qio_channel_socket_new();
if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index a56ebc3..874481d 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -73,6 +73,9 @@ Disconnect the device @var{dev}
Allow up to @var{num} clients to share the device (default @samp{1})
@item -t, --persistent
Don't exit on the last connection
+@item -x NAME, --export-name=NAME
+Set the NBD volume export name. This switches the server to use
+the new style NBD protocol negotiation
@item -v, --verbose
Display extra debugging information
@item -h, --help
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (21 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 22/28] nbd: allow setting of an export name for qemu-nbd server Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-05-17 9:53 ` Richard W.M. Jones
2016-02-16 16:34 ` [Qemu-devel] [PULL 24/28] nbd: use "" as a default export name if none provided Paolo Bonzini
` (5 subsequent siblings)
28 siblings, 1 reply; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
With the new style protocol, the NBD client will currenetly
send NBD_OPT_EXPORT_NAME as the first (and indeed only)
option it wants. The problem is that the NBD protocol spec
does not allow for returning an error message with the
NBD_OPT_EXPORT_NAME option. So if the server mandates use
of TLS, the client will simply see an immediate connection
close after issuing NBD_OPT_EXPORT_NAME which is not user
friendly.
To improve this situation, if we have the fixed new style
protocol, we can sent NBD_OPT_LIST as the first option
to query the list of server exports. We can check for our
named export in this list and raise an error if it is not
found, instead of going ahead and sending NBD_OPT_EXPORT_NAME
with a name that we know will be rejected.
This improves the error reporting both in the case that the
server required TLS, and in the case that the client requested
export name does not exist on the server.
If the server does not support NBD_OPT_LIST, we just ignore
that and carry on with NBD_OPT_EXPORT_NAME as before.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-12-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/client.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
nbd/server.c | 2 +
tests/qemu-iotests/140.out | 2 +-
tests/qemu-iotests/143.out | 2 +-
4 files changed, 196 insertions(+), 5 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 88f2ada..be5f08d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -71,6 +71,177 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
*/
+
+static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
+{
+ if (!(type & (1 << 31))) {
+ return 0;
+ }
+
+ switch (type) {
+ case NBD_REP_ERR_UNSUP:
+ error_setg(errp, "Unsupported option type %x", opt);
+ break;
+
+ case NBD_REP_ERR_INVALID:
+ error_setg(errp, "Invalid data length for option %x", opt);
+ break;
+
+ default:
+ error_setg(errp, "Unknown error code when asking for option %x", opt);
+ break;
+ }
+
+ return -1;
+}
+
+static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
+{
+ uint64_t magic;
+ uint32_t opt;
+ uint32_t type;
+ uint32_t len;
+ uint32_t namelen;
+
+ *name = NULL;
+ if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+ error_setg(errp, "failed to read list option magic");
+ return -1;
+ }
+ magic = be64_to_cpu(magic);
+ if (magic != NBD_REP_MAGIC) {
+ error_setg(errp, "Unexpected option list magic");
+ return -1;
+ }
+ if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+ error_setg(errp, "failed to read list option");
+ return -1;
+ }
+ opt = be32_to_cpu(opt);
+ if (opt != NBD_OPT_LIST) {
+ error_setg(errp, "Unexpected option type %x expected %x",
+ opt, NBD_OPT_LIST);
+ return -1;
+ }
+
+ if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) {
+ error_setg(errp, "failed to read list option type");
+ return -1;
+ }
+ type = be32_to_cpu(type);
+ if (type == NBD_REP_ERR_UNSUP) {
+ return 0;
+ }
+ if (nbd_handle_reply_err(opt, type, errp) < 0) {
+ return -1;
+ }
+
+ if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) {
+ error_setg(errp, "failed to read option length");
+ return -1;
+ }
+ len = be32_to_cpu(len);
+
+ if (type == NBD_REP_ACK) {
+ if (len != 0) {
+ error_setg(errp, "length too long for option end");
+ return -1;
+ }
+ } else if (type == NBD_REP_SERVER) {
+ if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+ error_setg(errp, "failed to read option name length");
+ return -1;
+ }
+ namelen = be32_to_cpu(namelen);
+ if (len != (namelen + sizeof(namelen))) {
+ error_setg(errp, "incorrect option mame length");
+ return -1;
+ }
+ if (namelen > 255) {
+ error_setg(errp, "export name length too long %d", namelen);
+ return -1;
+ }
+
+ *name = g_new0(char, namelen + 1);
+ if (read_sync(ioc, *name, namelen) != namelen) {
+ error_setg(errp, "failed to read export name");
+ g_free(*name);
+ *name = NULL;
+ return -1;
+ }
+ (*name)[namelen] = '\0';
+ } else {
+ error_setg(errp, "Unexpected reply type %x expected %x",
+ type, NBD_REP_SERVER);
+ return -1;
+ }
+ return 1;
+}
+
+
+static int nbd_receive_query_exports(QIOChannel *ioc,
+ const char *wantname,
+ Error **errp)
+{
+ uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
+ uint32_t opt = cpu_to_be32(NBD_OPT_LIST);
+ uint32_t length = 0;
+ bool foundExport = false;
+
+ TRACE("Querying export list");
+ if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+ error_setg(errp, "Failed to send list option magic");
+ return -1;
+ }
+
+ if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+ error_setg(errp, "Failed to send list option number");
+ return -1;
+ }
+
+ if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
+ error_setg(errp, "Failed to send list option length");
+ return -1;
+ }
+
+ TRACE("Reading available export names");
+ while (1) {
+ char *name = NULL;
+ int ret = nbd_receive_list(ioc, &name, errp);
+
+ if (ret < 0) {
+ g_free(name);
+ name = NULL;
+ return -1;
+ }
+ if (ret == 0) {
+ /* Server doesn't support export listing, so
+ * we will just assume an export with our
+ * wanted name exists */
+ foundExport = true;
+ break;
+ }
+ if (name == NULL) {
+ TRACE("End of export name list");
+ break;
+ }
+ if (g_str_equal(name, wantname)) {
+ foundExport = true;
+ TRACE("Found desired export name '%s'", name);
+ } else {
+ TRACE("Ignored export name '%s'", name);
+ }
+ g_free(name);
+ }
+
+ if (!foundExport) {
+ error_setg(errp, "No export with name '%s' available", wantname);
+ return -1;
+ }
+
+ return 0;
+}
+
int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
off_t *size, Error **errp)
{
@@ -121,28 +292,44 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
uint32_t namesize;
uint16_t globalflags;
uint16_t exportflags;
+ bool fixedNewStyle = false;
if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
sizeof(globalflags)) {
error_setg(errp, "Failed to read server flags");
goto fail;
}
- *flags = be16_to_cpu(globalflags) << 16;
+ globalflags = be16_to_cpu(globalflags);
+ *flags = globalflags << 16;
+ TRACE("Global flags are %x", globalflags);
if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+ fixedNewStyle = true;
TRACE("Server supports fixed new style");
clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
}
/* client requested flags */
+ clientflags = cpu_to_be32(clientflags);
if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
sizeof(clientflags)) {
error_setg(errp, "Failed to send clientflags field");
goto fail;
}
- /* write the export name */
if (!name) {
error_setg(errp, "Server requires an export name");
goto fail;
}
+ if (fixedNewStyle) {
+ /* Check our desired export is present in the
+ * server export list. Since NBD_OPT_EXPORT_NAME
+ * cannot return an error message, running this
+ * query gives us good error reporting if the
+ * server required TLS
+ */
+ if (nbd_receive_query_exports(ioc, name, errp) < 0) {
+ goto fail;
+ }
+ }
+ /* write the export name */
magic = cpu_to_be64(magic);
if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
error_setg(errp, "Failed to send export name magic");
@@ -176,7 +363,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
error_setg(errp, "Failed to read export flags");
goto fail;
}
- *flags |= be16_to_cpu(exportflags);
+ exportflags = be16_to_cpu(exportflags);
+ *flags |= exportflags;
+ TRACE("Export flags are %x", exportflags);
} else if (magic == NBD_CLIENT_MAGIC) {
if (name) {
error_setg(errp, "Server does not support export names");
diff --git a/nbd/server.c b/nbd/server.c
index 074a1e6..3d2fb10 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -294,6 +294,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
}
name[length] = '\0';
+ TRACE("Client requested export '%s'", name);
+
client->exp = nbd_export_find(name);
if (!client->exp) {
LOG("export not found");
diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
index fdedeb3..72f1b4c 100644
--- a/tests/qemu-iotests/140.out
+++ b/tests/qemu-iotests/140.out
@@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
{"return": {}}
-can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
+can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
no file open, try 'help open'
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
index dad2024..d24ad20 100644
--- a/tests/qemu-iotests/143.out
+++ b/tests/qemu-iotests/143.out
@@ -1,7 +1,7 @@
QA output created by 143
{"return": {}}
{"return": {}}
-can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: Failed to read export length
+can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: No export with name 'no_such_export' available
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
*** done
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-02-16 16:34 ` [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol Paolo Bonzini
@ 2016-05-17 9:53 ` Richard W.M. Jones
2016-05-17 15:09 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-17 9:53 UTC (permalink / raw)
To: Paolo Bonzini, berrange; +Cc: qemu-devel
On Tue, Feb 16, 2016 at 05:34:41PM +0100, Paolo Bonzini wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> With the new style protocol, the NBD client will currenetly
> send NBD_OPT_EXPORT_NAME as the first (and indeed only)
> option it wants. The problem is that the NBD protocol spec
> does not allow for returning an error message with the
> NBD_OPT_EXPORT_NAME option. So if the server mandates use
> of TLS, the client will simply see an immediate connection
> close after issuing NBD_OPT_EXPORT_NAME which is not user
> friendly.
>
> To improve this situation, if we have the fixed new style
> protocol, we can sent NBD_OPT_LIST as the first option
> to query the list of server exports. We can check for our
> named export in this list and raise an error if it is not
> found, instead of going ahead and sending NBD_OPT_EXPORT_NAME
> with a name that we know will be rejected.
>
> This improves the error reporting both in the case that the
> server required TLS, and in the case that the client requested
> export name does not exist on the server.
>
> If the server does not support NBD_OPT_LIST, we just ignore
> that and carry on with NBD_OPT_EXPORT_NAME as before.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Message-Id: <1455129674-17255-12-git-send-email-berrange@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
I just bisected qemu 2.6 to find out where it breaks interop with
nbdkit, and it is this commit.
nbdkit implements the newstyle protocol, but doesn't implement export
names (yet, maybe in future). It processes the NBD_OPT_EXPORT_NAME
option, but ignores the export name and carries on regardless.
nbdkit's implemention of NBD_OPT_LIST returns an error, because there
is no such thing as a list of export names supported (in effect nbdkit
allows any export name).
Therefore I don't believe the assumption made here -- that you can
list export names and choose them on the client side -- is a valid
one.
Naturally the protocol document
(https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
this case.
To test qemu against nbdkit you can do this in the nbdkit sources:
make
make check TESTS=test-newstyle \
LIBGUESTFS_HV=/path/to/qemu/x86_64-softmmu/qemu-system-x86_64 \
LIBGUESTFS_DEBUG=1 LIBGUESTFS_TRACE=1
Rich.
> nbd/client.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
> nbd/server.c | 2 +
> tests/qemu-iotests/140.out | 2 +-
> tests/qemu-iotests/143.out | 2 +-
> 4 files changed, 196 insertions(+), 5 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 88f2ada..be5f08d 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -71,6 +71,177 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
>
> */
>
> +
> +static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
> +{
> + if (!(type & (1 << 31))) {
> + return 0;
> + }
> +
> + switch (type) {
> + case NBD_REP_ERR_UNSUP:
> + error_setg(errp, "Unsupported option type %x", opt);
> + break;
> +
> + case NBD_REP_ERR_INVALID:
> + error_setg(errp, "Invalid data length for option %x", opt);
> + break;
> +
> + default:
> + error_setg(errp, "Unknown error code when asking for option %x", opt);
> + break;
> + }
> +
> + return -1;
> +}
> +
> +static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
> +{
> + uint64_t magic;
> + uint32_t opt;
> + uint32_t type;
> + uint32_t len;
> + uint32_t namelen;
> +
> + *name = NULL;
> + if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
> + error_setg(errp, "failed to read list option magic");
> + return -1;
> + }
> + magic = be64_to_cpu(magic);
> + if (magic != NBD_REP_MAGIC) {
> + error_setg(errp, "Unexpected option list magic");
> + return -1;
> + }
> + if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
> + error_setg(errp, "failed to read list option");
> + return -1;
> + }
> + opt = be32_to_cpu(opt);
> + if (opt != NBD_OPT_LIST) {
> + error_setg(errp, "Unexpected option type %x expected %x",
> + opt, NBD_OPT_LIST);
> + return -1;
> + }
> +
> + if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) {
> + error_setg(errp, "failed to read list option type");
> + return -1;
> + }
> + type = be32_to_cpu(type);
> + if (type == NBD_REP_ERR_UNSUP) {
> + return 0;
> + }
> + if (nbd_handle_reply_err(opt, type, errp) < 0) {
> + return -1;
> + }
> +
> + if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) {
> + error_setg(errp, "failed to read option length");
> + return -1;
> + }
> + len = be32_to_cpu(len);
> +
> + if (type == NBD_REP_ACK) {
> + if (len != 0) {
> + error_setg(errp, "length too long for option end");
> + return -1;
> + }
> + } else if (type == NBD_REP_SERVER) {
> + if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> + error_setg(errp, "failed to read option name length");
> + return -1;
> + }
> + namelen = be32_to_cpu(namelen);
> + if (len != (namelen + sizeof(namelen))) {
> + error_setg(errp, "incorrect option mame length");
> + return -1;
> + }
> + if (namelen > 255) {
> + error_setg(errp, "export name length too long %d", namelen);
> + return -1;
> + }
> +
> + *name = g_new0(char, namelen + 1);
> + if (read_sync(ioc, *name, namelen) != namelen) {
> + error_setg(errp, "failed to read export name");
> + g_free(*name);
> + *name = NULL;
> + return -1;
> + }
> + (*name)[namelen] = '\0';
> + } else {
> + error_setg(errp, "Unexpected reply type %x expected %x",
> + type, NBD_REP_SERVER);
> + return -1;
> + }
> + return 1;
> +}
> +
> +
> +static int nbd_receive_query_exports(QIOChannel *ioc,
> + const char *wantname,
> + Error **errp)
> +{
> + uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
> + uint32_t opt = cpu_to_be32(NBD_OPT_LIST);
> + uint32_t length = 0;
> + bool foundExport = false;
> +
> + TRACE("Querying export list");
> + if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
> + error_setg(errp, "Failed to send list option magic");
> + return -1;
> + }
> +
> + if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
> + error_setg(errp, "Failed to send list option number");
> + return -1;
> + }
> +
> + if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
> + error_setg(errp, "Failed to send list option length");
> + return -1;
> + }
> +
> + TRACE("Reading available export names");
> + while (1) {
> + char *name = NULL;
> + int ret = nbd_receive_list(ioc, &name, errp);
> +
> + if (ret < 0) {
> + g_free(name);
> + name = NULL;
> + return -1;
> + }
> + if (ret == 0) {
> + /* Server doesn't support export listing, so
> + * we will just assume an export with our
> + * wanted name exists */
> + foundExport = true;
> + break;
> + }
> + if (name == NULL) {
> + TRACE("End of export name list");
> + break;
> + }
> + if (g_str_equal(name, wantname)) {
> + foundExport = true;
> + TRACE("Found desired export name '%s'", name);
> + } else {
> + TRACE("Ignored export name '%s'", name);
> + }
> + g_free(name);
> + }
> +
> + if (!foundExport) {
> + error_setg(errp, "No export with name '%s' available", wantname);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
> off_t *size, Error **errp)
> {
> @@ -121,28 +292,44 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
> uint32_t namesize;
> uint16_t globalflags;
> uint16_t exportflags;
> + bool fixedNewStyle = false;
>
> if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
> sizeof(globalflags)) {
> error_setg(errp, "Failed to read server flags");
> goto fail;
> }
> - *flags = be16_to_cpu(globalflags) << 16;
> + globalflags = be16_to_cpu(globalflags);
> + *flags = globalflags << 16;
> + TRACE("Global flags are %x", globalflags);
> if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
> + fixedNewStyle = true;
> TRACE("Server supports fixed new style");
> clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
> }
> /* client requested flags */
> + clientflags = cpu_to_be32(clientflags);
> if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
> sizeof(clientflags)) {
> error_setg(errp, "Failed to send clientflags field");
> goto fail;
> }
> - /* write the export name */
> if (!name) {
> error_setg(errp, "Server requires an export name");
> goto fail;
> }
> + if (fixedNewStyle) {
> + /* Check our desired export is present in the
> + * server export list. Since NBD_OPT_EXPORT_NAME
> + * cannot return an error message, running this
> + * query gives us good error reporting if the
> + * server required TLS
> + */
> + if (nbd_receive_query_exports(ioc, name, errp) < 0) {
> + goto fail;
> + }
> + }
> + /* write the export name */
> magic = cpu_to_be64(magic);
> if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
> error_setg(errp, "Failed to send export name magic");
> @@ -176,7 +363,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
> error_setg(errp, "Failed to read export flags");
> goto fail;
> }
> - *flags |= be16_to_cpu(exportflags);
> + exportflags = be16_to_cpu(exportflags);
> + *flags |= exportflags;
> + TRACE("Export flags are %x", exportflags);
> } else if (magic == NBD_CLIENT_MAGIC) {
> if (name) {
> error_setg(errp, "Server does not support export names");
> diff --git a/nbd/server.c b/nbd/server.c
> index 074a1e6..3d2fb10 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -294,6 +294,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
> }
> name[length] = '\0';
>
> + TRACE("Client requested export '%s'", name);
> +
> client->exp = nbd_export_find(name);
> if (!client->exp) {
> LOG("export not found");
> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> index fdedeb3..72f1b4c 100644
> --- a/tests/qemu-iotests/140.out
> +++ b/tests/qemu-iotests/140.out
> @@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
> 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
> {"return": {}}
> -can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
> +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
> no file open, try 'help open'
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
> diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
> index dad2024..d24ad20 100644
> --- a/tests/qemu-iotests/143.out
> +++ b/tests/qemu-iotests/143.out
> @@ -1,7 +1,7 @@
> QA output created by 143
> {"return": {}}
> {"return": {}}
> -can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: Failed to read export length
> +can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: No export with name 'no_such_export' available
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
> *** done
> --
> 2.5.0
>
>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 9:53 ` Richard W.M. Jones
@ 2016-05-17 15:09 ` Eric Blake
2016-05-17 15:22 ` [Qemu-devel] [Nbd] " Alex Bligh
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2016-05-17 15:09 UTC (permalink / raw)
To: Richard W.M. Jones, Paolo Bonzini, berrange; +Cc: qemu-devel, nbd-general
[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]
[adding nbd-list]
On 05/17/2016 03:53 AM, Richard W.M. Jones wrote:
> On Tue, Feb 16, 2016 at 05:34:41PM +0100, Paolo Bonzini wrote:
>> From: "Daniel P. Berrange" <berrange@redhat.com>
>>
>> With the new style protocol, the NBD client will currenetly
>> send NBD_OPT_EXPORT_NAME as the first (and indeed only)
>> option it wants. The problem is that the NBD protocol spec
>> does not allow for returning an error message with the
>> NBD_OPT_EXPORT_NAME option. So if the server mandates use
>> of TLS, the client will simply see an immediate connection
>> close after issuing NBD_OPT_EXPORT_NAME which is not user
>> friendly.
>>
> I just bisected qemu 2.6 to find out where it breaks interop with
> nbdkit, and it is this commit.
>
> nbdkit implements the newstyle protocol, but doesn't implement export
> names (yet, maybe in future). It processes the NBD_OPT_EXPORT_NAME
> option, but ignores the export name and carries on regardless.
>
> nbdkit's implemention of NBD_OPT_LIST returns an error, because there
> is no such thing as a list of export names supported (in effect nbdkit
> allows any export name).
Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
default name) as its only list entry?
And at some point, nbdkit should probably implement NBD_OPT_GO (which is
a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
qemu to implement that).
In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
try NBD_OPT_LIST.
>
> Therefore I don't believe the assumption made here -- that you can
> list export names and choose them on the client side -- is a valid
> one.
Qemu is not choosing an export name, so much as proving whether any
OTHER errors can be detected (such as export name not present, or TLS
required). It _should_ be gracefully ignoring NBD_OPT_LIST errors if
such errors don't definitively prove that the export will not succeed,
but I guess this is not happening. I'll see if I can tweak the qemu
logic to be more forgiving of nbdkit's current behavior.
>
> Naturally the protocol document
> (https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
> this case.
You're right that we may also want to tweak the NBD protocol to make
this interoperability point obvious.
>
> To test qemu against nbdkit you can do this in the nbdkit sources:
>
> make
> make check TESTS=test-newstyle \
> LIBGUESTFS_HV=/path/to/qemu/x86_64-softmmu/qemu-system-x86_64 \
> LIBGUESTFS_DEBUG=1 LIBGUESTFS_TRACE=1
>
> Rich.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:09 ` Eric Blake
@ 2016-05-17 15:22 ` Alex Bligh
2016-05-17 15:52 ` Eric Blake
2016-05-17 15:54 ` Richard W.M. Jones
0 siblings, 2 replies; 51+ messages in thread
From: Alex Bligh @ 2016-05-17 15:22 UTC (permalink / raw)
To: Eric Blake, Richard W.M. Jones
Cc: Alex Bligh, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3478 bytes --]
Eric, Richard,
On 17 May 2016, at 16:09, Eric Blake <eblake@redhat.com> wrote:
> [adding nbd-list]
>
> On 05/17/2016 03:53 AM, Richard W.M. Jones wrote:
>> On Tue, Feb 16, 2016 at 05:34:41PM +0100, Paolo Bonzini wrote:
>>> From: "Daniel P. Berrange" <berrange@redhat.com>
>>>
>>> With the new style protocol, the NBD client will currenetly
>>> send NBD_OPT_EXPORT_NAME as the first (and indeed only)
>>> option it wants. The problem is that the NBD protocol spec
>>> does not allow for returning an error message with the
>>> NBD_OPT_EXPORT_NAME option. So if the server mandates use
>>> of TLS, the client will simply see an immediate connection
>>> close after issuing NBD_OPT_EXPORT_NAME which is not user
>>> friendly.
>>>
>
>> I just bisected qemu 2.6 to find out where it breaks interop with
>> nbdkit, and it is this commit.
>>
>> nbdkit implements the newstyle protocol, but doesn't implement export
>> names (yet, maybe in future). It processes the NBD_OPT_EXPORT_NAME
>> option, but ignores the export name and carries on regardless.
>>
>> nbdkit's implemention of NBD_OPT_LIST returns an error, because there
>> is no such thing as a list of export names supported (in effect nbdkit
>> allows any export name).
nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
compulsory, even if you support it by returning a nameless export
(or default). Moreover support of export names is compulsory
(even if you have a single fixed one called "" or "default").
This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
supports 'newstyle' negotiation, then we know qemu won't connect
to it as modern qemu only supports servers that support 'fixed newstyle'
I believe.
> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
> default name) as its only list entry?
Or "default".
> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
> qemu to implement that).
>
> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
> try NBD_OPT_LIST.
Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.
>> Therefore I don't believe the assumption made here -- that you can
>> list export names and choose them on the client side -- is a valid
>> one.
>
> Qemu is not choosing an export name, so much as proving whether any
> OTHER errors can be detected (such as export name not present, or TLS
> required). It _should_ be gracefully ignoring NBD_OPT_LIST errors if
> such errors don't definitively prove that the export will not succeed,
> but I guess this is not happening. I'll see if I can tweak the qemu
> logic to be more forgiving of nbdkit's current behavior.
Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
specification. If a server doesn't implement mandatory parts of
the specification, then bad things will happen.
My interpretation of NBD_OPT_LIST failing would be 'this server
doesn't have anything it can export'.
>> Naturally the protocol document
>> (https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
>> this case.
>
> You're right that we may also want to tweak the NBD protocol to make
> this interoperability point obvious.
I can't actually see the issue here. It explains what needs to be
implemented by the server, and that includes NBD_OPT_LIST. Very
happy to add some clarity, but I'm not sure where it's needed.
--
Alex Bligh
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:22 ` [Qemu-devel] [Nbd] " Alex Bligh
@ 2016-05-17 15:52 ` Eric Blake
2016-05-17 15:58 ` Richard W.M. Jones
` (2 more replies)
2016-05-17 15:54 ` Richard W.M. Jones
1 sibling, 3 replies; 51+ messages in thread
From: Eric Blake @ 2016-05-17 15:52 UTC (permalink / raw)
To: Alex Bligh, Richard W.M. Jones
Cc: Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]
On 05/17/2016 09:22 AM, Alex Bligh wrote:
>>> nbdkit's implemention of NBD_OPT_LIST returns an error, because there
>>> is no such thing as a list of export names supported (in effect nbdkit
>>> allows any export name).
>
> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
> compulsory, even if you support it by returning a nameless export
> (or default). Moreover support of export names is compulsory
> (even if you have a single fixed one called "" or "default").
Where does the protocol state that? I don't see any mandatory NBD_OPT in
the protocol, except for NBD_OPT_EXPORT_NAME (as I read it, a server may
reply with NBD_REP_ERR_UNSUP for every other option). I'm fine if we
WANT to make NBD_OPT_LIST mandatory (and either document under
NBD_OPT_LIST that NBD_REP_ERR_UNSUP cannot be used, or document under
NBD_REP_ERR_UNSUP that it cannot be used with NBD_OPT_LIST), but that
would make the current nbdkit non-conforming; so it might be nicer to
make a change to the protocol document that instead permits current
nbdkit behavior and puts the burden on clients to interoperate when
NBD_OPT_LIST is not supported.
>
> This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
> supports 'newstyle' negotiation, then we know qemu won't connect
> to it as modern qemu only supports servers that support 'fixed newstyle'
> I believe.
Not quite true. Qemu as a client is perfectly fine connecting with a
plain 'newstyle' server (and not just 'fixed newstyle'); but will limit
itself to JUST NBD_OPT_EXPORT_NAME in that case. So nbdkit must be
exposing 'fixed newstyle' for qemu to be attempting NBD_OPT_LIST.
>
>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>> default name) as its only list entry?
>
> Or "default".
As I read the protocol, I don't see "default" as a permissible name of
the default export, just "".
Also, we currently state that NBD_OPT_LIST has zero or more
NBD_REP_SERVER replies, which means that it is valid for the command to
succeed with NBD_REP_ACK _without_ advertising any exports at all
(rather annoying in that it tells you nothing about whether
NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways). Should we reword
that to require that if NBD_REP_ACK is sent, then at least one
NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
same time where "" is not mandatory if some other name is given)?
>
>> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
>> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
>> qemu to implement that).
>>
>> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
>> try NBD_OPT_LIST.
>
> Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.
Hopefully not much longer; we have multiple implementations converging
on interoperable use of it.
>
>>> Therefore I don't believe the assumption made here -- that you can
>>> list export names and choose them on the client side -- is a valid
>>> one.
>>
>> Qemu is not choosing an export name, so much as proving whether any
>> OTHER errors can be detected (such as export name not present, or TLS
>> required). It _should_ be gracefully ignoring NBD_OPT_LIST errors if
>> such errors don't definitively prove that the export will not succeed,
>> but I guess this is not happening. I'll see if I can tweak the qemu
>> logic to be more forgiving of nbdkit's current behavior.
>
> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
> specification. If a server doesn't implement mandatory parts of
> the specification, then bad things will happen.
Again, I don't see how the protocol justifies that claim. Maybe it
should, but I'd like to see your reasoning for stating that it is
mandatory that the server support it.
>
> My interpretation of NBD_OPT_LIST failing would be 'this server
> doesn't have anything it can export'.
Indeed, and that's why qemu as a client is currently dropping the
connection with nbdkit. But I would also make that interpretation for
NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
so maybe it is worth a note in the protocol how to detect servers that
are exporting exactly one volume and don't care what name you pass, then
tweaking either nbdkit, qemu, or both to comply to that added protocol
wording.
>
>>> Naturally the protocol document
>>> (https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
>>> this case.
>>
>> You're right that we may also want to tweak the NBD protocol to make
>> this interoperability point obvious.
>
> I can't actually see the issue here. It explains what needs to be
> implemented by the server, and that includes NBD_OPT_LIST. Very
> happy to add some clarity, but I'm not sure where it's needed.
I've hinted at it above - either an explicit statement that servers
cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
other exports, then the SHOULD include an NBD_REP_SERVER with name "";
or an explicit statement that if a server rejects NBD_OPT_LIST, then the
client SHOULD assume that any name will work for
NBD_OPT_EXPORT_NAME/NBD_OPT_GO.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:52 ` Eric Blake
@ 2016-05-17 15:58 ` Richard W.M. Jones
2016-05-17 16:05 ` Eric Blake
2016-05-17 15:59 ` Eric Blake
2016-05-17 16:22 ` Alex Bligh
2 siblings, 1 reply; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-17 15:58 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
> so it might be nicer to
> make a change to the protocol document that instead permits current
> nbdkit behavior and puts the burden on clients to interoperate when
> NBD_OPT_LIST is not supported.
The purpose of nbdkit is to be a server for qemu, to be a replacement
for qemu-nbd in cases where proprietary code cannot be combined with
qemu for copyright/licensing/legal reasons. So we aim to make sure we
can interoperate with qemu. No need to change any standards for
nbdkit! Clarifying standards documents is OK though.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:58 ` Richard W.M. Jones
@ 2016-05-17 16:05 ` Eric Blake
2016-05-17 16:41 ` Richard W.M. Jones
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2016-05-17 16:05 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Alex Bligh, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]
On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
>> so it might be nicer to
>> make a change to the protocol document that instead permits current
>> nbdkit behavior and puts the burden on clients to interoperate when
>> NBD_OPT_LIST is not supported.
>
> The purpose of nbdkit is to be a server for qemu, to be a replacement
> for qemu-nbd in cases where proprietary code cannot be combined with
> qemu for copyright/licensing/legal reasons. So we aim to make sure we
> can interoperate with qemu. No need to change any standards for
> nbdkit! Clarifying standards documents is OK though.
I also noticed that nbdkit forcefully rejects a client that sends more
than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
on the part of the client. Maybe the protocol should document a
(higher) limit on minimum number of options a client can expect to be
serviced before the server dropping the connection because the client is
wasting the server's time.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:05 ` Eric Blake
@ 2016-05-17 16:41 ` Richard W.M. Jones
2016-05-17 16:56 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-17 16:41 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
On Tue, May 17, 2016 at 10:05:50AM -0600, Eric Blake wrote:
> On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
> > On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
> >> so it might be nicer to
> >> make a change to the protocol document that instead permits current
> >> nbdkit behavior and puts the burden on clients to interoperate when
> >> NBD_OPT_LIST is not supported.
> >
> > The purpose of nbdkit is to be a server for qemu, to be a replacement
> > for qemu-nbd in cases where proprietary code cannot be combined with
> > qemu for copyright/licensing/legal reasons. So we aim to make sure we
> > can interoperate with qemu. No need to change any standards for
> > nbdkit! Clarifying standards documents is OK though.
>
> I also noticed that nbdkit forcefully rejects a client that sends more
> than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
> on the part of the client. Maybe the protocol should document a
> (higher) limit on minimum number of options a client can expect to be
> serviced before the server dropping the connection because the client is
> wasting the server's time.
This, as you say, is a brake on clients that try to waste time by
sending infinite numbers of options. Is there any danger that 32 is
too small?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:41 ` Richard W.M. Jones
@ 2016-05-17 16:56 ` Eric Blake
2016-05-17 17:36 ` Alex Bligh
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2016-05-17 16:56 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Alex Bligh, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]
On 05/17/2016 10:41 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 10:05:50AM -0600, Eric Blake wrote:
>> On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
>>> On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
>>>> so it might be nicer to
>>>> make a change to the protocol document that instead permits current
>>>> nbdkit behavior and puts the burden on clients to interoperate when
>>>> NBD_OPT_LIST is not supported.
>>>
>>> The purpose of nbdkit is to be a server for qemu, to be a replacement
>>> for qemu-nbd in cases where proprietary code cannot be combined with
>>> qemu for copyright/licensing/legal reasons. So we aim to make sure we
>>> can interoperate with qemu. No need to change any standards for
>>> nbdkit! Clarifying standards documents is OK though.
>>
>> I also noticed that nbdkit forcefully rejects a client that sends more
>> than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
>> on the part of the client. Maybe the protocol should document a
>> (higher) limit on minimum number of options a client can expect to be
>> serviced before the server dropping the connection because the client is
>> wasting the server's time.
>
> This, as you say, is a brake on clients that try to waste time by
> sending infinite numbers of options. Is there any danger that 32 is
> too small?
Yes. Consider a client that connects to a server that lists more than 32
exports in NBD_OPT_LIST, then the client calls (the still-experimental)
NBD_OPT_INFO on each of those exports, to learn further details about
each export, before finally using NBD_OPT_GO to pick the one the user
likes best.
That's why I wonder if we need to document a minimum cutoff at which
clients should assume will always be serviced, and which servers should
not treat as an attack, and whether it should be larger than 32.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:56 ` Eric Blake
@ 2016-05-17 17:36 ` Alex Bligh
2016-05-17 18:47 ` Richard W.M. Jones
0 siblings, 1 reply; 51+ messages in thread
From: Alex Bligh @ 2016-05-17 17:36 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, Richard W.M. Jones, Paolo Bonzini,
Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 614 bytes --]
On 17 May 2016, at 17:56, Eric Blake <eblake@redhat.com> wrote:
> That's why I wonder if we need to document a minimum cutoff at which
> clients should assume will always be serviced, and which servers should
> not treat as an attack, and whether it should be larger than 32.
I don't think we need a specific number, but I think a one sentence
statement that servers are in general permitted to disconnect clients
which are behaving in a denial of service attack type manner would be
useful.
Given nbdkit only ever has one export, it seems eminently reasonable
for it to use a lower number.
--
Alex Bligh
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 17:36 ` Alex Bligh
@ 2016-05-17 18:47 ` Richard W.M. Jones
0 siblings, 0 replies; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-17 18:47 UTC (permalink / raw)
To: Alex Bligh
Cc: Eric Blake, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
Just to close this thread out, I have implemented the exportname in
qemu. NBD_OPT_LIST returns the exportname. nbdkit now interoperates
with qemu 2.5 and 2.6.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:52 ` Eric Blake
2016-05-17 15:58 ` Richard W.M. Jones
@ 2016-05-17 15:59 ` Eric Blake
2016-05-17 16:39 ` Richard W.M. Jones
2016-05-17 16:22 ` Alex Bligh
2 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2016-05-17 15:59 UTC (permalink / raw)
To: Alex Bligh, Richard W.M. Jones
Cc: nbd-general, Paolo Bonzini, Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]
On 05/17/2016 09:52 AM, Eric Blake wrote:
>>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>>> default name) as its only list entry?
>>
>> Or "default".
>
> As I read the protocol, I don't see "default" as a permissible name of
> the default export, just "".
>
> Also, we currently state that NBD_OPT_LIST has zero or more
> NBD_REP_SERVER replies, which means that it is valid for the command to
> succeed with NBD_REP_ACK _without_ advertising any exports at all
> (rather annoying in that it tells you nothing about whether
> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways). Should we reword
> that to require that if NBD_REP_ACK is sent, then at least one
> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
> same time where "" is not mandatory if some other name is given)?
Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
entries and NBD_REP_ACK (for success), and NOT an error code; so it is
implying that there are NO known export names but that the command was
recognized.
>>
>> My interpretation of NBD_OPT_LIST failing would be 'this server
>> doesn't have anything it can export'.
>
> Indeed, and that's why qemu as a client is currently dropping the
> connection with nbdkit.
Except that nbdkit is NOT failing NBD_OPT_LIST, just merely listing 0
replies.
> But I would also make that interpretation for
> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
which is the nbdkit behavior
> so maybe it is worth a note in the protocol how to detect servers that
> are exporting exactly one volume and don't care what name you pass, then
> tweaking either nbdkit, qemu, or both to comply to that added protocol
> wording.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:59 ` Eric Blake
@ 2016-05-17 16:39 ` Richard W.M. Jones
2016-05-17 16:58 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-17 16:39 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, nbd-general, Paolo Bonzini, Daniel P. Berrange, qemu-devel
On Tue, May 17, 2016 at 09:59:02AM -0600, Eric Blake wrote:
> On 05/17/2016 09:52 AM, Eric Blake wrote:
> >>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
> >>> default name) as its only list entry?
> >>
> >> Or "default".
> >
> > As I read the protocol, I don't see "default" as a permissible name of
> > the default export, just "".
> >
> > Also, we currently state that NBD_OPT_LIST has zero or more
> > NBD_REP_SERVER replies, which means that it is valid for the command to
> > succeed with NBD_REP_ACK _without_ advertising any exports at all
> > (rather annoying in that it tells you nothing about whether
> > NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways). Should we reword
> > that to require that if NBD_REP_ACK is sent, then at least one
> > NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
> > same time where "" is not mandatory if some other name is given)?
>
> Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
> entries and NBD_REP_ACK (for success), and NOT an error code; so it is
> implying that there are NO known export names but that the command was
> recognized.
Bleah you are right. I misread the code I wrote :-(
We can add a empty string list entry there easily enough.
Rich.
> >>
> >> My interpretation of NBD_OPT_LIST failing would be 'this server
> >> doesn't have anything it can export'.
> >
> > Indeed, and that's why qemu as a client is currently dropping the
> > connection with nbdkit.
>
> Except that nbdkit is NOT failing NBD_OPT_LIST, just merely listing 0
> replies.
>
> > But I would also make that interpretation for
> > NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
>
> which is the nbdkit behavior
>
> > so maybe it is worth a note in the protocol how to detect servers that
> > are exporting exactly one volume and don't care what name you pass, then
> > tweaking either nbdkit, qemu, or both to comply to that added protocol
> > wording.
>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:39 ` Richard W.M. Jones
@ 2016-05-17 16:58 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2016-05-17 16:58 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Alex Bligh, nbd-general, Paolo Bonzini, Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]
On 05/17/2016 10:39 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 09:59:02AM -0600, Eric Blake wrote:
>> On 05/17/2016 09:52 AM, Eric Blake wrote:
>>>>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>>>>> default name) as its only list entry?
>>>>
>>>> Or "default".
>>>
>>> As I read the protocol, I don't see "default" as a permissible name of
>>> the default export, just "".
>>>
>>> Also, we currently state that NBD_OPT_LIST has zero or more
>>> NBD_REP_SERVER replies, which means that it is valid for the command to
>>> succeed with NBD_REP_ACK _without_ advertising any exports at all
>>> (rather annoying in that it tells you nothing about whether
>>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways). Should we reword
>>> that to require that if NBD_REP_ACK is sent, then at least one
>>> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
>>> same time where "" is not mandatory if some other name is given)?
>>
>> Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
>> entries and NBD_REP_ACK (for success), and NOT an error code; so it is
>> implying that there are NO known export names but that the command was
>> recognized.
>
> Bleah you are right. I misread the code I wrote :-(
>
> We can add a empty string list entry there easily enough.
Will adding the empty string break qemu 2.5 connections? qemu 2.6 is
picky in that if the name it is requesting is not in the advertised
list, it gives up; but if there is NO advertised list, it assumes the
name will just work. Since nbdkit has the latter behavior, I'm thinking
that nbdkit returning NBD_REP_ERR_UNSUP instead of NBD_REP_ACK is a
smarter course of action.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:52 ` Eric Blake
2016-05-17 15:58 ` Richard W.M. Jones
2016-05-17 15:59 ` Eric Blake
@ 2016-05-17 16:22 ` Alex Bligh
2016-05-17 16:50 ` Eric Blake
2 siblings, 1 reply; 51+ messages in thread
From: Alex Bligh @ 2016-05-17 16:22 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, Richard W.M. Jones, Paolo Bonzini,
Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6005 bytes --]
On 17 May 2016, at 16:52, Eric Blake <eblake@redhat.com> wrote:
> On 05/17/2016 09:22 AM, Alex Bligh wrote:
>
>>>> nbdkit's implemention of NBD_OPT_LIST returns an error, because there
>>>> is no such thing as a list of export names supported (in effect nbdkit
>>>> allows any export name).
>>
>> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
>> compulsory, even if you support it by returning a nameless export
>> (or default). Moreover support of export names is compulsory
>> (even if you have a single fixed one called "" or "default").
>
> Where does the protocol state that? I don't see any mandatory NBD_OPT in
> the protocol, except for NBD_OPT_EXPORT_NAME (as I read it, a server may
> reply with NBD_REP_ERR_UNSUP for every other option). I'm fine if we
> WANT to make NBD_OPT_LIST mandatory (and either document under
> NBD_OPT_LIST that NBD_REP_ERR_UNSUP cannot be used, or document under
> NBD_REP_ERR_UNSUP that it cannot be used with NBD_OPT_LIST), but that
> would make the current nbdkit non-conforming; so it might be nicer to
> make a change to the protocol document that instead permits current
> nbdkit behavior and puts the burden on clients to interoperate when
> NBD_OPT_LIST is not supported.
Under "Option Types" it says:
NBD_OPT_LIST (3) "Return zero or more NBD_REP_SERVER replies, one
for each export, followed by NBD_REP_ACK or an error (such as
NBD_REP_ERR_SHUTDOWN). The server MAY omit entries from this list if
TLS has not been negotiated, the server is operating in SELECTIVETLS
mode, and the entry concerned is a TLS-only export."
The language is imperative (if admittedly not RFC2119).
Also
The server will reply to any option apart from NBD_OPT_EXPORT_NAME
with reply packets in the following format:
again not in RFC2119 words, but says 'will' not 'MAY'.
I think the burden here is on the server to implement the protocol
as described.
I now accept that the wording should be cleared up :-)
>> This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
>> supports 'newstyle' negotiation, then we know qemu won't connect
>> to it as modern qemu only supports servers that support 'fixed newstyle'
>> I believe.
>
> Not quite true. Qemu as a client is perfectly fine connecting with a
> plain 'newstyle' server (and not just 'fixed newstyle'); but will limit
> itself to JUST NBD_OPT_EXPORT_NAME in that case. So nbdkit must be
> exposing 'fixed newstyle' for qemu to be attempting NBD_OPT_LIST.
OK. I think the problem is that if nbdkit supports 'fixed newstyle'
it should really support all of the options. Alternatively it can
support just 'newstyle' and then nothing will send anything bar
NBD_OPT_EXPORT_NAME.
>>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>>> default name) as its only list entry?
>>
>> Or "default".
>
> As I read the protocol, I don't see "default" as a permissible name of
> the default export, just "".
Sorry, I meant that if Richard implements NBD_OPT_LIST he could either
return an export name of "" or "default". Or for that matter "nbdkit"
or "hello world". Just "default" might display better than "". As he's
ignoring whatever comes back in NBD_OPT_EXPORT_NAME it doesn't really
matter what he returns in NBD_OPT_LIST.
> Also, we currently state that NBD_OPT_LIST has zero or more
> NBD_REP_SERVER replies, which means that it is valid for the command to
> succeed with NBD_REP_ACK _without_ advertising any exports at all
Yes. This is a valid way of saying "I have no exports that work".
> (rather annoying in that it tells you nothing about whether
> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).
If there are no exports then NBD_OPT_GO can't be expected to work.
> Should we reword
> that to require that if NBD_REP_ACK is sent, then at least one
> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
> same time where "" is not mandatory if some other name is given)?
I don't think so. A server might legitimately serve an empty list to
one IP and a non-empty list to another IP depending on configuration.
>>> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
>>> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
>>> qemu to implement that).
>>>
>>> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
>>> try NBD_OPT_LIST.
>>
>> Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.
>
> Hopefully not much longer; we have multiple implementations converging
> on interoperable use of it.
Indeed.
>> My interpretation of NBD_OPT_LIST failing would be 'this server
>> doesn't have anything it can export'.
>
> Indeed, and that's why qemu as a client is currently dropping the
> connection with nbdkit. But I would also make that interpretation for
> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
> so maybe it is worth a note in the protocol how to detect servers that
> are exporting exactly one volume and don't care what name you pass, then
> tweaking either nbdkit, qemu, or both to comply to that added protocol
> wording.
I think the correct thing for nbdkit to do would be to return a single
entry with an arbitrary name.
I can't see much harm in a client being 'nice' if it gets an UNSUP
error, but other errors (e.g. TLSREQD) have to be respected as errors.
> I've hinted at it above - either an explicit statement that servers
> cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
> other exports, then the SHOULD include an NBD_REP_SERVER with name "";
> or an explicit statement that if a server rejects NBD_OPT_LIST, then the
> client SHOULD assume that any name will work for
> NBD_OPT_EXPORT_NAME/NBD_OPT_GO.
I think it's simpler than that. Servers claiming to implement fixed
newstyle in my view need to implement all the options unless the options
are marked as optional. I admit (now) those could be clarified.
--
Alex Bligh
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:22 ` Alex Bligh
@ 2016-05-17 16:50 ` Eric Blake
2016-05-17 17:34 ` Alex Bligh
2016-05-21 21:53 ` Wouter Verhelst
0 siblings, 2 replies; 51+ messages in thread
From: Eric Blake @ 2016-05-17 16:50 UTC (permalink / raw)
To: Alex Bligh
Cc: Richard W.M. Jones, Paolo Bonzini, Daniel P. Berrange,
nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5412 bytes --]
On 05/17/2016 10:22 AM, Alex Bligh wrote:
[replying as I read, so I may seem to change my mind as I go...]
>>
>> As I read the protocol, I don't see "default" as a permissible name of
>> the default export, just "".
>
> Sorry, I meant that if Richard implements NBD_OPT_LIST he could either
> return an export name of "" or "default". Or for that matter "nbdkit"
> or "hello world". Just "default" might display better than "". As he's
> ignoring whatever comes back in NBD_OPT_EXPORT_NAME it doesn't really
> matter what he returns in NBD_OPT_LIST.
Except that qemu client DOES check that the returned list includes the
name that qemu plans on sending to NBD_OPT_EXPORT_NAME (that is, if the
client wants to connect to "foo", it expects "foo" in the list returned
by NBD_OPT_LIST).
Option 1: I think what I would prefer is a doc patch to the NBD protocol
that explicitly states that returning 0 names followed by NBD_REP_ACK
(current nbdkit behavior) implies that the server doesn't honor names at
all, and will let ANY name work for NBD_OPT_EXPORT_NAME/NBD_OPT_GO; then
patch qemu client to treat 0 names the same as NBD_REP_UNSUP (the server
doesn't honor names, so assume any name will work).
>
>> Also, we currently state that NBD_OPT_LIST has zero or more
>> NBD_REP_SERVER replies, which means that it is valid for the command to
>> succeed with NBD_REP_ACK _without_ advertising any exports at all
>
> Yes. This is a valid way of saying "I have no exports that work".
Except that nbdkit is using it as a way of saying "ALL export names
work" - and that's the special case I'm leaning towards documenting, and
fixing qemu client to support.
>
>> (rather annoying in that it tells you nothing about whether
>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).
>
> If there are no exports then NBD_OPT_GO can't be expected to work.
Option 2: An alternative solution would be to allow nbdkit to fail
NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
It is the fact that it is returning NBD_REP_ACK with 0 names that is
giving qemu grief.
>
>> Should we reword
>> that to require that if NBD_REP_ACK is sent, then at least one
>> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
>> same time where "" is not mandatory if some other name is given)?
>
> I don't think so. A server might legitimately serve an empty list to
> one IP and a non-empty list to another IP depending on configuration.
Okay, in such a case, returning 0 names to mean NBD_OPT_GO will never
succeed makes sense. So I'd argue that NBD_OPT_LIST should NOT succeed
in the case where names are ignored, and that nbdkit could be fixed by
merely ALWAYS returning NBD_REP_ERR_UNSUP to NBD_OPT_LIST.
>>> My interpretation of NBD_OPT_LIST failing would be 'this server
>>> doesn't have anything it can export'.
>>
>> Indeed, and that's why qemu as a client is currently dropping the
>> connection with nbdkit. But I would also make that interpretation for
>> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
>> so maybe it is worth a note in the protocol how to detect servers that
>> are exporting exactly one volume and don't care what name you pass, then
>> tweaking either nbdkit, qemu, or both to comply to that added protocol
>> wording.
>
> I think the correct thing for nbdkit to do would be to return a single
> entry with an arbitrary name.
Still not ideal, because qemu 2.6 won't connect if the arbitrary name
doesn't match qemu's expectations. But qemu 2.6 WILL connect if nbdkit
rejects the command instead of succeeding the command with 0 exports (my
Option 2 above).
>
> I can't see much harm in a client being 'nice' if it gets an UNSUP
> error, but other errors (e.g. TLSREQD) have to be respected as errors.
That's what qemu 2.6 does: UNSUP errors are special cased to continue
negotiation, all other errors are treated as "can't possibly succeed" so
it terminates immediately. The interoperability problem is that
returning 0 names (or even 1 name but where the arbitrary name doesn't
match qemu's expectation) is ALSO treated as grounds for "my export is
not available, give up". So the more I type, the more I'm leaning
towards Option 2: nbdkit should reject NBD_OPT_LIST with
NBD_REP_ERR_UNSUP, rather than succeeding it.
>
>> I've hinted at it above - either an explicit statement that servers
>> cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
>> other exports, then the SHOULD include an NBD_REP_SERVER with name "";
>> or an explicit statement that if a server rejects NBD_OPT_LIST, then the
>> client SHOULD assume that any name will work for
>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO.
>
> I think it's simpler than that. Servers claiming to implement fixed
> newstyle in my view need to implement all the options unless the options
> are marked as optional. I admit (now) those could be clarified.
But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
_are_ optional. In fact, I'm arguing that per Option 2, we WANT
NBD_OPT_LIST to be optional for servers that don't care about names.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:50 ` Eric Blake
@ 2016-05-17 17:34 ` Alex Bligh
2016-05-21 21:53 ` Wouter Verhelst
1 sibling, 0 replies; 51+ messages in thread
From: Alex Bligh @ 2016-05-17 17:34 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, Richard W.M. Jones, Paolo Bonzini,
Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]
On 17 May 2016, at 17:50, Eric Blake <eblake@redhat.com> wrote:
>
> Option 1: I think what I would prefer is a doc patch to the NBD protocol
> that explicitly states that returning 0 names followed by NBD_REP_ACK
> (current nbdkit behavior) implies that the server doesn't honor names at
> all, and will let ANY name work for NBD_OPT_EXPORT_NAME/NBD_OPT_GO; then
> patch qemu client to treat 0 names the same as NBD_REP_UNSUP (the server
> doesn't honor names, so assume any name will work).
...
> But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
> _are_ optional. In fact, I'm arguing that per Option 2, we WANT
> NBD_OPT_LIST to be optional for servers that don't care about names.
Option 3: (which I would prefer) fix the wording indicating what
options are mandatory (I think they all should be but that's another
discussion really), and in the meantime simply have nbdkit return
the empty string as an export name in NBD_OPT_LIST as you
originally suggested, which is what qemu will connect to by default,
and which is what other clients connect to by default.
I don't want to be an arse about this, but I think the standard is
meant to be be normative, IE influence behaviour, rather than just
document existing behaviour. In this instance I think nbdkit
is actually wrong (returning zero entries cannot be right as
that explicitly says nothing is exported, whereas an error is
forgiveable perhaps), and the author is OK to fix it, so I think
we should fix the software to confirm to the standard, not the
standard to conform to the software.
--
Alex Bligh
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:50 ` Eric Blake
2016-05-17 17:34 ` Alex Bligh
@ 2016-05-21 21:53 ` Wouter Verhelst
2016-05-22 18:16 ` Richard W.M. Jones
1 sibling, 1 reply; 51+ messages in thread
From: Wouter Verhelst @ 2016-05-21 21:53 UTC (permalink / raw)
To: Eric Blake
Cc: Alex Bligh, nbd-general, Paolo Bonzini, Daniel P. Berrange,
Richard W.M. Jones, qemu-devel
On Tue, May 17, 2016 at 10:50:01AM -0600, Eric Blake wrote:
> Option 2: An alternative solution would be to allow nbdkit to fail
> NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
> should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
> It is the fact that it is returning NBD_REP_ACK with 0 names that is
> giving qemu grief.
I think this makes most sense. If you don't look at export names, you
effectively don't support them, and you can't be expected to send a list
of "supported" names, because *everything* is supported (or, put a
different way, nothing is).
I note that nbdkit has been patched to now send the empty name, which
is also fine as a way to fix interoperability in this particular case --
but for the general case, I think if we want to define ways for a server
to explicitly say that it doesn't support export names, returning
ERR_UNSUP to OPT_LIST seems cleaner.
(That does mean I'll have to fix nbd-client, since it currently assumes
that fixed newstyle implies support for OPT_LIST -- but that should be
an easy enough patch)
> But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
> _are_ optional. In fact, I'm arguing that per Option 2, we WANT
> NBD_OPT_LIST to be optional for servers that don't care about names.
Sortof. Like I said, nbd-client assumes that servers which support fixed
newstyle will also support OPT_LIST -- but that's mostly an artefact of
the fact that OPT_LIST was used as a test case for fixed newstyle, and
isn't necessarily a good enough reason to make that a required part of
the protocol.
--
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
people in the world who think they really understand all of its rules,
and pretty much all of them are just lying to themselves too.
-- #debian-devel, OFTC, 2016-02-12
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-21 21:53 ` Wouter Verhelst
@ 2016-05-22 18:16 ` Richard W.M. Jones
0 siblings, 0 replies; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-22 18:16 UTC (permalink / raw)
To: Wouter Verhelst
Cc: Eric Blake, Alex Bligh, nbd-general, Paolo Bonzini,
Daniel P. Berrange, qemu-devel
On Sat, May 21, 2016 at 11:53:12PM +0200, Wouter Verhelst wrote:
> On Tue, May 17, 2016 at 10:50:01AM -0600, Eric Blake wrote:
> > Option 2: An alternative solution would be to allow nbdkit to fail
> > NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
> > should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
> > It is the fact that it is returning NBD_REP_ACK with 0 names that is
> > giving qemu grief.
>
> I think this makes most sense. If you don't look at export names, you
> effectively don't support them, and you can't be expected to send a list
> of "supported" names, because *everything* is supported (or, put a
> different way, nothing is).
>
> I note that nbdkit has been patched to now send the empty name, which
> is also fine as a way to fix interoperability in this particular case --
nbdkit now fully supports export names via the '-e' option.
This discussion is moot.
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:22 ` [Qemu-devel] [Nbd] " Alex Bligh
2016-05-17 15:52 ` Eric Blake
@ 2016-05-17 15:54 ` Richard W.M. Jones
2016-05-17 16:26 ` Alex Bligh
1 sibling, 1 reply; 51+ messages in thread
From: Richard W.M. Jones @ 2016-05-17 15:54 UTC (permalink / raw)
To: Alex Bligh
Cc: Eric Blake, Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
> compulsory, even if you support it by returning a nameless export
> (or default). Moreover support of export names is compulsory
> (even if you have a single fixed one called "" or "default").
The protocol doc doesn't mention "default" (assuming you mean that
literal string). It says:
A special, "empty", name (i.e., the length field is zero and no name
is specified), is reserved for a "default" export, to be used in
cases where explicitly specifying an export name makes no sense.
which seems to indicate only the empty string should be used for this
case.
Unfortunately qemu 2.5 (as a client) treated exportname == "" as
meaning that the old-style protocol should be used. This is at least
fixed in qemu 2.6, so nbdkit could now answer NBD_OPT_LIST with a
single empty string, although we lose backwards compatibility with
qemu 2.5.
> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
> specification. If a server doesn't implement mandatory parts of
> the specification, then bad things will happen.
It implements it, it's just that there wasn't a way to implement
anything other than returning an error since we accept anything as an
export name.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 15:54 ` Richard W.M. Jones
@ 2016-05-17 16:26 ` Alex Bligh
2016-05-17 17:00 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Alex Bligh @ 2016-05-17 16:26 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Alex Bligh, Eric Blake, Paolo Bonzini, Daniel P. Berrange,
nbd-general, qemu-devel
On 17 May 2016, at 16:54, Richard W.M. Jones <rjones@redhat.com> wrote:
> On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
>> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
>> compulsory, even if you support it by returning a nameless export
>> (or default). Moreover support of export names is compulsory
>> (even if you have a single fixed one called "" or "default").
>
> The protocol doc doesn't mention "default" (assuming you mean that
> literal string). It says:
As per my message to Eric, I meant use an arbitrary piece of
text in your reply to NBD_OPT_LIST (it doesn't matter what it is,
'default' is not special). It doesn't matter because you ignore
what is passed in NBD_OPT_EXPORT_NAME. I was just suggesting making
things more readable for clients that did a list.
>> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
>> specification. If a server doesn't implement mandatory parts of
>> the specification, then bad things will happen.
>
> It implements it, it's just that there wasn't a way to implement
> anything other than returning an error since we accept anything as an
> export name.
In which case you should just return an export name. Any
export name will work. The fact you don't have names for your
exports doesn't mean (in my book) you can error the list
request. You have one export. You don't know what it's name is,
but you should list it.
--
Alex Bligh
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol
2016-05-17 16:26 ` Alex Bligh
@ 2016-05-17 17:00 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2016-05-17 17:00 UTC (permalink / raw)
To: Alex Bligh, Richard W.M. Jones
Cc: Paolo Bonzini, Daniel P. Berrange, nbd-general, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]
On 05/17/2016 10:26 AM, Alex Bligh wrote:
>
> On 17 May 2016, at 16:54, Richard W.M. Jones <rjones@redhat.com> wrote:
>
>> On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
>>> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
>>> compulsory, even if you support it by returning a nameless export
>>> (or default). Moreover support of export names is compulsory
>>> (even if you have a single fixed one called "" or "default").
>>
>> The protocol doc doesn't mention "default" (assuming you mean that
>> literal string). It says:
>
> As per my message to Eric, I meant use an arbitrary piece of
> text in your reply to NBD_OPT_LIST (it doesn't matter what it is,
> 'default' is not special). It doesn't matter because you ignore
> what is passed in NBD_OPT_EXPORT_NAME. I was just suggesting making
> things more readable for clients that did a list.
>
>>> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
>>> specification. If a server doesn't implement mandatory parts of
>>> the specification, then bad things will happen.
>>
>> It implements it, it's just that there wasn't a way to implement
>> anything other than returning an error since we accept anything as an
>> export name.
>
> In which case you should just return an export name. Any
> export name will work. The fact you don't have names for your
> exports doesn't mean (in my book) you can error the list
> request. You have one export. You don't know what it's name is,
> but you should list it.
Except that the qemu 2.6 client is specifically checking that the name
it is looking for is present in the list, and gives up trying if the
name was not found. And there is no way for the server to know a priori
which name the client is trying to match. An arbitrary name is less
likely to be found; the empty name "" is the most likely arbitrary name,
but even then, there is no guarantee that the qemu client is using the
"" default name, and since NBD_OPT_EXPORT_NAME doesn't care, I think
it's nicer to state that NBD_OPT_LIST should ONLY be supported if names
matter to NBD_OPT_EXPORT_NAME (and should return NBD_REP_ERR_UNSUP when
names don't matter).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PULL 24/28] nbd: use "" as a default export name if none provided
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (22 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 25/28] nbd: implement TLS support in the protocol negotiation Paolo Bonzini
` (4 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
If the user does not provide an export name and the server
is running the new style protocol, where export names are
mandatory, use "" as the default export name if the user
has not specified any. "" is defined in the NBD protocol
as the default name to use in such scenarios.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-13-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/client.c | 4 ++--
nbd/server.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index be5f08d..5e47ac7 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -315,8 +315,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
goto fail;
}
if (!name) {
- error_setg(errp, "Server requires an export name");
- goto fail;
+ TRACE("Using default NBD export name \"\"");
+ name = "";
}
if (fixedNewStyle) {
/* Check our desired export is present in the
diff --git a/nbd/server.c b/nbd/server.c
index 3d2fb10..9fee1d4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -220,6 +220,7 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
uint64_t magic, name_len;
uint32_t opt, type, len;
+ TRACE("Advertizing export name '%s'", exp->name ? exp->name : "");
name_len = strlen(exp->name);
magic = cpu_to_be64(NBD_REP_MAGIC);
if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 25/28] nbd: implement TLS support in the protocol negotiation
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (23 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 24/28] nbd: use "" as a default export name if none provided Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 26/28] nbd: enable use of TLS with NBD block driver Paolo Bonzini
` (3 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This extends the NBD protocol handling code so that it is capable
of negotiating TLS support during the connection setup. This involves
requesting the STARTTLS protocol option before any other NBD options.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-14-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd-client.c | 12 +++--
blockdev-nbd.c | 2 +-
include/block/nbd.h | 8 ++++
nbd/client.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++-
nbd/common.c | 15 ++++++
nbd/nbd-internal.h | 14 ++++++
nbd/server.c | 118 ++++++++++++++++++++++++++++++++++++++++++---
qemu-nbd.c | 4 +-
8 files changed, 296 insertions(+), 13 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 75bb2d9..1c79e4b 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -405,7 +405,10 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
- &client->nbdflags, &client->size, errp);
+ &client->nbdflags,
+ NULL, NULL,
+ &client->ioc,
+ &client->size, errp);
if (ret < 0) {
logout("Failed to negotiate with the NBD server\n");
return ret;
@@ -415,8 +418,11 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
qemu_co_mutex_init(&client->free_sema);
client->sioc = sioc;
object_ref(OBJECT(client->sioc));
- client->ioc = QIO_CHANNEL(sioc);
- object_ref(OBJECT(client->ioc));
+
+ if (!client->ioc) {
+ client->ioc = QIO_CHANNEL(sioc);
+ object_ref(OBJECT(client->ioc));
+ }
/* Now that we're connected, set the socket to be non-blocking and
* kick the reply mechanism. */
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 2148753..f181840 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -34,7 +34,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
return TRUE;
}
- nbd_client_new(NULL, cioc, nbd_client_put);
+ nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
object_unref(OBJECT(cioc));
return TRUE;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 1080ef8..b197adc 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -24,6 +24,7 @@
#include "qemu-common.h"
#include "qemu/option.h"
#include "io/channel-socket.h"
+#include "crypto/tlscreds.h"
struct nbd_request {
uint32_t magic;
@@ -56,7 +57,10 @@ struct nbd_reply {
#define NBD_REP_ACK (1) /* Data sending finished. */
#define NBD_REP_SERVER (2) /* Export description. */
#define NBD_REP_ERR_UNSUP ((UINT32_C(1) << 31) | 1) /* Unknown option. */
+#define NBD_REP_ERR_POLICY ((UINT32_C(1) << 31) | 2) /* Server denied */
#define NBD_REP_ERR_INVALID ((UINT32_C(1) << 31) | 3) /* Invalid length. */
+#define NBD_REP_ERR_TLS_REQD ((UINT32_C(1) << 31) | 5) /* TLS required */
+
#define NBD_CMD_MASK_COMMAND 0x0000ffff
#define NBD_CMD_FLAG_FUA (1 << 16)
@@ -81,6 +85,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
size_t length,
bool do_read);
int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+ QCryptoTLSCreds *tlscreds, const char *hostname,
+ QIOChannel **outioc,
off_t *size, Error **errp);
int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
@@ -106,6 +112,8 @@ void nbd_export_close_all(void);
void nbd_client_new(NBDExport *exp,
QIOChannelSocket *sioc,
+ QCryptoTLSCreds *tlscreds,
+ const char *tlsaclname,
void (*close)(NBDClient *));
void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
diff --git a/nbd/client.c b/nbd/client.c
index 5e47ac7..9e5b651 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -83,10 +83,18 @@ static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
error_setg(errp, "Unsupported option type %x", opt);
break;
+ case NBD_REP_ERR_POLICY:
+ error_setg(errp, "Denied by server for option %x", opt);
+ break;
+
case NBD_REP_ERR_INVALID:
error_setg(errp, "Invalid data length for option %x", opt);
break;
+ case NBD_REP_ERR_TLS_REQD:
+ error_setg(errp, "TLS negotiation required before option %x", opt);
+ break;
+
default:
error_setg(errp, "Unknown error code when asking for option %x", opt);
break;
@@ -242,17 +250,127 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
return 0;
}
+static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
+ QCryptoTLSCreds *tlscreds,
+ const char *hostname, Error **errp)
+{
+ uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
+ uint32_t opt = cpu_to_be32(NBD_OPT_STARTTLS);
+ uint32_t length = 0;
+ uint32_t type;
+ QIOChannelTLS *tioc;
+ struct NBDTLSHandshakeData data = { 0 };
+
+ TRACE("Requesting TLS from server");
+ if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+ error_setg(errp, "Failed to send option magic");
+ return NULL;
+ }
+
+ if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+ error_setg(errp, "Failed to send option number");
+ return NULL;
+ }
+
+ if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
+ error_setg(errp, "Failed to send option length");
+ return NULL;
+ }
+
+ TRACE("Getting TLS reply from server1");
+ if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+ error_setg(errp, "failed to read option magic");
+ return NULL;
+ }
+ magic = be64_to_cpu(magic);
+ if (magic != NBD_REP_MAGIC) {
+ error_setg(errp, "Unexpected option magic");
+ return NULL;
+ }
+ TRACE("Getting TLS reply from server2");
+ if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+ error_setg(errp, "failed to read option");
+ return NULL;
+ }
+ opt = be32_to_cpu(opt);
+ if (opt != NBD_OPT_STARTTLS) {
+ error_setg(errp, "Unexpected option type %x expected %x",
+ opt, NBD_OPT_STARTTLS);
+ return NULL;
+ }
+
+ TRACE("Getting TLS reply from server");
+ if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) {
+ error_setg(errp, "failed to read option type");
+ return NULL;
+ }
+ type = be32_to_cpu(type);
+ if (type != NBD_REP_ACK) {
+ error_setg(errp, "Server rejected request to start TLS %x",
+ type);
+ return NULL;
+ }
+
+ TRACE("Getting TLS reply from server");
+ if (read_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
+ error_setg(errp, "failed to read option length");
+ return NULL;
+ }
+ length = be32_to_cpu(length);
+ if (length != 0) {
+ error_setg(errp, "Start TLS reponse was not zero %x",
+ length);
+ return NULL;
+ }
+
+ TRACE("TLS request approved, setting up TLS");
+ tioc = qio_channel_tls_new_client(ioc, tlscreds, hostname, errp);
+ if (!tioc) {
+ return NULL;
+ }
+ data.loop = g_main_loop_new(g_main_context_default(), FALSE);
+ TRACE("Starting TLS hanshake");
+ qio_channel_tls_handshake(tioc,
+ nbd_tls_handshake,
+ &data,
+ NULL);
+
+ if (!data.complete) {
+ g_main_loop_run(data.loop);
+ }
+ g_main_loop_unref(data.loop);
+ if (data.error) {
+ error_propagate(errp, data.error);
+ object_unref(OBJECT(tioc));
+ return NULL;
+ }
+
+ return QIO_CHANNEL(tioc);
+}
+
+
int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+ QCryptoTLSCreds *tlscreds, const char *hostname,
+ QIOChannel **outioc,
off_t *size, Error **errp)
{
char buf[256];
uint64_t magic, s;
int rc;
- TRACE("Receiving negotiation.");
+ TRACE("Receiving negotiation tlscreds=%p hostname=%s.",
+ tlscreds, hostname ? hostname : "<null>");
rc = -EINVAL;
+ if (outioc) {
+ *outioc = NULL;
+ }
+ if (tlscreds && !outioc) {
+ error_setg(errp, "Output I/O channel required for TLS");
+ goto fail;
+ }
+
if (read_sync(ioc, buf, 8) != 8) {
error_setg(errp, "Failed to read data");
goto fail;
@@ -314,6 +432,18 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
error_setg(errp, "Failed to send clientflags field");
goto fail;
}
+ if (tlscreds) {
+ if (fixedNewStyle) {
+ *outioc = nbd_receive_starttls(ioc, tlscreds, hostname, errp);
+ if (!*outioc) {
+ goto fail;
+ }
+ ioc = *outioc;
+ } else {
+ error_setg(errp, "Server does not support STARTTLS");
+ goto fail;
+ }
+ }
if (!name) {
TRACE("Using default NBD export name \"\"");
name = "";
@@ -371,6 +501,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
error_setg(errp, "Server does not support export names");
goto fail;
}
+ if (tlscreds) {
+ error_setg(errp, "Server does not support STARTTLS");
+ goto fail;
+ }
if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
error_setg(errp, "Failed to read export length");
diff --git a/nbd/common.c b/nbd/common.c
index 2b19c95..bde673a 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -75,3 +75,18 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
g_free(local_iov_head);
return done;
}
+
+
+void nbd_tls_handshake(Object *src,
+ Error *err,
+ void *opaque)
+{
+ struct NBDTLSHandshakeData *data = opaque;
+
+ if (err) {
+ TRACE("TLS failed %s", error_get_pretty(err));
+ data->error = error_copy(err);
+ }
+ data->complete = true;
+ g_main_loop_quit(data->loop);
+}
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 9960034..db6ab65 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -11,6 +11,7 @@
#define NBD_INTERNAL_H
#include "block/nbd.h"
#include "sysemu/block-backend.h"
+#include "io/channel-tls.h"
#include "qemu/coroutine.h"
#include "qemu/iov.h"
@@ -78,6 +79,8 @@
#define NBD_OPT_EXPORT_NAME (1)
#define NBD_OPT_ABORT (2)
#define NBD_OPT_LIST (3)
+#define NBD_OPT_PEEK_EXPORT (4)
+#define NBD_OPT_STARTTLS (5)
/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
* but only a limited set of errno values is specified in the protocol.
@@ -108,4 +111,15 @@ static inline ssize_t write_sync(QIOChannel *ioc, void *buffer, size_t size)
return nbd_wr_syncv(ioc, &iov, 1, 0, size, false);
}
+struct NBDTLSHandshakeData {
+ GMainLoop *loop;
+ bool complete;
+ Error *error;
+};
+
+
+void nbd_tls_handshake(Object *src,
+ Error *err,
+ void *opaque);
+
#endif
diff --git a/nbd/server.c b/nbd/server.c
index 9fee1d4..d4225cd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -76,6 +76,8 @@ struct NBDClient {
void (*close)(NBDClient *client);
NBDExport *exp;
+ QCryptoTLSCreds *tlscreds;
+ char *tlsaclname;
QIOChannelSocket *sioc; /* The underlying data channel */
QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -192,6 +194,8 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
uint64_t magic;
uint32_t len;
+ TRACE("Reply opt=%x type=%x", type, opt);
+
magic = cpu_to_be64(NBD_REP_MAGIC);
if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
LOG("write failed (rep magic)");
@@ -310,6 +314,55 @@ fail:
return rc;
}
+
+static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
+ uint32_t length)
+{
+ QIOChannel *ioc;
+ QIOChannelTLS *tioc;
+ struct NBDTLSHandshakeData data = { 0 };
+
+ TRACE("Setting up TLS");
+ ioc = client->ioc;
+ if (length) {
+ if (nbd_negotiate_drop_sync(ioc, length) != length) {
+ return NULL;
+ }
+ nbd_negotiate_send_rep(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS);
+ return NULL;
+ }
+
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_STARTTLS);
+
+ tioc = qio_channel_tls_new_server(ioc,
+ client->tlscreds,
+ client->tlsaclname,
+ NULL);
+ if (!tioc) {
+ return NULL;
+ }
+
+ TRACE("Starting TLS handshake");
+ data.loop = g_main_loop_new(g_main_context_default(), FALSE);
+ qio_channel_tls_handshake(tioc,
+ nbd_tls_handshake,
+ &data,
+ NULL);
+
+ if (!data.complete) {
+ g_main_loop_run(data.loop);
+ }
+ g_main_loop_unref(data.loop);
+ if (data.error) {
+ object_unref(OBJECT(tioc));
+ error_free(data.error);
+ return NULL;
+ }
+
+ return QIO_CHANNEL(tioc);
+}
+
+
static int nbd_negotiate_options(NBDClient *client)
{
uint32_t flags;
@@ -377,7 +430,30 @@ static int nbd_negotiate_options(NBDClient *client)
length = be32_to_cpu(length);
TRACE("Checking option 0x%x", clientflags);
- if (fixedNewstyle) {
+ if (client->tlscreds &&
+ client->ioc == (QIOChannel *)client->sioc) {
+ QIOChannel *tioc;
+ if (!fixedNewstyle) {
+ TRACE("Unsupported option 0x%x", clientflags);
+ return -EINVAL;
+ }
+ switch (clientflags) {
+ case NBD_OPT_STARTTLS:
+ tioc = nbd_negotiate_handle_starttls(client, length);
+ if (!tioc) {
+ return -EIO;
+ }
+ object_unref(OBJECT(client->ioc));
+ client->ioc = QIO_CHANNEL(tioc);
+ break;
+
+ default:
+ TRACE("Option 0x%x not permitted before TLS", clientflags);
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_REQD,
+ clientflags);
+ return -EINVAL;
+ }
+ } else if (fixedNewstyle) {
switch (clientflags) {
case NBD_OPT_LIST:
ret = nbd_negotiate_handle_list(client, length);
@@ -392,6 +468,17 @@ static int nbd_negotiate_options(NBDClient *client)
case NBD_OPT_EXPORT_NAME:
return nbd_negotiate_handle_export_name(client, length);
+ case NBD_OPT_STARTTLS:
+ if (client->tlscreds) {
+ TRACE("TLS already enabled");
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_INVALID,
+ clientflags);
+ } else {
+ TRACE("TLS not configured");
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_POLICY,
+ clientflags);
+ }
+ return -EINVAL;
default:
TRACE("Unsupported option 0x%x", clientflags);
nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP,
@@ -427,8 +514,9 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
int rc;
const int myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
+ bool oldStyle;
- /* Negotiation header without options:
+ /* Old style negotiation header without options
[ 0 .. 7] passwd ("NBDMAGIC")
[ 8 .. 15] magic (NBD_CLIENT_MAGIC)
[16 .. 23] size
@@ -436,12 +524,11 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
[26 .. 27] export flags
[28 .. 151] reserved (0)
- Negotiation header with options, part 1:
+ New style negotiation header with options
[ 0 .. 7] passwd ("NBDMAGIC")
[ 8 .. 15] magic (NBD_OPTS_MAGIC)
[16 .. 17] server flags (0)
-
- part 2 (after options are sent):
+ ....options sent....
[18 .. 25] size
[26 .. 27] export flags
[28 .. 151] reserved (0)
@@ -453,7 +540,9 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
TRACE("Beginning negotiation.");
memset(buf, 0, sizeof(buf));
memcpy(buf, "NBDMAGIC", 8);
- if (client->exp) {
+
+ oldStyle = client->exp != NULL && !client->tlscreds;
+ if (oldStyle) {
assert ((client->exp->nbdflags & ~65535) == 0);
stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
stq_be_p(buf + 16, client->exp->size);
@@ -463,7 +552,11 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE);
}
- if (client->exp) {
+ if (oldStyle) {
+ if (client->tlscreds) {
+ TRACE("TLS cannot be enabled with oldstyle protocol");
+ goto fail;
+ }
if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) != sizeof(buf)) {
LOG("write failed");
goto fail;
@@ -602,6 +695,10 @@ void nbd_client_put(NBDClient *client)
nbd_unset_handlers(client);
object_unref(OBJECT(client->sioc));
object_unref(OBJECT(client->ioc));
+ if (client->tlscreds) {
+ object_unref(OBJECT(client->tlscreds));
+ }
+ g_free(client->tlsaclname);
if (client->exp) {
QTAILQ_REMOVE(&client->exp->clients, client, next);
nbd_export_put(client->exp);
@@ -1150,6 +1247,8 @@ out:
void nbd_client_new(NBDExport *exp,
QIOChannelSocket *sioc,
+ QCryptoTLSCreds *tlscreds,
+ const char *tlsaclname,
void (*close_fn)(NBDClient *))
{
NBDClient *client;
@@ -1158,6 +1257,11 @@ void nbd_client_new(NBDExport *exp,
client = g_malloc0(sizeof(NBDClient));
client->refcount = 1;
client->exp = exp;
+ client->tlscreds = tlscreds;
+ if (tlscreds) {
+ object_ref(OBJECT(client->tlscreds));
+ }
+ client->tlsaclname = g_strdup(tlsaclname);
client->sioc = sioc;
object_ref(OBJECT(client->sioc));
client->ioc = QIO_CHANNEL(sioc);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9710a26..8acd515 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -251,6 +251,7 @@ static void *nbd_client_thread(void *arg)
}
ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
+ NULL, NULL, NULL,
&size, &local_error);
if (ret < 0) {
if (local_error) {
@@ -340,7 +341,8 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
nb_fds++;
nbd_update_server_watch();
- nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed);
+ nbd_client_new(newproto ? NULL : exp, cioc,
+ NULL, NULL, nbd_client_closed);
object_unref(OBJECT(cioc));
return TRUE;
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 26/28] nbd: enable use of TLS with NBD block driver
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (24 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 25/28] nbd: implement TLS support in the protocol negotiation Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 27/28] nbd: enable use of TLS with qemu-nbd server Paolo Bonzini
` (2 subsequent siblings)
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This modifies the NBD driver so that it is possible to request
use of TLS. This is done by providing the 'tls-creds' parameter
with the ID of a previously created QCryptoTLSCreds object.
For example
$QEMU -object tls-creds-x509,id=tls0,endpoint=client,\
dir=/home/berrange/security/qemutls \
-drive driver=nbd,host=localhost,port=9000,tls-creds=tls0
The client will drop the connection if the NBD server does not
provide TLS.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-15-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd-client.c | 10 ++++---
block/nbd-client.h | 2 ++
block/nbd.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 78 insertions(+), 12 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 1c79e4b..6a9b4c7 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -394,8 +394,12 @@ void nbd_client_close(BlockDriverState *bs)
nbd_teardown_connection(bs);
}
-int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
- const char *export, Error **errp)
+int nbd_client_init(BlockDriverState *bs,
+ QIOChannelSocket *sioc,
+ const char *export,
+ QCryptoTLSCreds *tlscreds,
+ const char *hostname,
+ Error **errp)
{
NbdClientSession *client = nbd_get_client_session(bs);
int ret;
@@ -406,7 +410,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
&client->nbdflags,
- NULL, NULL,
+ tlscreds, hostname,
&client->ioc,
&client->size, errp);
if (ret < 0) {
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e8b3283..53f116d 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -39,6 +39,8 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
int nbd_client_init(BlockDriverState *bs,
QIOChannelSocket *sock,
const char *export_name,
+ QCryptoTLSCreds *tlscreds,
+ const char *hostname,
Error **errp);
void nbd_client_close(BlockDriverState *bs);
diff --git a/block/nbd.c b/block/nbd.c
index d7116e2..db57b49 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -258,36 +258,92 @@ static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
return sioc;
}
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+ Object *obj;
+ QCryptoTLSCreds *creds;
+
+ obj = object_resolve_path_component(
+ object_get_objects_root(), id);
+ if (!obj) {
+ error_setg(errp, "No TLS credentials with id '%s'",
+ id);
+ return NULL;
+ }
+ creds = (QCryptoTLSCreds *)
+ object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+ if (!creds) {
+ error_setg(errp, "Object with id '%s' is not TLS credentials",
+ id);
+ return NULL;
+ }
+
+ if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
+ error_setg(errp,
+ "Expecting TLS credentials with a client endpoint");
+ return NULL;
+ }
+ object_ref(obj);
+ return creds;
+}
+
+
static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVNBDState *s = bs->opaque;
char *export = NULL;
- int result;
- QIOChannelSocket *sioc;
+ QIOChannelSocket *sioc = NULL;
SocketAddress *saddr;
+ const char *tlscredsid;
+ QCryptoTLSCreds *tlscreds = NULL;
+ const char *hostname = NULL;
+ int ret = -EINVAL;
/* Pop the config into our state object. Exit if invalid. */
saddr = nbd_config(s, options, &export, errp);
if (!saddr) {
- return -EINVAL;
+ goto error;
+ }
+
+ tlscredsid = g_strdup(qdict_get_try_str(options, "tls-creds"));
+ if (tlscredsid) {
+ qdict_del(options, "tls-creds");
+ tlscreds = nbd_get_tls_creds(tlscredsid, errp);
+ if (!tlscreds) {
+ goto error;
+ }
+
+ if (saddr->type != SOCKET_ADDRESS_KIND_INET) {
+ error_setg(errp, "TLS only supported over IP sockets");
+ goto error;
+ }
+ hostname = saddr->u.inet->host;
}
/* establish TCP connection, return error if it fails
* TODO: Configurable retry-until-timeout behaviour.
*/
sioc = nbd_establish_connection(saddr, errp);
- qapi_free_SocketAddress(saddr);
if (!sioc) {
- g_free(export);
- return -ECONNREFUSED;
+ ret = -ECONNREFUSED;
+ goto error;
}
/* NBD handshake */
- result = nbd_client_init(bs, sioc, export, errp);
- object_unref(OBJECT(sioc));
+ ret = nbd_client_init(bs, sioc, export,
+ tlscreds, hostname, errp);
+ error:
+ if (sioc) {
+ object_unref(OBJECT(sioc));
+ }
+ if (tlscreds) {
+ object_unref(OBJECT(tlscreds));
+ }
+ qapi_free_SocketAddress(saddr);
g_free(export);
- return result;
+ return ret;
}
static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -349,6 +405,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
const char *host = qdict_get_try_str(options, "host");
const char *port = qdict_get_try_str(options, "port");
const char *export = qdict_get_try_str(options, "export");
+ const char *tlscreds = qdict_get_try_str(options, "tls-creds");
qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
@@ -383,6 +440,9 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
if (export) {
qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
}
+ if (tlscreds) {
+ qdict_put_obj(opts, "tls-creds", QOBJECT(qstring_from_str(tlscreds)));
+ }
bs->full_open_options = opts;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 27/28] nbd: enable use of TLS with qemu-nbd server
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (25 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 26/28] nbd: enable use of TLS with NBD block driver Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 16:34 ` [Qemu-devel] [PULL 28/28] nbd: enable use of TLS with nbd-server-start command Paolo Bonzini
2016-02-16 18:25 ` [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Peter Maydell
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This modifies the qemu-nbd program so that it is possible to
request the use of TLS with the server. It simply adds a new
command line option --tls-creds which is used to provide the
ID of a QCryptoTLSCreds object previously created via the
--object command line option.
For example
qemu-nbd --object tls-creds-x509,id=tls0,endpoint=server,\
dir=/home/berrange/security/qemutls \
--tls-creds tls0 \
--exportname default
TLS requires the new style NBD protocol, so if no export name
is set (via --export-name), then we use the default NBD protocol
export name ""
TLS is only supported when using an IPv4/IPv6 socket listener.
It is not possible to use with UNIX sockets, which includes
when connecting the NBD server to a host device.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-16-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-nbd.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
qemu-nbd.texi | 9 +++++++--
2 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 8acd515..933ca4a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -42,6 +42,7 @@
#define QEMU_NBD_OPT_DISCARD 3
#define QEMU_NBD_OPT_DETECT_ZEROES 4
#define QEMU_NBD_OPT_OBJECT 5
+#define QEMU_NBD_OPT_TLSCREDS 6
static NBDExport *exp;
static bool newproto;
@@ -54,6 +55,7 @@ static int shared = 1;
static int nb_fds;
static QIOChannelSocket *server_ioc;
static int server_watch = -1;
+static QCryptoTLSCreds *tlscreds;
static void usage(const char *name)
{
@@ -342,7 +344,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
nb_fds++;
nbd_update_server_watch();
nbd_client_new(newproto ? NULL : exp, cioc,
- NULL, NULL, nbd_client_closed);
+ tlscreds, NULL, nbd_client_closed);
object_unref(OBJECT(cioc));
return TRUE;
@@ -402,6 +404,37 @@ static QemuOptsList qemu_object_opts = {
};
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+ Object *obj;
+ QCryptoTLSCreds *creds;
+
+ obj = object_resolve_path_component(
+ object_get_objects_root(), id);
+ if (!obj) {
+ error_setg(errp, "No TLS credentials with id '%s'",
+ id);
+ return NULL;
+ }
+ creds = (QCryptoTLSCreds *)
+ object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+ if (!creds) {
+ error_setg(errp, "Object with id '%s' is not TLS credentials",
+ id);
+ return NULL;
+ }
+
+ if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+ error_setg(errp,
+ "Expecting TLS credentials with a server endpoint");
+ return NULL;
+ }
+ object_ref(obj);
+ return creds;
+}
+
+
int main(int argc, char **argv)
{
BlockBackend *blk;
@@ -441,6 +474,7 @@ int main(int argc, char **argv)
{ "verbose", 0, NULL, 'v' },
{ "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
{ "export-name", 1, NULL, 'x' },
+ { "tls-creds", 1, NULL, QEMU_NBD_OPT_TLSCREDS },
{ NULL, 0, NULL, 0 }
};
int ch;
@@ -458,6 +492,7 @@ int main(int argc, char **argv)
BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
QDict *options = NULL;
const char *export_name = NULL;
+ const char *tlscredsid = NULL;
/* The client thread uses SIGTERM to interrupt the server. A signal
* handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -634,6 +669,9 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
} break;
+ case QEMU_NBD_OPT_TLSCREDS:
+ tlscredsid = optarg;
+ break;
}
}
@@ -650,6 +688,28 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
+ if (tlscredsid) {
+ if (sockpath) {
+ error_report("TLS is only supported with IPv4/IPv6");
+ exit(EXIT_FAILURE);
+ }
+ if (device) {
+ error_report("TLS is not supported with a host device");
+ exit(EXIT_FAILURE);
+ }
+ if (!export_name) {
+ /* Set the default NBD protocol export name, since
+ * we *must* use new style protocol for TLS */
+ export_name = "";
+ }
+ tlscreds = nbd_get_tls_creds(tlscredsid, &local_err);
+ if (local_err) {
+ error_report("Failed to get TLS creds %s",
+ error_get_pretty(local_err));
+ exit(EXIT_FAILURE);
+ }
+ }
+
if (disconnect) {
int nbdfd = open(argv[optind], O_RDWR);
if (nbdfd < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 874481d..227a73c 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -21,9 +21,10 @@ Export a QEMU disk image using the NBD protocol.
@item --object type,id=@var{id},...props...
Define a new instance of the @var{type} object class identified by @var{id}.
See the @code{qemu(1)} manual page for full details of the properties
-supported. The common object type that it makes sense to define is the
+supported. The common object types that it makes sense to define are the
@code{secret} object, which is used to supply passwords and/or encryption
-keys.
+keys, and the @code{tls-creds} object, which is used to supply TLS
+credentials for the qemu-nbd server.
@item -p, --port=@var{port}
The TCP port to listen on (default @samp{10809})
@item -o, --offset=@var{offset}
@@ -76,6 +77,10 @@ Don't exit on the last connection
@item -x NAME, --export-name=NAME
Set the NBD volume export name. This switches the server to use
the new style NBD protocol negotiation
+@item --tls-creds=ID
+Enable mandatory TLS encryption for the server by setting the ID
+of the TLS credentials object previously created with the --object
+option.
@item -v, --verbose
Display extra debugging information
@item -h, --help
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PULL 28/28] nbd: enable use of TLS with nbd-server-start command
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (26 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 27/28] nbd: enable use of TLS with qemu-nbd server Paolo Bonzini
@ 2016-02-16 16:34 ` Paolo Bonzini
2016-02-16 18:25 ` [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Peter Maydell
28 siblings, 0 replies; 51+ messages in thread
From: Paolo Bonzini @ 2016-02-16 16:34 UTC (permalink / raw)
To: qemu-devel
From: "Daniel P. Berrange" <berrange@redhat.com>
This modifies the nbd-server-start QMP command so that it
is possible to request use of TLS. This is done by adding
a new optional parameter "tls-creds" which provides the ID
of a previously created QCryptoTLSCreds object instance.
TLS is only supported when using an IPv4/IPv6 socket listener.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1455129674-17255-17-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
blockdev-nbd.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++----------
hmp.c | 2 +-
qapi/block.json | 4 +-
qmp-commands.hx | 2 +-
4 files changed, 105 insertions(+), 25 deletions(-)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index f181840..12cae0e 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -20,42 +20,126 @@
#include "block/nbd.h"
#include "io/channel-socket.h"
-static QIOChannelSocket *server_ioc;
-static int server_watch = -1;
+typedef struct NBDServerData {
+ QIOChannelSocket *listen_ioc;
+ int watch;
+ QCryptoTLSCreds *tlscreds;
+} NBDServerData;
+
+static NBDServerData *nbd_server;
+
static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
gpointer opaque)
{
QIOChannelSocket *cioc;
+ if (!nbd_server) {
+ return FALSE;
+ }
+
cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
NULL);
if (!cioc) {
return TRUE;
}
- nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
+ nbd_client_new(NULL, cioc,
+ nbd_server->tlscreds, NULL,
+ nbd_client_put);
object_unref(OBJECT(cioc));
return TRUE;
}
-void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
+
+static void nbd_server_free(NBDServerData *server)
{
- if (server_ioc) {
- error_setg(errp, "NBD server already running");
+ if (!server) {
return;
}
- server_ioc = qio_channel_socket_new();
- if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+ if (server->watch != -1) {
+ g_source_remove(server->watch);
+ }
+ object_unref(OBJECT(server->listen_ioc));
+ if (server->tlscreds) {
+ object_unref(OBJECT(server->tlscreds));
+ }
+
+ g_free(server);
+}
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+ Object *obj;
+ QCryptoTLSCreds *creds;
+
+ obj = object_resolve_path_component(
+ object_get_objects_root(), id);
+ if (!obj) {
+ error_setg(errp, "No TLS credentials with id '%s'",
+ id);
+ return NULL;
+ }
+ creds = (QCryptoTLSCreds *)
+ object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+ if (!creds) {
+ error_setg(errp, "Object with id '%s' is not TLS credentials",
+ id);
+ return NULL;
+ }
+
+ if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+ error_setg(errp,
+ "Expecting TLS credentials with a server endpoint");
+ return NULL;
+ }
+ object_ref(obj);
+ return creds;
+}
+
+
+void qmp_nbd_server_start(SocketAddress *addr,
+ bool has_tls_creds, const char *tls_creds,
+ Error **errp)
+{
+ if (nbd_server) {
+ error_setg(errp, "NBD server already running");
return;
}
- server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
- G_IO_IN,
- nbd_accept,
- NULL,
- NULL);
+ nbd_server = g_new0(NBDServerData, 1);
+ nbd_server->watch = -1;
+ nbd_server->listen_ioc = qio_channel_socket_new();
+ if (qio_channel_socket_listen_sync(
+ nbd_server->listen_ioc, addr, errp) < 0) {
+ goto error;
+ }
+
+ if (has_tls_creds) {
+ nbd_server->tlscreds = nbd_get_tls_creds(tls_creds, errp);
+ if (!nbd_server->tlscreds) {
+ goto error;
+ }
+
+ if (addr->type != SOCKET_ADDRESS_KIND_INET) {
+ error_setg(errp, "TLS is only supported with IPv4/IPv6");
+ goto error;
+ }
+ }
+
+ nbd_server->watch = qio_channel_add_watch(
+ QIO_CHANNEL(nbd_server->listen_ioc),
+ G_IO_IN,
+ nbd_accept,
+ NULL,
+ NULL);
+
+ return;
+
+ error:
+ nbd_server_free(nbd_server);
+ nbd_server = NULL;
}
void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
@@ -64,7 +148,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
BlockBackend *blk;
NBDExport *exp;
- if (!server_ioc) {
+ if (!nbd_server) {
error_setg(errp, "NBD server not running");
return;
}
@@ -110,12 +194,6 @@ void qmp_nbd_server_stop(Error **errp)
{
nbd_export_close_all();
- if (server_watch != -1) {
- g_source_remove(server_watch);
- server_watch = -1;
- }
- if (server_ioc) {
- object_unref(OBJECT(server_ioc));
- server_ioc = NULL;
- }
+ nbd_server_free(nbd_server);
+ nbd_server = NULL;
}
diff --git a/hmp.c b/hmp.c
index 41fb9ca..996cb91 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1793,7 +1793,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
goto exit;
}
- qmp_nbd_server_start(addr, &local_err);
+ qmp_nbd_server_start(addr, false, NULL, &local_err);
qapi_free_SocketAddress(addr);
if (local_err != NULL) {
goto exit;
diff --git a/qapi/block.json b/qapi/block.json
index ed61f82..58e6b30 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -146,13 +146,15 @@
# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
#
# @addr: Address on which to listen.
+# @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
#
# Returns: error if the server is already running.
#
# Since: 1.3.0
##
{ 'command': 'nbd-server-start',
- 'data': { 'addr': 'SocketAddress' } }
+ 'data': { 'addr': 'SocketAddress',
+ '*tls-creds': 'str'} }
##
# @nbd-server-add:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 020e5ee..9fb0d78 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3825,7 +3825,7 @@ EQMP
{
.name = "nbd-server-start",
- .args_type = "addr:q",
+ .args_type = "addr:q,tls-creds:s?",
.mhandler.cmd_new = qmp_marshal_nbd_server_start,
},
{
--
2.5.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16
2016-02-16 16:34 [Qemu-devel] [PULL 00/28] Bug fixes + NBD-over-TLS support patches for 2016-02-16 Paolo Bonzini
` (27 preceding siblings ...)
2016-02-16 16:34 ` [Qemu-devel] [PULL 28/28] nbd: enable use of TLS with nbd-server-start command Paolo Bonzini
@ 2016-02-16 18:25 ` Peter Maydell
28 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2016-02-16 18:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: QEMU Developers
On 16 February 2016 at 16:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The following changes since commit 80b5d6bfc1280fa06e2514a414690c0e5b4b514b:
>
> Merge remote-tracking branch 'remotes/rth/tags/pull-i386-20160215' into staging (2016-02-15 11:45:11 +0000)
>
> are available in the git repository at:
>
> git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to ddffee3904828f11d596a13bd3c8960d747c66d8:
>
> nbd: enable use of TLS with nbd-server-start command (2016-02-16 17:17:49 +0100)
>
> ----------------------------------------------------------------
> * Coverity fixes for IPMI and mptsas
> * qemu-char fixes from Daniel and Marc-André
> * Bug fixes that break qemu-iotests
> * Changes to fix reset from panicked state
> * checkpatch false positives for designated initializers
> * TLS support in the NBD servers and clients
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread