Re: [Qemu-block] [PATCH v2 0/9] iotests: Make them work for both Python 2 and 3
On 10/19/18 3:15 PM, Max Reitz wrote: > This series prepares the iotests to work with both Python 2 and 3. In > some places, it adds version-specific code and decides what to do based > on the version (for instance, whether to import the StringIO or the > BytesIO class from 'io' for use with the test runner), but most of the > time, it just makes code work for both versions in general. > Tested on both Python 2 and 3. Tested-by: Cleber Rosa
Re: [Qemu-block] [PATCH v2 9/9] iotests: Unify log outputs between Python 2 and 3
On 10/19/18 3:15 PM, Max Reitz wrote: > When dumping an object into the log, there are differences between > Python 2 and 3. First, unicode strings are prefixed by 'u' in Python 2 > (they are no longer in 3, because unicode strings are the default > there). Second, the order of keys in dicts may differ. Third, > especially long numbers are longs in Python 2 and thus get an 'L' > suffix, which does not happen in Python 3. > > We can get around all of these differences by dumping objects (lists and > dicts) in a language-independent format, namely JSON. The JSON > generator even allows emitting dicts with their keys sorted > alphabetically. > > This changes the output of all tests that use these logging functions > (dict keys are ordered now, strings in dicts are now enclosed in double > quotes instead of single quotes, the 'L' suffix of large integers is > dropped, and "true" and "false" are now in lower case). > The quote change necessitates a small change to a filter used in test > 207. > > Suggested-by: Eduardo Habkost > Signed-off-by: Max Reitz Reviewed-by: Cleber Rosa
Re: [Qemu-block] [PATCH v2 8/9] iotests: Modify imports for Python 3
On 10/19/18 3:15 PM, Max Reitz wrote: > There are two imports that need to be modified when running the iotests > under Python 3: One is StringIO, which no longer exists; instead, the > StringIO class comes from the io module, so import it from there (and > use the BytesIO class for Python 2). The other is the ConfigParser, > which has just been renamed to configparser. > > Signed-off-by: Max Reitz Reviewed-by: Cleber Rosa
Re: [Qemu-block] [PATCH v2 7/9] iotests: 'new' module replacement in 169
On 10/19/18 3:15 PM, Max Reitz wrote: > iotest 169 uses the 'new' module to add methods to a class. This module > no longer exists in Python 3. Instead, we can use a lambda. Best of > all, this works in 2.7 just as well. > > Signed-off-by: Max Reitz > Reviewed-by: Eduardo Habkost Reviewed-by: Cleber Rosa
Re: [Qemu-block] [PATCH v2 6/9] iotests: Explicitly inherit FDs in Python
On 10/19/18 3:15 PM, Max Reitz wrote: > Python 3.4 introduced the inheritable attribute for FDs. At the same > time, it changed the default so that all FDs are not inheritable by > default, that only inheritable FDs are inherited to subprocesses, and > only if close_fds is explicitly set to False. > > Adhere to this by setting close_fds to False when working with > subprocesses that may want to inherit FDs, and by trying to > set_inheritable() on FDs that we do want to bequeath to them. > > Signed-off-by: Max Reitz Reviewed-by: Cleber Rosa
Re: [Qemu-block] [PATCH v2 5/9] iotests: Different iterator behavior in Python 3
On 10/19/18 3:15 PM, Max Reitz wrote: > In Python 3, several functions now return iterators instead of lists. > This includes range(), items(), map(), and filter(). This means that if > we really want a list, we have to wrap those instances with list(). But > then again, the two instances where this is the case for map() and > filter(), there are shorter expressions which work without either > function. > > On the other hand, sometimes we do just want an iterator, in which case > we have sometimes used xrange() and iteritems() which no longer exist in > Python 3. Just change these calls to be range() and items(), works in > both Python 2 and 3, and is really what we want in 3 (which is what > matters). But because it is so simple to do (and to find and remove > once we completely switch to Python 3), make range() be an alias for > xrange() in the two affected tests (044 and 163). > > In one instance, we only wanted the first instance of the result of a > filter() call. Instead of using next(filter()) which would work only in > Python 3, or list(filter())[0] which would work everywhere but is a bit > weird, this instance is changed to use list comprehension with a next() > wrapped around, which works both in 2.7 and 3. > > Signed-off-by: Max Reitz Reviewed-by: Cleber Rosa
Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/9] iotests: Use // for Python integer division
On 10/19/18 3:15 PM, Max Reitz wrote: > In Python 3, / is always a floating-point division. We usually do not > want this, and as Python 2.7 understands // as well, change all integer > divisions to use that. > > Signed-off-by: Max Reitz Reviewed-by: Cleber Rosa
[Qemu-block] [PATCH v4 8/8] kvm: Potential NULL pointer dereference in kvm_arch_init_vcpu()
In kvm_arch_init_vcpu() a call to cpuid_find_entry() can return
NULL so the pointer returned should be checked before dereferencing it.
Signed-off-by: Liam Merwick
---
target/i386/kvm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index dc4047b02fc5..eb19c87a9d25 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1177,7 +1177,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
c->ecx = c->edx = 0;
c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
-c->eax = MAX(c->eax, KVM_CPUID_SIGNATURE | 0x10);
+if (c) {
+c->eax = MAX(c->eax, KVM_CPUID_SIGNATURE | 0x10);
+ }
}
cpuid_data.cpuid.nent = cpuid_i;
--
1.8.3.1
[Qemu-block] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c
The calls to find_mapping_for_cluster() may return NULL but it
isn't always checked for before dereferencing the value returned.
Additionally, add some asserts to cover cases where NULL can't
be returned but which might not be obvious at first glance.
Signed-off-by: Liam Merwick
---
block/vvfat.c | 33 -
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..19f6725054a0 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -100,6 +100,7 @@ static inline void array_free(array_t* array)
/* does not automatically grow */
static inline void* array_get(array_t* array,unsigned int index) {
assert(index < array->next);
+assert(array->pointer);
return array->pointer + index * array->item_size;
}
@@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array,
int index)
if((index + 1) * array->item_size > array->size) {
int new_size = (index + 32) * array->item_size;
array->pointer = g_realloc(array->pointer, new_size);
-if (!array->pointer)
-return -1;
+assert(array->pointer);
memset(array->pointer + array->size, 0, new_size - array->size);
array->size = new_size;
array->next = index + 1;
@@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
}
if (index >= s->mapping.next || mapping->begin > begin) {
mapping = array_insert(&(s->mapping), index, 1);
+if (mapping == NULL) {
+return NULL;
+}
mapping->path = NULL;
adjust_mapping_indices(s, index, +1);
}
@@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s,
direntry_t* direntry = array_get(&(s->directory), dir_index);
uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
+if (mapping == NULL) {
+return -1;
+}
int factor = 0x10 * s->sectors_per_cluster;
int old_cluster_count, new_cluster_count;
@@ -2494,6 +2500,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s,
parent_mapping_index %d\n", mapp
direntry = array_get(&(s->directory), first_dir_index + i);
if (is_directory(direntry) && !is_dot(direntry)) {
mapping = find_mapping_for_cluster(s, first_cluster);
+if (mapping == NULL) {
+return -1;
+}
assert(mapping->mode & MODE_DIRECTORY);
ret = commit_direntries(s, first_dir_index + i,
array_index(&(s->mapping), mapping));
@@ -2522,6 +2531,10 @@ static int commit_one_file(BDRVVVFATState* s,
assert(offset < size);
assert((offset % s->cluster_size) == 0);
+if (mapping == NULL) {
+return -1;
+}
+
for (i = s->cluster_size; i < offset; i += s->cluster_size)
c = modified_fat_get(s, c);
@@ -2668,8 +2681,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
if (commit->action == ACTION_RENAME) {
mapping_t* mapping = find_mapping_for_cluster(s,
commit->param.rename.cluster);
-char* old_path = mapping->path;
+char *old_path;
+if (mapping == NULL) {
+return -1;
+}
+old_path = mapping->path;
assert(commit->path);
mapping->path = commit->path;
if (rename(old_path, mapping->path))
@@ -2690,10 +2707,15 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
direntry_t* d = direntry + i;
if (is_file(d) || (is_directory(d) && !is_dot(d))) {
+int l;
+char *new_path;
mapping_t* m = find_mapping_for_cluster(s,
begin_of_direntry(d));
-int l = strlen(m->path);
-char* new_path = g_malloc(l + diff + 1);
+if (m == NULL) {
+return -1;
+}
+l = strlen(m->path);
+new_path = g_malloc(l + diff + 1);
assert(!strncmp(m->path, mapping->path, l2));
@@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
backing = bdrv_new_open_driver(&vvfat_write_target, NULL,
BDRV_O_ALLOW_RDWR,
&error_abort);
+assert(backing);
*(void**) backing->opaque = s;
bdrv_set_backing_hd(s->bs, backing, &error_abort);
--
1.8.3.1
[Qemu-block] [PATCH v4 7/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()
The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not
add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[].
As a result, an array dereference of metadata_ol_names[8] in
qcow2_pre_write_overlap_check() could result in a read outside of the array
bounds.
Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory')
Cc: Vladimir Sementsov-Ogievskiy
Signed-off-by: Liam Merwick
Reviewed-by: Eric Blake
Reviewed-by: Max Reitz
---
block/qcow2-refcount.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5ec..46082aeac1d6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,15 +2719,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs,
int ign, int64_t offset,
}
static const char *metadata_ol_names[] = {
-[QCOW2_OL_MAIN_HEADER_BITNR]= "qcow2_header",
-[QCOW2_OL_ACTIVE_L1_BITNR] = "active L1 table",
-[QCOW2_OL_ACTIVE_L2_BITNR] = "active L2 table",
-[QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
-[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
-[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
-[QCOW2_OL_INACTIVE_L1_BITNR]= "inactive L1 table",
-[QCOW2_OL_INACTIVE_L2_BITNR]= "inactive L2 table",
+[QCOW2_OL_MAIN_HEADER_BITNR]= "qcow2_header",
+[QCOW2_OL_ACTIVE_L1_BITNR] = "active L1 table",
+[QCOW2_OL_ACTIVE_L2_BITNR] = "active L2 table",
+[QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
+[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
+[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
+[QCOW2_OL_INACTIVE_L1_BITNR]= "inactive L1 table",
+[QCOW2_OL_INACTIVE_L2_BITNR]= "inactive L2 table",
+[QCOW2_OL_BITMAP_DIRECTORY_BITNR] = "bitmap directory",
};
+QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
/*
* First performs a check for metadata overlaps (through
--
1.8.3.1
[Qemu-block] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer
A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.
Given that dump_qlist() is static, and callers already do the right
thing, just add an assert to catch future potential bugs (plus the
added benefit of suppressing a warning from a static analysis tool
and removing this noise will help us better find real issues).
Signed-off-by: Liam Merwick
Reviewed-by: Eric Blake
---
block/qapi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/qapi.c b/block/qapi.c
index c66f949db839..e81be604217c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void
*f, int indentation,
const QListEntry *entry;
int i = 0;
+assert(list);
+
for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
--
1.8.3.1
[Qemu-block] [PATCH v4 1/8] configure: Provide option to explicitly disable AVX2
The configure script detects if the compiler has AVX2 support and
automatically sets avx2_opt="yes" which in turn defines CONFIG_AVX2_OPT.
There is no way of explicitly overriding this setting so this commit adds
two command-line options: --enable-avx2 and --disable-avx2.
The default behaviour, when no option is specified, is to maintain the
current behaviour and enable AVX2 if the compiler supports it.
Signed-off-by: Liam Merwick
Reviewed-by: Darren Kenny
Reviewed-by: Mark Kanda
---
configure | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index 9138af37f8a0..3a3e5f7004ce 100755
--- a/configure
+++ b/configure
@@ -428,7 +428,7 @@ usb_redir=""
opengl=""
opengl_dmabuf="no"
cpuid_h="no"
-avx2_opt="no"
+avx2_opt=""
zlib="yes"
capstone=""
lzo=""
@@ -1332,6 +1332,10 @@ for opt do
;;
--disable-glusterfs) glusterfs="no"
;;
+ --disable-avx2) avx2_opt="no"
+ ;;
+ --enable-avx2) avx2_opt="yes"
+ ;;
--enable-glusterfs) glusterfs="yes"
;;
--disable-virtio-blk-data-plane|--enable-virtio-blk-data-plane)
@@ -1706,6 +1710,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
libxml2 for Parallels image format
tcmalloctcmalloc support
jemallocjemalloc support
+ avx2AVX2 optimization support
replication replication support
vhost-vsock virtio sockets device support
opengl opengl support
@@ -5094,7 +5099,7 @@ fi
# There is no point enabling this if cpuid.h is not usable,
# since we won't be able to select the new routines.
-if test $cpuid_h = yes; then
+if test "$cpuid_h" = "yes" -a "$avx2_opt" != "no"; then
cat > $TMPC << EOF
#pragma GCC push_options
#pragma GCC target("avx2")
@@ -5108,6 +5113,8 @@ int main(int argc, char *argv[]) { return bar(argv[0]); }
EOF
if compile_object "" ; then
avx2_opt="yes"
+ else
+avx2_opt="no"
fi
fi
--
1.8.3.1
[Qemu-block] [PATCH v4 4/8] qemu-img: assert block_job_get() does not return NULL in img_commit()
Although the function block_job_get() can return NULL, it would be a
serious bug if it did so (because the job yields before executing anything
(if it started successfully); but otherwise, commit_active_start() would
have returned an error). However, as a precaution, before dereferencing
the 'job' pointer in img_commit() assert it is not NULL.
Signed-off-by: Liam Merwick
---
qemu-img.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b0a..457aa152296b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,7 @@ static int img_commit(int argc, char **argv)
}
job = block_job_get("commit");
+assert(job);
run_block_job(job, &local_err);
if (local_err) {
goto unref_backing;
--
1.8.3.1
[Qemu-block] [PATCH v4 0/8] off-by-one and NULL pointer accesses detected by static analysis
Below are a number of fixes to some off-by-one, read outside array bounds, and NULL pointer accesses detected by an internal Oracle static analysis tool (Parfait). https://labs.oracle.com/pls/apex/f?p=labs:49:P49_PROJECT_ID:13 I have also included a patch to add a command-line option to configure to select if AVX2 is used or not (keeping the existing behaviour by default). My motivation was avoiding an issue with the static analysis tool but NetSpectre was announced as I was working on this and I felt it may have more general uses. v1 -> v2 Based on feedback from Eric Blake: patch2: reworded commit message to clarify issue patch6: Reverted common qlist routines and added assert to qlist_dump instead patch7: Fixed incorrect logic patch8: Added QEMU_BUILD_BUG_ON to catch future іnstance at compile-time v2 -> v3 Based on feedback from Eric Blake: patch6: removed double space from commit message patch8: removed unnecessary comment and updated QEMU_BUILD_BUG_ON to use ARRAY_SIZE Added Eric's R-b to patches 6,7,8 v3 -> v4 Based on feedback from Max Reitz: patch2: Added R-b from John Snow patch3: fixed blk_get_attached_dev_id() instead of checking return value patch4: switched to assert() patch5: numerous changes based on feedback from Max patch6: updated commit message patch7: (was patch8): Added Max's R-b patch8: (new): patch fixing NULL pointer dereference in kvm_arch_init_vcpu() I also dropped the 'io: potential unnecessary check in qio_channel_command_new_spawn()' patch from v3 - it was correct but of no benefit to staic analysis checking Liam Merwick (8): configure: Provide option to explicitly disable AVX2 job: Fix off-by-one assert checks for JobSTT and JobVerbTable block: Null pointer dereference in blk_root_get_parent_desc() qemu-img: assert block_job_get() does not return NULL in img_commit() block: Fix potential Null pointer dereferences in vvfat.c block: dump_qlist() may dereference a Null pointer qcow2: Read outside array bounds in qcow2_pre_write_overlap_check() kvm: Potential NULL pointer dereference in kvm_arch_init_vcpu() block/block-backend.c | 6 +- block/qapi.c | 2 ++ block/qcow2-refcount.c | 18 ++ block/vvfat.c | 33 - configure | 11 +-- dtc| 2 +- job.c | 4 ++-- qemu-img.c | 1 + target/i386/kvm.c | 4 +++- 9 files changed, 61 insertions(+), 20 deletions(-) -- 1.8.3.1
[Qemu-block] [PATCH v4 3/8] block: Null pointer dereference in blk_root_get_parent_desc()
The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL).
Instead of just checking this case before before dereferencing,
adjust blk_get_attached_dev_id() to return the empty string if no
object path can be found (similar to the case when blk->dev is NULL
and an empty string is returned).
Signed-off-by: Liam Merwick
---
block/block-backend.c | 6 +-
dtc | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index dc0cd5772413..e628920f3cd8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -909,6 +909,7 @@ void *blk_get_attached_dev(BlockBackend *blk)
char *blk_get_attached_dev_id(BlockBackend *blk)
{
DeviceState *dev;
+char *dev_id;
assert(!blk->legacy_dev);
dev = blk->dev;
@@ -918,7 +919,10 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
} else if (dev->id) {
return g_strdup(dev->id);
}
-return object_get_canonical_path(OBJECT(dev));
+
+dev_id = object_get_canonical_path(OBJECT(dev));
+
+return dev_id ? dev_id : g_strdup("");
}
/*
diff --git a/dtc b/dtc
index 88f18909db73..e54388015af1 16
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 88f18909db731a627456f26d779445f84e449536
+Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
--
1.8.3.1
[Qemu-block] [PATCH v4 2/8] job: Fix off-by-one assert checks for JobSTT and JobVerbTable
In the assert checking the array dereference of JobVerbTable[verb]
in job_apply_verb() the check of the index, verb, allows an overrun
because an index equal to the array size is permitted.
Similarly, in the assert check of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is not flagged
either.
This is not a run-time issue as there are no callers actually
passing in the max value.
Signed-off-by: Liam Merwick
Reviewed-by: Darren Kenny
Reviewed-by: Mark Kanda
Reviewed-by: Eric Blake
Reviewed-by: John Snow
---
job.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/job.c b/job.c
index c65e01bbfa34..da8e4b7bf2f3 100644
--- a/job.c
+++ b/job.c
@@ -159,7 +159,7 @@ bool job_is_internal(Job *job)
static void job_state_transition(Job *job, JobStatus s1)
{
JobStatus s0 = job->status;
-assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
trace_job_state_transition(job, job->ret,
JobSTT[s0][s1] ? "allowed" : "disallowed",
JobStatus_str(s0), JobStatus_str(s1));
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
int job_apply_verb(Job *job, JobVerb verb, Error **errp)
{
JobStatus s0 = job->status;
-assert(verb >= 0 && verb <= JOB_VERB__MAX);
+assert(verb >= 0 && verb < JOB_VERB__MAX);
trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
JobVerbTable[verb][s0] ? "allowed" : "prohibited");
if (JobVerbTable[verb][s0]) {
--
1.8.3.1
Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/8] block: Null pointer dereference in blk_root_get_parent_desc()
On 12/10/18 15:48, Max Reitz wrote:
Hi,
On 31.08.18 20:16, Liam Merwick wrote:
The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL) so it should
be checked before dereferencing.
Signed-off-by: Liam Merwick
Reviewed-by: Darren Kenny
Reviewed-by: Mark Kanda
---
block/block-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index fa120630be83..210eee75006a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -136,7 +136,7 @@ static char *blk_root_get_parent_desc(BdrvChild *child)
}
dev_id = blk_get_attached_dev_id(blk);
-if (*dev_id) {
+if (dev_id && *dev_id) {
return dev_id;
I rather think that blk_get_attached_dev_id() needs attention first. It
returns an explicitly empty string if blk->dev is NULL. If NULL was a
valid return value, it should just return NULL there.
Besides this caller, there are two callers that pass the dev_id to
qapi_event_send_device_tray_moved(). Now in practice that allows the
string to be NULL, but there is a comment in visit_type_str() that says
one should not pass NULL.
So it's either changing blk_get_attached_dev_id() to return NULL when
there is no valid ID (instead of the empty string, and then we could
save ourselves the check "*dev_id" here and elsewhere), but then we have
to fix all callers.
Or we make it return an empty string if object_get_canonical_path()
returned NULL.
I went with the latter and now (in upcoming v4) check the return value
from object_get_canonical_path() and return an empty string if it's NULL.
Regards,
Liam
Max
} else {
/* TODO Callback into the BB owner for something more detailed */
Re: [Qemu-block] [PATCH v2 8/9] iotests: Modify imports for Python 3
On Fri, Oct 19, 2018 at 09:15:22PM +0200, Max Reitz wrote:
> There are two imports that need to be modified when running the iotests
> under Python 3: One is StringIO, which no longer exists; instead, the
> StringIO class comes from the io module, so import it from there (and
> use the BytesIO class for Python 2). The other is the ConfigParser,
> which has just been renamed to configparser.
>
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/iotests.py| 13 +
> tests/qemu-iotests/nbd-fault-injector.py | 7 +--
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7ca94e9278..ed91095505 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -29,6 +29,7 @@ import json
> import signal
> import logging
> import atexit
> +import io
>
> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'scripts'))
> import qtest
> @@ -681,15 +682,19 @@ def main(supported_fmts=[], supported_oses=['linux'],
> supported_cache_modes=[],
> verify_platform(supported_oses)
> verify_cache_mode(supported_cache_modes)
>
> -# We need to filter out the time taken from the output so that
> qemu-iotest
> -# can reliably diff the results against master output.
> -import StringIO
> if debug:
> output = sys.stdout
> verbosity = 2
> sys.argv.remove('-d')
> else:
> -output = StringIO.StringIO()
> +# We need to filter out the time taken from the output so that
> +# qemu-iotest can reliably diff the results against master output.
> +if sys.version_info.major >= 3:
> +output = io.StringIO()
> +else:
> +# StringIO() is for unicode strings, which is not what
Nit: I would change the comment to say "io.StringIO" instead of
"StringIO", to avoid confusion with StringIO.StringIO.
Not a big deal, so:
Reviewed-by: Eduardo Habkost
> +# 2.x's test runner emits.
> +output = io.BytesIO()
>
> logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>
> diff --git a/tests/qemu-iotests/nbd-fault-injector.py
> b/tests/qemu-iotests/nbd-fault-injector.py
> index d45e2e0a6a..6b2d659dee 100755
> --- a/tests/qemu-iotests/nbd-fault-injector.py
> +++ b/tests/qemu-iotests/nbd-fault-injector.py
> @@ -48,7 +48,10 @@ import sys
> import socket
> import struct
> import collections
> -import ConfigParser
> +if sys.version_info.major >= 3:
> +import configparser
> +else:
> +import ConfigParser as configparser
>
> FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
>
> @@ -225,7 +228,7 @@ def parse_config(config):
> return rules
>
> def load_rules(filename):
> -config = ConfigParser.RawConfigParser()
> +config = configparser.RawConfigParser()
> with open(filename, 'rt') as f:
> config.readfp(f, filename)
> return parse_config(config)
> --
> 2.17.1
>
--
Eduardo
Re: [Qemu-block] [PATCH v2 6/9] iotests: Explicitly inherit FDs in Python
On Fri, Oct 19, 2018 at 09:15:20PM +0200, Max Reitz wrote:
> Python 3.4 introduced the inheritable attribute for FDs. At the same
> time, it changed the default so that all FDs are not inheritable by
> default, that only inheritable FDs are inherited to subprocesses, and
> only if close_fds is explicitly set to False.
>
> Adhere to this by setting close_fds to False when working with
> subprocesses that may want to inherit FDs, and by trying to
> set_inheritable() on FDs that we do want to bequeath to them.
>
> Signed-off-by: Max Reitz
> ---
> scripts/qemu.py| 34 +-
> tests/qemu-iotests/045 | 2 +-
> tests/qemu-iotests/147 | 2 +-
> 3 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index f099ce7278..fb29b73c30 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -142,11 +142,19 @@ class QEMUMachine(object):
> if opts:
> options.append(opts)
>
> +# This did not exist before 3.4, but since then it is
> +# mandatory for our purpose
> +if hasattr(os, 'set_inheritable'):
> +os.set_inheritable(fd, True)
> +
> self._args.append('-add-fd')
> self._args.append(','.join(options))
> return self
>
> -def send_fd_scm(self, fd_file_path):
> +# Exactly one of fd and file_path must be given.
> +# (If it is file_path, the helper will open that file and pass its
> +# own fd)
> +def send_fd_scm(self, fd=None, file_path=None):
> # In iotest.py, the qmp should always use unix socket.
> assert self._qmp.is_scm_available()
> if self._socket_scm_helper is None:
> @@ -154,12 +162,27 @@ class QEMUMachine(object):
> if not os.path.exists(self._socket_scm_helper):
> raise QEMUMachineError("%s does not exist" %
> self._socket_scm_helper)
> +
> +# This did not exist before 3.4, but since then it is
> +# mandatory for our purpose
> +if hasattr(os, 'set_inheritable'):
> +os.set_inheritable(self._qmp.get_sock_fd(), True)
> +if fd is not None:
I was going to suggest keeping the existing function parameter,
and using:
isinstance(fd_file_path, int)
But your solution makes callers more explicit. This seems to be
a good thing.
Reviewed-by: Eduardo Habkost
> +os.set_inheritable(fd, True)
> +
> fd_param = ["%s" % self._socket_scm_helper,
> -"%d" % self._qmp.get_sock_fd(),
> -"%s" % fd_file_path]
> +"%d" % self._qmp.get_sock_fd()]
> +
> +if file_path is not None:
> +assert fd is None
> +fd_param.append(file_path)
> +else:
> +assert fd is not None
> +fd_param.append(str(fd))
> +
> devnull = open(os.path.devnull, 'rb')
> proc = subprocess.Popen(fd_param, stdin=devnull,
> stdout=subprocess.PIPE,
> -stderr=subprocess.STDOUT)
> +stderr=subprocess.STDOUT, close_fds=False)
> output = proc.communicate()[0]
> if output:
> LOG.debug(output)
> @@ -280,7 +303,8 @@ class QEMUMachine(object):
> stdin=devnull,
> stdout=self._qemu_log_file,
> stderr=subprocess.STDOUT,
> - shell=False)
> + shell=False,
> + close_fds=False)
> self._post_launch()
>
> def wait(self):
> diff --git a/tests/qemu-iotests/045 b/tests/qemu-iotests/045
> index 6be8fc4912..55a5d31ca8 100755
> --- a/tests/qemu-iotests/045
> +++ b/tests/qemu-iotests/045
> @@ -140,7 +140,7 @@ class TestSCMFd(iotests.QMPTestCase):
> os.remove(image0)
>
> def _send_fd_by_SCM(self):
> -ret = self.vm.send_fd_scm(image0)
> +ret = self.vm.send_fd_scm(file_path=image0)
> self.assertEqual(ret, 0, 'Failed to send fd with UNIX SCM')
>
> def test_add_fd(self):
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> index d2081df84b..05b374b7d3 100755
> --- a/tests/qemu-iotests/147
> +++ b/tests/qemu-iotests/147
> @@ -229,7 +229,7 @@ class BuiltinNBD(NBDBlockdevAddBase):
> sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> sockfd.connect(unix_socket)
>
> -result = self.vm.send_fd_scm(str(sockfd.fileno()))
> +result = self.vm.send_fd_scm(fd=sockfd.fileno())
> self.assertEqual(result, 0, 'Failed to send socket FD')
>
> result = self.vm.qmp('getfd', fdname='nbd-fifo')
> --
> 2.17.1
>
--
Eduardo
Re: [Qemu-block] [PATCH v2 4/9] iotests: Use // for Python integer division
On Fri, Oct 19, 2018 at 09:15:18PM +0200, Max Reitz wrote: > In Python 3, / is always a floating-point division. We usually do not > want this, and as Python 2.7 understands // as well, change all integer > divisions to use that. > > Signed-off-by: Max Reitz Reviewed-by: Eduardo Habkost -- Eduardo
Re: [Qemu-block] [PATCH v2 5/9] iotests: Different iterator behavior in Python 3
On Fri, Oct 19, 2018 at 09:15:19PM +0200, Max Reitz wrote:
> In Python 3, several functions now return iterators instead of lists.
> This includes range(), items(), map(), and filter(). This means that if
> we really want a list, we have to wrap those instances with list(). But
> then again, the two instances where this is the case for map() and
> filter(), there are shorter expressions which work without either
> function.
>
> On the other hand, sometimes we do just want an iterator, in which case
> we have sometimes used xrange() and iteritems() which no longer exist in
> Python 3. Just change these calls to be range() and items(), works in
> both Python 2 and 3, and is really what we want in 3 (which is what
> matters). But because it is so simple to do (and to find and remove
> once we completely switch to Python 3), make range() be an alias for
> xrange() in the two affected tests (044 and 163).
>
> In one instance, we only wanted the first instance of the result of a
> filter() call. Instead of using next(filter()) which would work only in
> Python 3, or list(filter())[0] which would work everywhere but is a bit
> weird, this instance is changed to use list comprehension with a next()
> wrapped around, which works both in 2.7 and 3.
Nit: the expression you put inside next(...) is not a list
comprehension; it's a generator expression. A list comprehension
expression would generate the full list in advance before you get
the first element.
It would be OK to rewrite the expression using an actual list
comprehension:
drive = [drive for drive in result if drive['device'] == 'drive0'][0]
But the solution you chose is OK, too.
Reviewed-by: Eduardo Habkost
>
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/044 | 16 ++--
> tests/qemu-iotests/056 | 2 +-
> tests/qemu-iotests/065 | 4 ++--
> tests/qemu-iotests/124 | 4 ++--
> tests/qemu-iotests/139 | 2 +-
> tests/qemu-iotests/163 | 11 +++
> 6 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
> index 7ef5e46fe9..9ec3dba734 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -26,6 +26,10 @@ import iotests
> from iotests import qemu_img, qemu_img_verbose, qemu_io
> import struct
> import subprocess
> +import sys
> +
> +if sys.version_info.major == 2:
> +range = xrange
>
> test_img = os.path.join(iotests.test_dir, 'test.img')
>
> @@ -52,23 +56,23 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
> # Write a refcount table
> fd.seek(off_reftable)
>
> -for i in xrange(0, h.refcount_table_clusters):
> +for i in range(0, h.refcount_table_clusters):
> sector = b''.join(struct.pack('>Q',
> off_refblock + i * 64 * 512 + j * 512)
> -for j in xrange(0, 64))
> +for j in range(0, 64))
> fd.write(sector)
>
> # Write the refcount blocks
> assert(fd.tell() == off_refblock)
> sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 *
> 256))
> -for block in xrange(0, h.refcount_table_clusters):
> +for block in range(0, h.refcount_table_clusters):
> fd.write(sector)
>
> # Write the L1 table
> assert(fd.tell() == off_l1)
> assert(off_l2 + 512 * h.l1_size == off_data)
> table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
> -for j in xrange(0, h.l1_size))
> +for j in range(0, h.l1_size))
> fd.write(table)
>
> # Write the L2 tables
> @@ -79,14 +83,14 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
> off = off_data
> while remaining > 1024 * 512:
> pytable = list((1 << 63) | off + 512 * j
> -for j in xrange(0, 1024))
> +for j in range(0, 1024))
> table = struct.pack('>1024Q', *pytable)
> fd.write(table)
> remaining = remaining - 1024 * 512
> off = off + 1024 * 512
>
> table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
> -for j in xrange(0, remaining // 512))
> +for j in range(0, remaining // 512))
> fd.write(table)
>
>
> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
> index 223292175a..3df323984d 100755
> --- a/tests/qemu-iotests/056
> +++ b/tests/qemu-iotests/056
> @@ -32,7 +32,7 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
> def img_create(img, fmt=iotests.imgfmt, size='64M', **kwargs):
> fullname = os.path.join(iotests.test_dir, '%s.%s' % (img, fmt))
> optargs = []
> -for k,v in kwargs.iteritems():
> +for k,v in kwargs.items():
> optargs = optargs + ['-o', '%s=%s' % (k,v)]
> args = [
[Qemu-block] [PATCH v2 0/9] iotests: Make them work for both Python 2 and 3
This series prepares the iotests to work with both Python 2 and 3. In some places, it adds version-specific code and decides what to do based on the version (for instance, whether to import the StringIO or the BytesIO class from 'io' for use with the test runner), but most of the time, it just makes code work for both versions in general. And when we make the switch to make Python 3 mandatory, we can simply drop the Python 2 branches. v2: - Patch 3: Commit message fix [Philippe] - Patch 4: Really convert all places that use / [Cleber] - Patch 5: - Use list comprehension expressions where that makes sense [Eduardo] - Make 'range' an alias for 'xrange' in 044 and 163 [Cleber] - Patch 6: - %s/3\.2/3.4/g [Cleber] - Use hasattr() instead of catching AttributeError [Cleber] - Call set_inheritable in send_fd_scm() for both the QMP socket FD and the FD to be passed (if any) [Eduardo] - Patch 7: Kept as-is, because it's just the minimal amount of change - Patch 8: Use io.BytesIO in Python 2 [Cleber] - Patch 9: Use json.dumps(..., sort_keys=True) instead of homegrown stringification function [Eduardo] (Also kept the order, because one vote against, one vote for, so I go for what's simplest for me. O:-)) git-backport-diff against v1: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/9:[] [--] 'iotests: Make nbd-fault-injector flush' 002/9:[] [--] 'iotests: Flush in iotests.py's QemuIoInteractive' 003/9:[] [--] 'iotests: Use Python byte strings where appropriate' 004/9:[0014] [FC] 'iotests: Use // for Python integer division' 005/9:[0013] [FC] 'iotests: Different iterator behavior in Python 3' 006/9:[0049] [FC] 'iotests: Explicitly inherit FDs in Python' 007/9:[] [--] 'iotests: 'new' module replacement in 169' 008/9:[0017] [FC] 'iotests: Modify imports for Python 3' 009/9:[1644] [FC] 'iotests: Unify log outputs between Python 2 and 3' Max Reitz (9): iotests: Make nbd-fault-injector flush iotests: Flush in iotests.py's QemuIoInteractive iotests: Use Python byte strings where appropriate iotests: Use // for Python integer division iotests: Different iterator behavior in Python 3 iotests: Explicitly inherit FDs in Python iotests: 'new' module replacement in 169 iotests: Modify imports for Python 3 iotests: Unify log outputs between Python 2 and 3 scripts/qemu.py | 34 +- scripts/qtest.py | 2 +- tests/qemu-iotests/030 | 2 +- tests/qemu-iotests/040 | 4 +- tests/qemu-iotests/041 | 4 +- tests/qemu-iotests/044 | 24 +- tests/qemu-iotests/045 | 2 +- tests/qemu-iotests/056 | 2 +- tests/qemu-iotests/065 | 4 +- tests/qemu-iotests/083.out | 9 + tests/qemu-iotests/093 | 18 +- tests/qemu-iotests/124 | 4 +- tests/qemu-iotests/136 | 2 +- tests/qemu-iotests/139 | 2 +- tests/qemu-iotests/147 | 2 +- tests/qemu-iotests/149 | 14 +- tests/qemu-iotests/151 | 12 +- tests/qemu-iotests/163 | 13 +- tests/qemu-iotests/169 | 3 +- tests/qemu-iotests/194.out | 22 +- tests/qemu-iotests/202.out | 12 +- tests/qemu-iotests/203.out | 14 +- tests/qemu-iotests/206.out | 218 +- tests/qemu-iotests/207 | 6 +- tests/qemu-iotests/207.out | 72 ++-- tests/qemu-iotests/208.out | 8 +- tests/qemu-iotests/210.out | 94 ++-- tests/qemu-iotests/211.out | 102 ++--- tests/qemu-iotests/212.out | 174 tests/qemu-iotests/213.out | 182 tests/qemu-iotests/216.out | 4 +- tests/qemu-iotests/218.out | 20 +- tests/qemu-iotests/219.out | 526 +++ tests/qemu-iotests/222.out | 24 +- tests/qemu-iotests/iotests.py| 37 +- tests/qemu-iotests/nbd-fault-injector.py | 12 +- tests/qemu-iotests/qcow2.py | 10 +- tests/qemu-iotests/qed.py| 6 +- 38 files changed, 879 insertions(+), 821 deletions(-) -- 2.17.1
[Qemu-block] [PATCH v2 9/9] iotests: Unify log outputs between Python 2 and 3
When dumping an object into the log, there are differences between
Python 2 and 3. First, unicode strings are prefixed by 'u' in Python 2
(they are no longer in 3, because unicode strings are the default
there). Second, the order of keys in dicts may differ. Third,
especially long numbers are longs in Python 2 and thus get an 'L'
suffix, which does not happen in Python 3.
We can get around all of these differences by dumping objects (lists and
dicts) in a language-independent format, namely JSON. The JSON
generator even allows emitting dicts with their keys sorted
alphabetically.
This changes the output of all tests that use these logging functions
(dict keys are ordered now, strings in dicts are now enclosed in double
quotes instead of single quotes, the 'L' suffix of large integers is
dropped, and "true" and "false" are now in lower case).
The quote change necessitates a small change to a filter used in test
207.
Suggested-by: Eduardo Habkost
Signed-off-by: Max Reitz
---
tests/qemu-iotests/194.out| 22 +-
tests/qemu-iotests/202.out| 12 +-
tests/qemu-iotests/203.out| 14 +-
tests/qemu-iotests/206.out| 218 +++---
tests/qemu-iotests/207| 2 +-
tests/qemu-iotests/207.out| 72 ++---
tests/qemu-iotests/208.out| 8 +-
tests/qemu-iotests/210.out| 94 +++---
tests/qemu-iotests/211.out| 102 +++
tests/qemu-iotests/212.out| 174 +--
tests/qemu-iotests/213.out| 182 ++--
tests/qemu-iotests/216.out| 4 +-
tests/qemu-iotests/218.out| 20 +-
tests/qemu-iotests/219.out| 526 +-
tests/qemu-iotests/222.out| 24 +-
tests/qemu-iotests/iotests.py | 10 +-
16 files changed, 744 insertions(+), 740 deletions(-)
diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out
index 50ac50da5e..71857853fb 100644
--- a/tests/qemu-iotests/194.out
+++ b/tests/qemu-iotests/194.out
@@ -1,18 +1,18 @@
Launching VMs...
Launching NBD server on destination...
-{u'return': {}}
-{u'return': {}}
+{"return": {}}
+{"return": {}}
Starting `drive-mirror` on source...
-{u'return': {}}
+{"return": {}}
Waiting for `drive-mirror` to complete...
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'device': u'mirror-job0', u'type': u'mirror', u'speed': 0, u'len':
1073741824, u'offset': 1073741824}, u'event': u'BLOCK_JOB_READY'}
+{"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824,
"speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_READY", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
Starting migration...
-{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'setup'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'active'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'completed'}, u'event': u'MIGRATION'}
+{"return": {}}
+{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"status": "active"}, "event": "MIGRATION", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
Gracefully ending the `drive-mirror` job on source...
-{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'device': u'mirror-job0', u'type': u'mirror', u'speed': 0, u'len':
1073741824, u'offset': 1073741824}, u'event': u'BLOCK_JOB_COMPLETED'}
+{"return": {}}
+{"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824,
"speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
Stopping the NBD server on destination...
-{u'return': {}}
+{"return": {}}
diff --git a/tests/qemu-iotests/202.out b/tests/qemu-iotests/202.out
index d5ea374e17..9a8619e796 100644
--- a/tests/qemu-iotests/202.out
+++ b/tests/qemu-iotests/202.out
@@ -1,11 +1,11 @@
Launching VM...
Adding IOThread...
-{u'return': {}}
+{"return": {}}
Adding blockdevs...
-{u'return': {}}
-{u'return': {}}
+{"return": {}}
+{"return": {}}
Setting iothread...
-{u'return': {}}
-{u'return': {}}
+{"return": {}}
+{"return": {}}
Creating external snapshots...
-{u'return': {}}
+{"return": {}}
diff --git a/tests/qemu-iotests/203.out b/tests/qemu-iotests/203.out
index 1a11f0975c..9d4abba8c5 100644
--- a/tests/qemu-iotests/203.out
+++ b/tests/qemu-iotests/203.out
@@ -1,11 +1,11 @@
Launching VM...
Setting IOThreads...
-{u'return': {}}
-{u'return': {}}
+{"return": {}}
+{"return": {}}
Enabling migration QMP events...
-{u'return': {}}
+{"return": {}}
Starting migration...
-{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'setup'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': '
[Qemu-block] [PATCH v2 6/9] iotests: Explicitly inherit FDs in Python
Python 3.4 introduced the inheritable attribute for FDs. At the same
time, it changed the default so that all FDs are not inheritable by
default, that only inheritable FDs are inherited to subprocesses, and
only if close_fds is explicitly set to False.
Adhere to this by setting close_fds to False when working with
subprocesses that may want to inherit FDs, and by trying to
set_inheritable() on FDs that we do want to bequeath to them.
Signed-off-by: Max Reitz
---
scripts/qemu.py| 34 +-
tests/qemu-iotests/045 | 2 +-
tests/qemu-iotests/147 | 2 +-
3 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index f099ce7278..fb29b73c30 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -142,11 +142,19 @@ class QEMUMachine(object):
if opts:
options.append(opts)
+# This did not exist before 3.4, but since then it is
+# mandatory for our purpose
+if hasattr(os, 'set_inheritable'):
+os.set_inheritable(fd, True)
+
self._args.append('-add-fd')
self._args.append(','.join(options))
return self
-def send_fd_scm(self, fd_file_path):
+# Exactly one of fd and file_path must be given.
+# (If it is file_path, the helper will open that file and pass its
+# own fd)
+def send_fd_scm(self, fd=None, file_path=None):
# In iotest.py, the qmp should always use unix socket.
assert self._qmp.is_scm_available()
if self._socket_scm_helper is None:
@@ -154,12 +162,27 @@ class QEMUMachine(object):
if not os.path.exists(self._socket_scm_helper):
raise QEMUMachineError("%s does not exist" %
self._socket_scm_helper)
+
+# This did not exist before 3.4, but since then it is
+# mandatory for our purpose
+if hasattr(os, 'set_inheritable'):
+os.set_inheritable(self._qmp.get_sock_fd(), True)
+if fd is not None:
+os.set_inheritable(fd, True)
+
fd_param = ["%s" % self._socket_scm_helper,
-"%d" % self._qmp.get_sock_fd(),
-"%s" % fd_file_path]
+"%d" % self._qmp.get_sock_fd()]
+
+if file_path is not None:
+assert fd is None
+fd_param.append(file_path)
+else:
+assert fd is not None
+fd_param.append(str(fd))
+
devnull = open(os.path.devnull, 'rb')
proc = subprocess.Popen(fd_param, stdin=devnull,
stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT)
+stderr=subprocess.STDOUT, close_fds=False)
output = proc.communicate()[0]
if output:
LOG.debug(output)
@@ -280,7 +303,8 @@ class QEMUMachine(object):
stdin=devnull,
stdout=self._qemu_log_file,
stderr=subprocess.STDOUT,
- shell=False)
+ shell=False,
+ close_fds=False)
self._post_launch()
def wait(self):
diff --git a/tests/qemu-iotests/045 b/tests/qemu-iotests/045
index 6be8fc4912..55a5d31ca8 100755
--- a/tests/qemu-iotests/045
+++ b/tests/qemu-iotests/045
@@ -140,7 +140,7 @@ class TestSCMFd(iotests.QMPTestCase):
os.remove(image0)
def _send_fd_by_SCM(self):
-ret = self.vm.send_fd_scm(image0)
+ret = self.vm.send_fd_scm(file_path=image0)
self.assertEqual(ret, 0, 'Failed to send fd with UNIX SCM')
def test_add_fd(self):
diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index d2081df84b..05b374b7d3 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -229,7 +229,7 @@ class BuiltinNBD(NBDBlockdevAddBase):
sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
sockfd.connect(unix_socket)
-result = self.vm.send_fd_scm(str(sockfd.fileno()))
+result = self.vm.send_fd_scm(fd=sockfd.fileno())
self.assertEqual(result, 0, 'Failed to send socket FD')
result = self.vm.qmp('getfd', fdname='nbd-fifo')
--
2.17.1
[Qemu-block] [PATCH v2 5/9] iotests: Different iterator behavior in Python 3
In Python 3, several functions now return iterators instead of lists.
This includes range(), items(), map(), and filter(). This means that if
we really want a list, we have to wrap those instances with list(). But
then again, the two instances where this is the case for map() and
filter(), there are shorter expressions which work without either
function.
On the other hand, sometimes we do just want an iterator, in which case
we have sometimes used xrange() and iteritems() which no longer exist in
Python 3. Just change these calls to be range() and items(), works in
both Python 2 and 3, and is really what we want in 3 (which is what
matters). But because it is so simple to do (and to find and remove
once we completely switch to Python 3), make range() be an alias for
xrange() in the two affected tests (044 and 163).
In one instance, we only wanted the first instance of the result of a
filter() call. Instead of using next(filter()) which would work only in
Python 3, or list(filter())[0] which would work everywhere but is a bit
weird, this instance is changed to use list comprehension with a next()
wrapped around, which works both in 2.7 and 3.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/044 | 16 ++--
tests/qemu-iotests/056 | 2 +-
tests/qemu-iotests/065 | 4 ++--
tests/qemu-iotests/124 | 4 ++--
tests/qemu-iotests/139 | 2 +-
tests/qemu-iotests/163 | 11 +++
6 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 7ef5e46fe9..9ec3dba734 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -26,6 +26,10 @@ import iotests
from iotests import qemu_img, qemu_img_verbose, qemu_io
import struct
import subprocess
+import sys
+
+if sys.version_info.major == 2:
+range = xrange
test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -52,23 +56,23 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
# Write a refcount table
fd.seek(off_reftable)
-for i in xrange(0, h.refcount_table_clusters):
+for i in range(0, h.refcount_table_clusters):
sector = b''.join(struct.pack('>Q',
off_refblock + i * 64 * 512 + j * 512)
-for j in xrange(0, 64))
+for j in range(0, 64))
fd.write(sector)
# Write the refcount blocks
assert(fd.tell() == off_refblock)
sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 * 256))
-for block in xrange(0, h.refcount_table_clusters):
+for block in range(0, h.refcount_table_clusters):
fd.write(sector)
# Write the L1 table
assert(fd.tell() == off_l1)
assert(off_l2 + 512 * h.l1_size == off_data)
table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
-for j in xrange(0, h.l1_size))
+for j in range(0, h.l1_size))
fd.write(table)
# Write the L2 tables
@@ -79,14 +83,14 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
off = off_data
while remaining > 1024 * 512:
pytable = list((1 << 63) | off + 512 * j
-for j in xrange(0, 1024))
+for j in range(0, 1024))
table = struct.pack('>1024Q', *pytable)
fd.write(table)
remaining = remaining - 1024 * 512
off = off + 1024 * 512
table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
-for j in xrange(0, remaining // 512))
+for j in range(0, remaining // 512))
fd.write(table)
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index 223292175a..3df323984d 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -32,7 +32,7 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
def img_create(img, fmt=iotests.imgfmt, size='64M', **kwargs):
fullname = os.path.join(iotests.test_dir, '%s.%s' % (img, fmt))
optargs = []
-for k,v in kwargs.iteritems():
+for k,v in kwargs.items():
optargs = optargs + ['-o', '%s=%s' % (k,v)]
args = ['create', '-f', fmt] + optargs + [fullname, size]
iotests.qemu_img(*args)
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 72aa9707c7..8bac383ea7 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -59,7 +59,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
:data.index('')]
for field in data:
self.assertTrue(re.match('^ {4}[^ ]', field) is not None)
-data = map(lambda line: line.strip(), data)
+data = [line.strip() for line in data]
self.assertEqual(data, self.human_compare)
class TestQMP(TestImageInfoSpecific):
@@ -80,7 +80,7 @@ class TestQMP(TestImageInfoSpecific):
def test_qmp
[Qemu-block] [PATCH v2 3/9] iotests: Use Python byte strings where appropriate
Since byte strings are no longer the default in Python 3, we have to
explicitly use them where we need to, which is mostly when working with
structures. It also means that we need to open a file in binary mode
when we want to use structures.
On the other hand, we have to accomodate for the fact that some
functions (still) work with byte strings but we want to use unicode
strings (in Python 3 at least, and it does not matter in Python 2).
This includes base64 encoding, but it is most notable when working with
the subprocess module: Either we set universal_newlines to True so that
the default streams are opened in text mode (hence this parameter is
aliased as "text" as of 3.7), or, if that is not possible, we have to
decode the output to a normal string.
Signed-off-by: Max Reitz
Reviewed-by: Eduardo Habkost
---
scripts/qtest.py | 2 +-
tests/qemu-iotests/044 | 8
tests/qemu-iotests/149 | 8 +---
tests/qemu-iotests/207 | 4 ++--
tests/qemu-iotests/iotests.py| 11 +++
tests/qemu-iotests/nbd-fault-injector.py | 4 ++--
tests/qemu-iotests/qcow2.py | 10 +-
7 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/scripts/qtest.py b/scripts/qtest.py
index df0daf26ca..adf1fe3f26 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -64,7 +64,7 @@ class QEMUQtestProtocol(object):
@param qtest_cmd: qtest command text to be sent
"""
-self._sock.sendall(qtest_cmd + "\n")
+self._sock.sendall((qtest_cmd + "\n").encode('utf-8'))
def close(self):
self._sock.close()
diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 11ea0f4d35..69e736f687 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -53,21 +53,21 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
fd.seek(off_reftable)
for i in xrange(0, h.refcount_table_clusters):
-sector = ''.join(struct.pack('>Q',
+sector = b''.join(struct.pack('>Q',
off_refblock + i * 64 * 512 + j * 512)
for j in xrange(0, 64))
fd.write(sector)
# Write the refcount blocks
assert(fd.tell() == off_refblock)
-sector = ''.join(struct.pack('>H', 1) for j in xrange(0, 64 * 256))
+sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 * 256))
for block in xrange(0, h.refcount_table_clusters):
fd.write(sector)
# Write the L1 table
assert(fd.tell() == off_l1)
assert(off_l2 + 512 * h.l1_size == off_data)
-table = ''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
+table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
for j in xrange(0, h.l1_size))
fd.write(table)
@@ -85,7 +85,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
remaining = remaining - 1024 * 512
off = off + 1024 * 512
-table = ''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
+table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
for j in xrange(0, remaining / 512))
fd.write(table)
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 9e0cad76f9..1225334cb8 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -79,7 +79,7 @@ class LUKSConfig(object):
def first_password_base64(self):
(pw, slot) = self.first_password()
-return base64.b64encode(pw)
+return base64.b64encode(pw.encode('ascii')).decode('ascii')
def active_slots(self):
slots = []
@@ -98,7 +98,8 @@ def verify_passwordless_sudo():
proc = subprocess.Popen(args,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT)
+stderr=subprocess.STDOUT,
+universal_newlines=True)
msg = proc.communicate()[0]
@@ -116,7 +117,8 @@ def cryptsetup(args, password=None):
proc = subprocess.Popen(fullargs,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT)
+stderr=subprocess.STDOUT,
+universal_newlines=True)
msg = proc.communicate(password)[0]
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 444ae233ae..2d86a3da37 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -109,7 +109,7 @@ with iotests.FilePath('t.img') as disk_path, \
md5_key = subprocess.check_output(
'ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" | ' +
'cut -d" " -f3 | base64 -d | md5sum -b | cut -d" "
[Qemu-block] [PATCH v2 8/9] iotests: Modify imports for Python 3
There are two imports that need to be modified when running the iotests
under Python 3: One is StringIO, which no longer exists; instead, the
StringIO class comes from the io module, so import it from there (and
use the BytesIO class for Python 2). The other is the ConfigParser,
which has just been renamed to configparser.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/iotests.py| 13 +
tests/qemu-iotests/nbd-fault-injector.py | 7 +--
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7ca94e9278..ed91095505 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -29,6 +29,7 @@ import json
import signal
import logging
import atexit
+import io
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
import qtest
@@ -681,15 +682,19 @@ def main(supported_fmts=[], supported_oses=['linux'],
supported_cache_modes=[],
verify_platform(supported_oses)
verify_cache_mode(supported_cache_modes)
-# We need to filter out the time taken from the output so that qemu-iotest
-# can reliably diff the results against master output.
-import StringIO
if debug:
output = sys.stdout
verbosity = 2
sys.argv.remove('-d')
else:
-output = StringIO.StringIO()
+# We need to filter out the time taken from the output so that
+# qemu-iotest can reliably diff the results against master output.
+if sys.version_info.major >= 3:
+output = io.StringIO()
+else:
+# StringIO() is for unicode strings, which is not what
+# 2.x's test runner emits.
+output = io.BytesIO()
logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
diff --git a/tests/qemu-iotests/nbd-fault-injector.py
b/tests/qemu-iotests/nbd-fault-injector.py
index d45e2e0a6a..6b2d659dee 100755
--- a/tests/qemu-iotests/nbd-fault-injector.py
+++ b/tests/qemu-iotests/nbd-fault-injector.py
@@ -48,7 +48,10 @@ import sys
import socket
import struct
import collections
-import ConfigParser
+if sys.version_info.major >= 3:
+import configparser
+else:
+import ConfigParser as configparser
FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
@@ -225,7 +228,7 @@ def parse_config(config):
return rules
def load_rules(filename):
-config = ConfigParser.RawConfigParser()
+config = configparser.RawConfigParser()
with open(filename, 'rt') as f:
config.readfp(f, filename)
return parse_config(config)
--
2.17.1
[Qemu-block] [PATCH v2 7/9] iotests: 'new' module replacement in 169
iotest 169 uses the 'new' module to add methods to a class. This module
no longer exists in Python 3. Instead, we can use a lambda. Best of
all, this works in 2.7 just as well.
Signed-off-by: Max Reitz
Reviewed-by: Eduardo Habkost
---
tests/qemu-iotests/169 | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index f243db9955..e5614b159d 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -23,7 +23,6 @@ import iotests
import time
import itertools
import operator
-import new
from iotests import qemu_img
@@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
def inject_test_case(klass, name, method, *args, **kwargs):
mc = operator.methodcaller(method, *args, **kwargs)
-setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
+setattr(klass, 'test_' + name, lambda self: mc(self))
for cmb in list(itertools.product((True, False), repeat=4)):
name = ('_' if cmb[0] else '_not_') + 'persistent_'
--
2.17.1
[Qemu-block] [PATCH v2 4/9] iotests: Use // for Python integer division
In Python 3, / is always a floating-point division. We usually do not
want this, and as Python 2.7 understands // as well, change all integer
divisions to use that.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/030| 2 +-
tests/qemu-iotests/040| 4 ++--
tests/qemu-iotests/041| 4 ++--
tests/qemu-iotests/044| 2 +-
tests/qemu-iotests/093| 18 +-
tests/qemu-iotests/136| 2 +-
tests/qemu-iotests/149| 6 +++---
tests/qemu-iotests/151| 12 ++--
tests/qemu-iotests/163| 2 +-
tests/qemu-iotests/iotests.py | 2 +-
tests/qemu-iotests/qed.py | 6 +++---
11 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1dbc2ddc49..276e06b5ba 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -521,7 +521,7 @@ new_state = "2"
state = "2"
event = "%s"
new_state = "1"
-''' % (event, errno, self.STREAM_BUFFER_SIZE / 512, event, event))
+''' % (event, errno, self.STREAM_BUFFER_SIZE // 512, event, event))
file.close()
class TestEIO(TestErrors):
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 1cb1ceeb33..b81133a474 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -195,7 +195,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
- base=backing_img, speed=(self.image_len / 4))
+ base=backing_img, speed=(self.image_len // 4))
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('device_del', id='scsi0')
self.assert_qmp(result, 'return', {})
@@ -225,7 +225,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
- base=backing_img, speed=(self.image_len / 4))
+ base=backing_img, speed=(self.image_len // 4))
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('query-block')
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 9336ab6ff5..3615011d98 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -404,7 +404,7 @@ new_state = "2"
state = "2"
event = "%s"
new_state = "1"
-''' % (event, errno, self.MIRROR_GRANULARITY / 512, event, event))
+''' % (event, errno, self.MIRROR_GRANULARITY // 512, event, event))
file.close()
def setUp(self):
@@ -569,7 +569,7 @@ new_state = "2"
state = "2"
event = "%s"
new_state = "1"
-''' % (event, errno, self.MIRROR_GRANULARITY / 512, event, event))
+''' % (event, errno, self.MIRROR_GRANULARITY // 512, event, event))
file.close()
def setUp(self):
diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 69e736f687..7ef5e46fe9 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -86,7 +86,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
off = off + 1024 * 512
table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
-for j in xrange(0, remaining / 512))
+for j in xrange(0, remaining // 512))
fd.write(table)
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index 9d1971a56c..d88fbc182e 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -69,18 +69,18 @@ class ThrottleTestCase(iotests.QMPTestCase):
# in. The throttled requests won't be executed until we
# advance the virtual clock.
rq_size = 512
-rd_nr = max(params['bps'] / rq_size / 2,
-params['bps_rd'] / rq_size,
-params['iops'] / 2,
+rd_nr = max(params['bps'] // rq_size // 2,
+params['bps_rd'] // rq_size,
+params['iops'] // 2,
params['iops_rd'])
rd_nr *= seconds * 2
-rd_nr /= ndrives
-wr_nr = max(params['bps'] / rq_size / 2,
-params['bps_wr'] / rq_size,
-params['iops'] / 2,
+rd_nr //= ndrives
+wr_nr = max(params['bps'] // rq_size // 2,
+params['bps_wr'] // rq_size,
+params['iops'] // 2,
params['iops_wr'])
wr_nr *= seconds * 2
-wr_nr /= ndrives
+wr_nr //= ndrives
# Send I/O requests to all drives
for i in range(rd_nr):
@@ -196,7 +196,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
self.configure_throttle(ndrives, settings)
# Wait for the bucket to empty so we can do bursts
-wait_ns = nsec_per_sec * burst_length * burst_rate / rate
+wait_ns = nsec_per_sec * burst_length * burst_rate // rate
self.vm.qtest("clock_step %d" % wai
[Qemu-block] [PATCH v2 1/9] iotests: Make nbd-fault-injector flush
When closing a connection, make the nbd-fault-injector flush the socket.
Without this, the output is a bit unreliable with Python 3.
Signed-off-by: Max Reitz
Reviewed-by: Eduardo Habkost
Reviewed-by: Cleber Rosa
Reviewed-by: Eric Blake
---
tests/qemu-iotests/083.out | 9 +
tests/qemu-iotests/nbd-fault-injector.py | 1 +
2 files changed, 10 insertions(+)
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index be6079d27e..f9af8bb691 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -41,6 +41,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
=== Check disconnect after neg2 ===
+Unable to read from socket: Connection reset by peer
Connection closed
read failed: Input/output error
@@ -54,6 +55,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
=== Check disconnect before request ===
+Unable to read from socket: Connection reset by peer
Connection closed
read failed: Input/output error
@@ -116,6 +118,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/
=== Check disconnect after neg-classic ===
+Unable to read from socket: Connection reset by peer
Connection closed
read failed: Input/output error
@@ -161,6 +164,8 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
=== Check disconnect after neg2 ===
+Unable to read from socket: Connection reset by peer
+Connection closed
read failed: Input/output error
=== Check disconnect 8 neg2 ===
@@ -173,6 +178,8 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
=== Check disconnect before request ===
+Unable to read from socket: Connection reset by peer
+Connection closed
read failed: Input/output error
=== Check disconnect after request ===
@@ -234,6 +241,8 @@ can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
=== Check disconnect after neg-classic ===
+Unable to read from socket: Connection reset by peer
+Connection closed
read failed: Input/output error
*** done
diff --git a/tests/qemu-iotests/nbd-fault-injector.py
b/tests/qemu-iotests/nbd-fault-injector.py
index f9193c0fae..439a090eb6 100755
--- a/tests/qemu-iotests/nbd-fault-injector.py
+++ b/tests/qemu-iotests/nbd-fault-injector.py
@@ -112,6 +112,7 @@ class FaultInjectionSocket(object):
if rule.match(event, io):
if rule.when == 0 or bufsize is None:
print('Closing connection on rule match %s' % rule.name)
+self.sock.flush()
sys.exit(0)
if rule.when != -1:
return rule.when
--
2.17.1
[Qemu-block] [PATCH v2 2/9] iotests: Flush in iotests.py's QemuIoInteractive
After issuing a command, flush the pipe. This does not change anything in Python 2, but it makes a difference in Python 3. Signed-off-by: Max Reitz Reviewed-by: Eduardo Habkost Reviewed-by: Cleber Rosa --- tests/qemu-iotests/iotests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 4e67fbbe96..10f2d17419 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -178,6 +178,7 @@ class QemuIoInteractive: cmd = cmd.strip() assert cmd != 'q' and cmd != 'quit' self._p.stdin.write(cmd + '\n') +self._p.stdin.flush() return self._read_output() -- 2.17.1
Re: [Qemu-block] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path
On 10/19/18 11:30 AM, Kevin Wolf wrote:
Commit e2b8247a322 introduced an error path in qemu_rbd_open() after
calling rbd_open(), but neglected to close the image again in this error
path. The error path should contain everything that the regular close
function qemu_rbd_close() contains.
This adds the missing rbd_close() call.
Signed-off-by: Kevin Wolf
---
block/rbd.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Eric Blake
diff --git a/block/rbd.c b/block/rbd.c
index 014c68d629..27c9a1e81c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -787,6 +787,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
"automatically marking the image read-only.");
r = bdrv_set_read_only(bs, true, &local_err);
if (r < 0) {
+rbd_close(s->image);
error_propagate(errp, local_err);
goto failed_open;
}
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-block] [PATCH v2 0/5] Various option help readability improvement suggestions
I noticed that with the (more or less) recent series from Marc-André the
output of qemu-img amend -f qcow2 -o help changed to this:
$ ./qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
qcow2-create-opts.backing_file=str - File name of a base image
qcow2-create-opts.backing_fmt=str - Image format of the base image
qcow2-create-opts.cluster_size=size - qcow2 cluster size
qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
[...]
The types are a nice addition, but I didn't like having the list name
printed in every single line (in fact, the list name does not make any
sense here at all, because there already is a caption which reads
"Creation options for 'qcow2'"), and I did not like the use of '=' for
types.
In general, I don't like the robot-y appearance, which is even worse in
things like -device virtio-blk,help, which gives you this (among
other lines):
> virtio-blk-pci.iothread=link
Sadly, there isn't much we can do about the "link", so this
series doesn't improve on that point.
What this series does do, however, is it changes these lists not to
print the list name on every single line, but only as a caption (and for
option lists, this caption is option, because the caller may want to
print a custom caption that is more expressive -- as is the case for
qemu-img amend -o help).
Consequentially, all list items are indented by two spaces to make clear
they belong to the caption. I can already see that some people might
disagree on having this indentation, but I like it, so I have it in this
series.
Furthermore, types are now enclosed by angle brackets, and the alignment
we originally had for descriptions is restored (although now after 24
instead of 16 characters, because every option name is now accompanied
by indentation and a type).
Thus, after this series, the amend output looks like this:
$ ./qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
backing_file= - File name of a base image
backing_fmt= - Image format of the base image
cluster_size=- qcow2 cluster size
compat= - Compatibility level (0.10 or 1.1)
[...]
virtio-blk's list presents itself like so:
$ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
virtio-blk-pci options:
iothread=>
request-merging= - on/off
secs=
[...]
And now we even print something when there are no options:
$ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
There are no options for can-bus.
(Before this series, there just is no output.)
As a side effect, patch 1 fixes iotest 082.
v2:
- Patch 1:
- Abandon the "$name: $type" formatting in favor of "$name=<$type>"
[Marc-André, at least the "abandon" part]
- Restore description alignment [Kevin]
- Do the alignment when generating each line's GString instead of when
printing them. This results in less lines modified and allows the
compiler to optimize the printf("%s\n", x) to puts(x).
- Patch 3:
- Same changes as above, with the addition of also separating the
description with " - " instead of enclosing it in parentheses (to
match the other places)
- Also, we never did align the descriptions here, so this is not
"restore" but "introduce description alignment".
- Patch 4:
- Same as patch 1, but again with the catch of s/restore/introduce/.
git-backport-diff against v1:
Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/5:[0963] [FC] 'option: Make option help nicer to read'
002/5:[] [--] 'chardev: Indent list of chardevs'
003/5:[0008] [FC] 'qdev-monitor: Make device options help nicer'
004/5:[0007] [FC] 'object: Make option help nicer to read'
005/5:[] [--] 'fw_cfg: Drop newline in @file description'
Max Reitz (5):
option: Make option help nicer to read
chardev: Indent list of chardevs
qdev-monitor: Make device options help nicer
object: Make option help nicer to read
fw_cfg: Drop newline in @file description
include/qemu/option.h | 2 +-
chardev/char.c | 2 +-
qdev-monitor.c | 13 +-
qemu-img.c | 4 +-
util/qemu-option.c | 32 +-
vl.c | 15 +-
tests/qemu-iotests/082.out | 956 ++---
7 files changed, 530 insertions(+), 494 deletions(-)
--
2.17.1
[Qemu-block] [PATCH v2 1/5] option: Make option help nicer to read
This adds some whitespace into the option help (including indentation)
and puts angle brackets around the type names. Furthermore, the list
name is no longer printed as part of every line, but only once in
advance, and only if the caller did not print a caption already.
This patch also restores the description alignment we had before commit
9cbef9d68ee1d8d0, just at 24 instead of 16 characters like we used to.
This increase is because now we have the type and two spaces of
indentation before the description, and with a usual type name length of
three chracters, this sums up to eight additional characters -- which
means that we now need 24 characters to get the same amount of padding
for most options. Also, 24 is a third of 80, which makes it kind of a
round number in terminal terms.
Finally, this patch amends the reference output of iotest 082 to match
the changes (and thus makes it pass again).
Signed-off-by: Max Reitz
---
include/qemu/option.h | 2 +-
qemu-img.c | 4 +-
util/qemu-option.c | 32 +-
tests/qemu-iotests/082.out | 956 ++---
4 files changed, 507 insertions(+), 487 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 3dfb4493cc..844587cab3 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -132,7 +132,7 @@ typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts
*opts, Error **errp);
int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
void *opaque, Error **errp);
void qemu_opts_print(QemuOpts *opts, const char *sep);
-void qemu_opts_print_help(QemuOptsList *list);
+void qemu_opts_print_help(QemuOptsList *list, bool print_caption);
void qemu_opts_free(QemuOptsList *list);
QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..4c96db7ba4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -269,7 +269,7 @@ static int print_block_option_help(const char *filename,
const char *fmt)
}
printf("Supported options:\n");
-qemu_opts_print_help(create_opts);
+qemu_opts_print_help(create_opts, false);
qemu_opts_free(create_opts);
return 0;
}
@@ -3773,7 +3773,7 @@ static int print_amend_option_help(const char *format)
assert(drv->create_opts);
printf("Creation options for '%s':\n", format);
-qemu_opts_print_help(drv->create_opts);
+qemu_opts_print_help(drv->create_opts, false);
printf("\nNote that not all of these options may be amendable.\n");
return 0;
}
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9a5f263294..de42e2a406 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -224,7 +224,14 @@ static const char *opt_type_to_string(enum QemuOptType
type)
g_assert_not_reached();
}
-void qemu_opts_print_help(QemuOptsList *list)
+/**
+ * Print the list of options available in the given list. If
+ * @print_caption is true, a caption (including the list name, if it
+ * exists) is printed. The options itself will be indented, so
+ * @print_caption should only be set to false if the caller prints its
+ * own custom caption (so that the indentation makes sense).
+ */
+void qemu_opts_print_help(QemuOptsList *list, bool print_caption)
{
QemuOptDesc *desc;
int i;
@@ -234,12 +241,12 @@ void qemu_opts_print_help(QemuOptsList *list)
desc = list->desc;
while (desc && desc->name) {
GString *str = g_string_new(NULL);
-if (list->name) {
-g_string_append_printf(str, "%s.", list->name);
-}
-g_string_append_printf(str, "%s=%s", desc->name,
+g_string_append_printf(str, " %s=<%s>", desc->name,
opt_type_to_string(desc->type));
if (desc->help) {
+if (str->len < 24) {
+g_string_append_printf(str, "%*s", 24 - (int)str->len, "");
+}
g_string_append_printf(str, " - %s", desc->help);
}
g_ptr_array_add(array, g_string_free(str, false));
@@ -247,6 +254,19 @@ void qemu_opts_print_help(QemuOptsList *list)
}
g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+if (print_caption && array->len > 0) {
+if (list->name) {
+printf("%s options:\n", list->name);
+} else {
+printf("Options:\n");
+}
+} else if (array->len == 0) {
+if (list->name) {
+printf("There are no options for %s.\n", list->name);
+} else {
+printf("No options available.\n");
+}
+}
for (i = 0; i < array->len; i++) {
printf("%s\n", (char *)array->pdata[i]);
}
@@ -930,7 +950,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const
char *params,
opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
if (err) {
if (invalidp && has_help_option(params)) {
-qemu_opts_print_help(list);
+qemu
[Qemu-block] [PATCH v2 2/5] chardev: Indent list of chardevs
Following the example of qemu_opts_print_help(), indent all entries in
the list of character devices.
Signed-off-by: Max Reitz
---
chardev/char.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/chardev/char.c b/chardev/char.c
index e115166995..10d44aaefc 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -569,7 +569,7 @@ help_string_append(const char *name, void *opaque)
{
GString *str = opaque;
-g_string_append_printf(str, "\n%s", name);
+g_string_append_printf(str, "\n %s", name);
}
static const char *chardev_alias_translate(const char *name)
--
2.17.1
[Qemu-block] [PATCH v2 5/5] fw_cfg: Drop newline in @file description
There is no good reason why there should be a newline in this
description, so remove it.
Signed-off-by: Max Reitz
Reviewed-by: Philippe Mathieu-Daudé
---
vl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index ecddbb6b8e..6604648570 100644
--- a/vl.c
+++ b/vl.c
@@ -529,7 +529,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
}, {
.name = "file",
.type = QEMU_OPT_STRING,
-.help = "Sets the name of the file from which\n"
+.help = "Sets the name of the file from which "
"the fw_cfg blob will be loaded",
}, {
.name = "string",
--
2.17.1
[Qemu-block] [PATCH v2 4/5] object: Make option help nicer to read
Just like in qemu_opts_print_help(), print the object name as a caption
instead of on every single line, indent all options, add angle brackets
around types, and align the descriptions after 24 characters.
Also, indent every object name in the list of available objects.
Signed-off-by: Max Reitz
---
vl.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/vl.c b/vl.c
index ac3ed17de4..ecddbb6b8e 100644
--- a/vl.c
+++ b/vl.c
@@ -2707,7 +2707,7 @@ static bool object_create_initial(const char *type,
QemuOpts *opts)
list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
for (l = list; l != NULL; l = l->next) {
ObjectClass *oc = OBJECT_CLASS(l->data);
-printf("%s\n", object_class_get_name(oc));
+printf(" %s\n", object_class_get_name(oc));
}
g_slist_free(list);
exit(0);
@@ -2729,14 +2729,21 @@ static bool object_create_initial(const char *type,
QemuOpts *opts)
}
str = g_string_new(NULL);
-g_string_append_printf(str, "%s.%s=%s", type,
- prop->name, prop->type);
+g_string_append_printf(str, " %s=<%s>", prop->name, prop->type);
if (prop->description) {
+if (str->len < 24) {
+g_string_append_printf(str, "%*s", 24 - (int)str->len, "");
+}
g_string_append_printf(str, " - %s", prop->description);
}
g_ptr_array_add(array, g_string_free(str, false));
}
g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+if (array->len > 0) {
+printf("%s options:\n", type);
+} else {
+printf("There are no options for %s.\n", type);
+}
for (i = 0; i < array->len; i++) {
printf("%s\n", (char *)array->pdata[i]);
}
--
2.17.1
[Qemu-block] [PATCH v2 3/5] qdev-monitor: Make device options help nicer
Just like in qemu_opts_print_help(), print the device name as a caption
instead of on every single line, indent all options, add angle brackets
around types, and align the descriptions after 24 characters. Also,
separate the descriptions with " - " instead of putting them in
parentheses, because that is what we do everywhere else. This does look
a bit funny here because basically all bits have the description
"on/off", but funny does not mean it is less readable.
Signed-off-by: Max Reitz
---
qdev-monitor.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 802c18a74e..07147c63bf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -285,10 +285,19 @@ int qdev_device_help(QemuOpts *opts)
goto error;
}
+if (prop_list) {
+out_printf("%s options:\n", driver);
+} else {
+out_printf("There are no options for %s.\n", driver);
+}
for (prop = prop_list; prop; prop = prop->next) {
-out_printf("%s.%s=%s", driver, prop->value->name, prop->value->type);
+int len;
+out_printf(" %s=<%s>%n", prop->value->name, prop->value->type, &len);
if (prop->value->has_description) {
-out_printf(" (%s)\n", prop->value->description);
+if (len < 24) {
+out_printf("%*s", 24 - len, "");
+}
+out_printf(" - %s\n", prop->value->description);
} else {
out_printf("\n");
}
--
2.17.1
[Qemu-block] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path
Commit e2b8247a322 introduced an error path in qemu_rbd_open() after
calling rbd_open(), but neglected to close the image again in this error
path. The error path should contain everything that the regular close
function qemu_rbd_close() contains.
This adds the missing rbd_close() call.
Signed-off-by: Kevin Wolf
---
block/rbd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/rbd.c b/block/rbd.c
index 014c68d629..27c9a1e81c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -787,6 +787,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
"automatically marking the image read-only.");
r = bdrv_set_read_only(bs, true, &local_err);
if (r < 0) {
+rbd_close(s->image);
error_propagate(errp, local_err);
goto failed_open;
}
--
2.19.1
[Qemu-block] [PATCH v4 09/11] iscsi: Support auto-read-only option
If read-only=off, but auto-read-only=on is given, open the volume
read-write if we have the permissions, but instead of erroring out for
read-only volumes, just degrade to read-only.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
block/iscsi.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index bb69faf34a..130ae26f5f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1878,9 +1878,11 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
/* Check the write protect flag of the LUN if we want to write */
if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
iscsilun->write_protected) {
-error_setg(errp, "Cannot open a write protected LUN as read-write");
-ret = -EACCES;
-goto out;
+ret = bdrv_apply_auto_read_only(bs, "LUN is write protected", errp);
+if (ret < 0) {
+goto out;
+}
+flags &= ~BDRV_O_RDWR;
}
iscsi_readcapacity_sync(iscsilun, &local_err);
--
2.19.1
[Qemu-block] [PATCH v4 08/11] gluster: Support auto-read-only option
If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.
Signed-off-by: Kevin Wolf
Reviewed-by: Niels de Vos
---
block/gluster.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..5e300c96c8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -849,8 +849,16 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict
*options,
qemu_gluster_parse_flags(bdrv_flags, &open_flags);
s->fd = glfs_open(s->glfs, gconf->path, open_flags);
-if (!s->fd) {
-ret = -errno;
+ret = s->fd ? 0 : -errno;
+
+if (ret == -EACCES || ret == -EROFS) {
+/* Try to degrade to read-only, but if it doesn't work, still use the
+ * normal error message. */
+if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
+s->fd = glfs_open(s->glfs, gconf->path, open_flags);
+ret = s->fd ? 0 : -errno;
+}
}
s->supports_seek_data = qemu_gluster_test_seek(s->fd);
--
2.19.1
[Qemu-block] [PATCH v4 05/11] nbd: Support auto-read-only option
If read-only=off, but auto-read-only=on is given, open a read-write NBD
connection if the server provides a read-write export, but instead of
erroring out for read-only exports, just degrade to read-only.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
block/nbd-client.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9686ecbd5e..76e9ca3abe 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -992,11 +992,11 @@ int nbd_client_init(BlockDriverState *bs,
logout("Failed to negotiate with the NBD server\n");
return ret;
}
-if (client->info.flags & NBD_FLAG_READ_ONLY &&
-!bdrv_is_read_only(bs)) {
-error_setg(errp,
- "request for write access conflicts with read-only export");
-return -EACCES;
+if (client->info.flags & NBD_FLAG_READ_ONLY) {
+ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
+if (ret < 0) {
+return ret;
+}
}
if (client->info.flags & NBD_FLAG_SEND_FUA) {
bs->supported_write_flags = BDRV_REQ_FUA;
--
2.19.1
[Qemu-block] [PATCH v4 04/11] block: Require auto-read-only for existing fallbacks
Some block drivers have traditionally changed their node to read-only
mode without asking the user. This behaviour has been marked deprecated
since 2.11, expecting users to provide an explicit read-only=on option.
Now that we have auto-read-only=on, enable these drivers to make use of
the option.
This is the only use of bdrv_set_read_only(), so we can make it a bit
more specific and turn it into a bdrv_apply_auto_read_only() that is
more convenient for drivers to use.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
include/block/block.h | 3 ++-
block.c | 42 +++---
block/bochs.c | 17 ++---
block/cloop.c | 16 +---
block/dmg.c | 16 +---
block/rbd.c | 15 ---
block/vvfat.c | 10 ++
7 files changed, 51 insertions(+), 68 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 580b3716c3..7f5453b45b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,7 +438,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
BlockDriverState *base,
bool bdrv_is_read_only(BlockDriverState *bs);
int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
bool ignore_allow_rdw, Error **errp);
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+ Error **errp);
bool bdrv_is_writable(BlockDriverState *bs);
bool bdrv_is_sg(BlockDriverState *bs);
bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ed73522cc6..a97e324f39 100644
--- a/block.c
+++ b/block.c
@@ -266,29 +266,41 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool
read_only,
return 0;
}
-/* TODO Remove (deprecated since 2.11)
- * Block drivers are not supposed to automatically change bs->read_only.
- * Instead, they should just check whether they can provide what the user
- * explicitly requested and error out if read-write is requested, but they can
- * only provide read-only access. */
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+/*
+ * Called by a driver that can only provide a read-only image.
+ *
+ * Returns 0 if the node is already read-only or it could switch the node to
+ * read-only because BDRV_O_AUTO_RDONLY is set.
+ *
+ * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set
+ * or bdrv_can_set_read_only() forbids making the node read-only. If @errmsg
+ * is not NULL, it is used as the error message for the Error object.
+ */
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+ Error **errp)
{
int ret = 0;
-ret = bdrv_can_set_read_only(bs, read_only, false, errp);
-if (ret < 0) {
-return ret;
+if (!(bs->open_flags & BDRV_O_RDWR)) {
+return 0;
+}
+if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
+goto fail;
}
-bs->read_only = read_only;
-
-if (read_only) {
-bs->open_flags &= ~BDRV_O_RDWR;
-} else {
-bs->open_flags |= BDRV_O_RDWR;
+ret = bdrv_can_set_read_only(bs, true, false, NULL);
+if (ret < 0) {
+goto fail;
}
+bs->read_only = true;
+bs->open_flags &= ~BDRV_O_RDWR;
+
return 0;
+
+fail:
+error_setg(errp, "%s", errmsg ?: "Image is read-only");
+return -EACCES;
}
void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..22e7d44211 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,23 +105,18 @@ static int bochs_open(BlockDriverState *bs, QDict
*options, int flags,
struct bochs_header bochs;
int ret;
+/* No write support yet */
+ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+if (ret < 0) {
+return ret;
+}
+
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
false, errp);
if (!bs->file) {
return -EINVAL;
}
-if (!bdrv_is_read_only(bs)) {
-error_report("Opening bochs images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
-ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
-if (ret < 0) {
-return ret;
-}
-}
-
ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
if (ret < 0) {
return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..df2b85f723 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,23 +67,17 @@ static int cloop_open(BlockDriverState *bs, QDict *options,
int flags,
uint32_t offsets_size, max_compressed_block_size = 1, i;
int re
[Qemu-block] [PATCH v4 02/11] block: Add auto-read-only option
If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.
Backing files should work on read-only storage, but at the same time, a
block job like commit should be able to reopen them read-write if they
are on read-write storage. However, without option inheritance, reopen
only changes the read-only option for the root node (typically the
format layer), but not the protocol layer, so reopening fails (the
format layer wants to get write permissions, but the protocol layer is
still read-only).
A simple workaround for the problem in the management tool would be to
open the protocol layer always read-write and to make only the format
layer read-only for backing files. However, sometimes the file is
actually stored on read-only storage and we don't know whether the image
can be opened read-write (for example, for NBD it depends on the server
we're trying to connect to). This adds an option that makes QEMU try to
open the image read-write, but allows it to degrade to a read-only mode
without returning an error.
The documentation for this option is consciously phrased in a way that
allows QEMU to switch to a better model eventually: Instead of trying
when the image is first opened, making the read-only flag dynamic and
changing it automatically whenever the first BLK_PERM_WRITE user is
attached or the last one is detached would be much more useful
behaviour.
Unfortunately, this more useful behaviour is also a lot harder to
implement, and libvirt needs a solution now before it can switch to
-blockdev, so let's start with this easier approach for now.
Instead of adding a new auto-read-only option, turning the existing
read-only into an enum (with a bool alternate for compatibility) was
considered, but it complicated the implementation to the point that it
didn't seem to be worth it.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
qapi/block-core.json | 7 +++
include/block/block.h | 2 ++
block.c | 17 +
block/vvfat.c | 1 +
4 files changed, 27 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cfb37f8c1d..2ac4e69e68 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3651,6 +3651,12 @@
# either generally or in certain configurations. In this case,
# the default value does not work and the option must be
# specified explicitly.
+# @auto-read-only: if true and @read-only is false, QEMU may automatically
+# decide not to open the image read-write as requested, but
+# fall back to read-only instead (and switch between the modes
+# later), e.g. depending on whether the image file is writable
+# or whether a writing user is attached to the node
+# (default: false, since 3.1)
# @detect-zeroes: detect and optimize zero writes (Since 2.1)
# (default: off)
# @force-share: force share all permission on added nodes.
@@ -3666,6 +3672,7 @@
'*discard': 'BlockdevDiscardOptions',
'*cache': 'BlockdevCacheOptions',
'*read-only': 'bool',
+'*auto-read-only': 'bool',
'*force-share': 'bool',
'*detect-zeroes': 'BlockdevDetectZeroesOptions' },
'discriminator': 'driver',
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..580b3716c3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -115,6 +115,7 @@ typedef struct HDGeometry {
select an appropriate protocol driver,
ignoring the format layer */
#define BDRV_O_NO_IO 0x1 /* don't initialize for I/O */
+#define BDRV_O_AUTO_RDONLY 0x2 /* degrade to read-only if opening
read-write fails */
#define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
@@ -125,6 +126,7 @@ typedef struct HDGeometry {
#define BDRV_OPT_CACHE_DIRECT "cache.direct"
#define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
#define BDRV_OPT_READ_ONLY "read-only"
+#define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
#define BDRV_OPT_DISCARD"discard"
#define BDRV_OPT_FORCE_SHARE"force-share"
diff --git a/block.c b/block.c
index d7bd6d29b4..ed73522cc6 100644
--- a/block.c
+++ b/block.c
@@ -930,6 +930,7 @@ static void bdrv_inherited_options(int *child_flags, QDict
*child_options,
/* Inherit the read-only option from the parent if it's not set */
qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_AUTO_READ_ONLY);
/* Our block drivers take care to send flushes and respect unmap policy,
* so we can default to enable both on lower layers regardless of the
@@ -1053,6 +1054,7 @@ static void bdrv_backing_options(int *child
[Qemu-block] [PATCH v4 06/11] file-posix: Support auto-read-only option
If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
block/file-posix.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 2da3a76355..0c1b81ce4b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -527,9 +527,22 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
s->fd = -1;
fd = qemu_open(filename, s->open_flags, 0644);
-if (fd < 0) {
-ret = -errno;
-error_setg_errno(errp, errno, "Could not open '%s'", filename);
+ret = fd < 0 ? -errno : 0;
+
+if (ret == -EACCES || ret == -EROFS) {
+/* Try to degrade to read-only, but if it doesn't work, still use the
+ * normal error message. */
+if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+bdrv_flags &= ~BDRV_O_RDWR;
+raw_parse_flags(bdrv_flags, &s->open_flags);
+assert(!(s->open_flags & O_CREAT));
+fd = qemu_open(filename, s->open_flags);
+ret = fd < 0 ? -errno : 0;
+}
+}
+
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not open '%s'", filename);
if (ret == -EROFS) {
ret = -EACCES;
}
--
2.19.1
[Qemu-block] [PATCH v4 00/11] block: Add auto-read-only option
See patch 2 for an explanation of the motivation. v4: - Split fix for missing rbd_close() into a separate patch [Eric] - Added qemu-iotests case v3: - Clarified QAPI schema documentation that auto-read-only can only degrade read-write to read-only, not the other way round [Eric] - Don't refuse to set copy-on-read=on and auto-read-only=on at the same time; only complain when actually trying to degrade to read-only - Let bdrv_apply_auto_read_only() return -EACCESS on all errors - Fixed file-posix and gluster implementations [Eric, Niels] - Added a patch to make auto-read-only=on the default for human user interfaces (-drive/-hda/...) v2: - Turn bdrv_set_read_only() into bdrv_apply_auto_read_only() - Support the option in a lot more block drivers Kevin Wolf (11): block: Update flags in bdrv_set_read_only() block: Add auto-read-only option rbd: Close image in qemu_rbd_open() error path block: Require auto-read-only for existing fallbacks nbd: Support auto-read-only option file-posix: Support auto-read-only option curl: Support auto-read-only option gluster: Support auto-read-only option iscsi: Support auto-read-only option block: Make auto-read-only=on default for -drive qemu-iotests: Test auto-read-only with -drive and -blockdev qapi/block-core.json | 7 ++ include/block/block.h | 5 +- block.c| 54 +++--- block/bochs.c | 17 ++--- block/cloop.c | 16 ++-- block/curl.c | 8 +- block/dmg.c| 16 ++-- block/file-posix.c | 19 - block/gluster.c| 12 ++- block/iscsi.c | 8 +- block/nbd-client.c | 10 +-- block/rbd.c| 14 +--- block/vvfat.c | 11 +-- blockdev.c | 1 + tests/qemu-iotests/232 | 147 + tests/qemu-iotests/232.out | 59 +++ tests/qemu-iotests/group | 1 + 17 files changed, 327 insertions(+), 78 deletions(-) create mode 100755 tests/qemu-iotests/232 create mode 100644 tests/qemu-iotests/232.out -- 2.19.1
[Qemu-block] [PATCH v4 07/11] curl: Support auto-read-only option
If read-only=off, but auto-read-only=on is given, just degrade to
read-only.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
---
block/curl.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index fabb2b4da7..db5d2bd8ef 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -684,10 +684,10 @@ static int curl_open(BlockDriverState *bs, QDict
*options, int flags,
const char *protocol_delimiter;
int ret;
-
-if (flags & BDRV_O_RDWR) {
-error_setg(errp, "curl block device does not support writes");
-return -EROFS;
+ret = bdrv_apply_auto_read_only(bs, "curl driver does not support writes",
+errp);
+if (ret < 0) {
+return ret;
}
if (!libcurl_initialized) {
--
2.19.1
[Qemu-block] [PATCH v4 01/11] block: Update flags in bdrv_set_read_only()
To fully change the read-only state of a node, we must not only change
bs->read_only, but also update bs->open_flags.
Signed-off-by: Kevin Wolf
Reviewed-by: Eric Blake
Reviewed-by: Alberto Garcia
---
block.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block.c b/block.c
index 0d6e5f1a76..d7bd6d29b4 100644
--- a/block.c
+++ b/block.c
@@ -281,6 +281,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool
read_only, Error **errp)
}
bs->read_only = read_only;
+
+if (read_only) {
+bs->open_flags &= ~BDRV_O_RDWR;
+} else {
+bs->open_flags |= BDRV_O_RDWR;
+}
+
return 0;
}
--
2.19.1
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] iotests: Make nbd-fault-injector flush
On 10/19/18 4:48 AM, Max Reitz wrote: On 16.10.18 20:07, Eric Blake wrote: On 10/15/18 9:14 AM, Max Reitz wrote: When closing a connection, make the nbd-fault-injector flush the socket. Without this, the output is a bit unreliable with Python 3. Signed-off-by: Max Reitz --- tests/qemu-iotests/083.out | 9 + tests/qemu-iotests/nbd-fault-injector.py | 1 + 2 files changed, 10 insertions(+) I already had a complaint that the error message in 083.out should NOT be printing a message You mean the NBD server itself, right? Yes, the NBD server should not be printing the redundant error messages that 083.out exposes. (whether the server is python 2 and auto-flushes, or python 3 and needs an explicit flush, the message itself is pointless, and the test is racy as a result). We may need to revisit this patch when that thread is resolved. https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg01041.html Well, it's not like the flush hurts either way. :-) True. On that grounds: Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [Qemu-devel] [PATCH 7/9] iotests: 'new' module replacement in 169
On Fri, Oct 19, 2018 at 11:46:59AM +0200, Max Reitz wrote: [...] > I mean, my personal goal is not to touch this beyond what I need to > because it's black magic to me anyway, so I'm happy with what works. I agree with this approach, and your original patch looks good enough to me. -- Eduardo
[Qemu-block] [PATCH v3 15/15] block: Assert that flags are up-to-date in bdrv_reopen_prepare()
Towards the end of bdrv_reopen_queue_child(), before starting to
process the children, the update_flags_from_options() function is
called in order to have BDRVReopenState.flags in sync with the options
from the QDict.
This is necessary because during the reopen process flags must be
updated for all nodes in the queue so bdrv_is_writable_after_reopen()
and the permission checks work correctly.
Because of that, calling update_flags_from_options() again in
bdrv_reopen_prepare() doesn't really change the flags (they are
already up-to-date). But we need to call it in order to remove those
options from QemuOpts and that way indicate that they have been
processed.
Signed-off-by: Alberto Garcia
---
block.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block.c b/block.c
index 0a0de2dc78..1d1cf3fb5f 100644
--- a/block.c
+++ b/block.c
@@ -3143,6 +3143,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
Error **errp)
{
int ret = -1;
+int old_flags;
Error *local_err = NULL;
BlockDriver *drv;
QemuOpts *opts;
@@ -3168,7 +3169,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
goto error;
}
+/* This was already called in bdrv_reopen_queue_child() so the flags
+ * are up-to-date. This time we simply want to remove the options from
+ * QemuOpts in order to indicate that they have been processed. */
+old_flags = reopen_state->flags;
update_flags_from_options(&reopen_state->flags, opts);
+assert(old_flags == reopen_state->flags);
discard = qemu_opt_get_del(opts, "discard");
if (discard != NULL) {
--
2.11.0
[Qemu-block] [PATCH v3 13/15] block: Stop passing flags to bdrv_reopen_queue_child()
Now that all callers are passing the new options using the QDict we no
longer need the 'flags' parameter.
This patch makes the following changes:
1) The update_options_from_flags() call is no longer necessary
so it can be removed.
2) The update_flags_from_options() call is now used in all cases,
and is moved down a few lines so it happens after the options
QDict contains the final set of values.
3) The flags parameter is removed. Now the flags are initialized
using the current value (for the top-level node) or the parent
flags (after inherit_options()). In both cases the initial
values are updated to reflect the new options in the QDict. This
happens in bdrv_reopen_queue_child() (as explained above) and in
bdrv_reopen_prepare().
Signed-off-by: Alberto Garcia
---
block.c | 48 +++-
1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/block.c b/block.c
index c142773358..039f03703b 100644
--- a/block.c
+++ b/block.c
@@ -2876,7 +2876,6 @@ BlockDriverState *bdrv_open(const char *filename, const
char *reference,
static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
QDict *options,
- int flags,
const BdrvChildRole *role,
QDict *parent_options,
int parent_flags)
@@ -2885,7 +2884,9 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockReopenQueueEntry *bs_entry;
BdrvChild *child;
-QDict *old_options, *explicit_options;
+QDict *old_options, *explicit_options, *options_copy;
+int flags;
+QemuOpts *opts;
/* Make sure that the caller remembered to use a drained section. This is
* important to avoid graph changes between the recursive queuing here and
@@ -2911,22 +2912,11 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
/*
* Precedence of options:
* 1. Explicitly passed in options (highest)
- * 2. Set in flags (only for top level)
- * 3. Retained from explicitly set options of bs
- * 4. Inherited from parent node
- * 5. Retained from effective options of bs
+ * 2. Retained from explicitly set options of bs
+ * 3. Inherited from parent node
+ * 4. Retained from effective options of bs
*/
-if (!parent_options) {
-/*
- * Any setting represented by flags is always updated. If the
- * corresponding QDict option is set, it takes precedence. Otherwise
- * the flag is translated into a QDict option. The old setting of bs is
- * not considered.
- */
-update_options_from_flags(options, flags);
-}
-
/* Old explicitly set values (don't overwrite by inherited value) */
if (bs_entry) {
old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
@@ -2940,16 +2930,10 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
/* Inherit from parent node */
if (parent_options) {
-QemuOpts *opts;
-QDict *options_copy;
-assert(!flags);
+flags = 0;
role->inherit_options(&flags, options, parent_flags, parent_options);
-options_copy = qdict_clone_shallow(options);
-opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options_copy, NULL);
-update_flags_from_options(&flags, opts);
-qemu_opts_del(opts);
-qobject_unref(options_copy);
+} else {
+flags = bdrv_get_flags(bs);
}
/* Old values are used for options that aren't set yet */
@@ -2957,6 +2941,14 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
bdrv_join_options(bs, options, old_options);
qobject_unref(old_options);
+/* We have the final set of options so let's update the flags */
+options_copy = qdict_clone_shallow(options);
+opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+qemu_opts_absorb_qdict(opts, options_copy, NULL);
+update_flags_from_options(&flags, opts);
+qemu_opts_del(opts);
+qobject_unref(options_copy);
+
/* bdrv_open_inherit() sets and clears some additional flags internally */
flags &= ~BDRV_O_PROTOCOL;
if (flags & BDRV_O_RDWR) {
@@ -2996,7 +2988,7 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
qdict_extract_subqdict(options, &new_child_options, child_key_dot);
g_free(child_key_dot);
-bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
+bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
[Qemu-block] [PATCH v3 00/15] Don't pass flags to bdrv_reopen_queue()
Hi all, when reopening a BlockDriverState using bdrv_reopen() and friends the new options can be specified either with a QDict or with flags. Both methods overlap and that makes the semantics and the implementation unnecessarily complicated. This series removes the 'flags' parameter from these functions, so from now on all option changes must be specified using a QDict. Apart from simplifying the API, a few bugs are fixed along the way. See the individual patches for more details. This was tested with the current master (09558375a634e17cea6cfbfec88). Regards, Berto v3: - Patch 1 [from v2] has been removed and all other patch numbers are shifted. - Patches 10 and 13: Fixed rebase conflicts after the patch removal. - Patch 14: Replacement for the removed patch using a different approach. - Patch 15: Document a non-obvious call to update_flags_from_options() v2: https://lists.gnu.org/archive/html/qemu-block/2018-10/msg00534.html - Patches 4 and 9: Fixed trivial rebase conflict - Patch 11: Update messages [Max] - Patch 12: Add comment and use bdrv_is_read_only() instead of using the flags directly [Max] - Patch 14: Remove inner block and move all variable declarations to the beginning of the function [Max] v1: https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00483.html - Initial version Output of backport-diff against v2: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/15:[] [--] 'block: Add bdrv_reopen_set_read_only()' 002/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()' 003/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in commit_start/complete()' 004/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_commit()' 005/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in stream_start/complete()' 006/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()' 007/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()' 008/15:[] [--] 'block: Use bdrv_reopen_set_read_only() in the mirror driver' 009/15:[] [--] 'block: Drop bdrv_reopen()' 010/15:[] [-C] 'qemu-io: Put flag changes in the options QDict in reopen_f()' 011/15:[] [--] 'block: Clean up reopen_backing_file() in block/replication.c' 012/15:[] [--] 'block: Remove flags parameter from bdrv_reopen_queue()' 013/15:[0022] [FC] 'block: Stop passing flags to bdrv_reopen_queue_child()' 014/15:[down] 'block: Remove assertions from update_flags_from_options()' 015/15:[down] 'block: Assert that flags are up-to-date in bdrv_reopen_prepare()' Alberto Garcia (15): block: Add bdrv_reopen_set_read_only() block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename() block: Use bdrv_reopen_set_read_only() in commit_start/complete() block: Use bdrv_reopen_set_read_only() in bdrv_commit() block: Use bdrv_reopen_set_read_only() in stream_start/complete() block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file() block: Use bdrv_reopen_set_read_only() in external_snapshot_commit() block: Use bdrv_reopen_set_read_only() in the mirror driver block: Drop bdrv_reopen() qemu-io: Put flag changes in the options QDict in reopen_f() block: Clean up reopen_backing_file() in block/replication.c block: Remove flags parameter from bdrv_reopen_queue() block: Stop passing flags to bdrv_reopen_queue_child() block: Remove assertions from update_flags_from_options() block: Assert that flags are up-to-date in bdrv_reopen_prepare() block.c| 88 -- block/commit.c | 23 +--- block/mirror.c | 19 ++ block/replication.c| 43 ++ block/stream.c | 20 +-- blockdev.c | 11 ++ include/block/block.h | 6 ++-- qemu-io-cmds.c | 29 +-- tests/qemu-iotests/133 | 17 + tests/qemu-iotests/133.out | 14 10 files changed, 153 insertions(+), 117 deletions(-) -- 2.11.0
[Qemu-block] [PATCH v3 12/15] block: Remove flags parameter from bdrv_reopen_queue()
Now that all callers are passing all flag changes as QDict options,
the flags parameter is no longer necessary, so we can get rid of it.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 5 +++--
block/replication.c | 6 ++
include/block/block.h | 3 +--
qemu-io-cmds.c| 2 +-
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index edb880a66b..c142773358 100644
--- a/block.c
+++ b/block.c
@@ -3005,8 +3005,9 @@ static BlockReopenQueue
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
-QDict *options, int flags)
+QDict *options)
{
+int flags = bdrv_get_flags(bs);
return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
NULL, NULL, 0);
}
@@ -3080,7 +3081,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool
read_only,
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
bdrv_subtree_drained_begin(bs);
-queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+queue = bdrv_reopen_queue(NULL, bs, opts);
ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
bdrv_subtree_drained_end(bs);
diff --git a/block/replication.c b/block/replication.c
index 481a225924..fdbfe47fa4 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -393,19 +393,17 @@ static void reopen_backing_file(BlockDriverState *bs,
bool writable,
bdrv_subtree_drained_begin(s->secondary_disk->bs);
if (s->orig_hidden_read_only) {
-int flags = bdrv_get_flags(s->hidden_disk->bs);
QDict *opts = qdict_new();
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
- opts, flags);
+ opts);
}
if (s->orig_secondary_read_only) {
-int flags = bdrv_get_flags(s->secondary_disk->bs);
QDict *opts = qdict_new();
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- opts, flags);
+ opts);
}
if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index 3649136689..38d5c1adf4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -297,8 +297,7 @@ BlockDriverState *bdrv_open(const char *filename, const
char *reference,
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
int flags, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-BlockDriverState *bs,
-QDict *options, int flags);
+BlockDriverState *bs, QDict *options);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error
**errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index b49a816cc8..e272e5c80b 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2075,7 +2075,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
}
bdrv_subtree_drained_begin(bs);
-brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+brq = bdrv_reopen_queue(NULL, bs, opts);
bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
bdrv_subtree_drained_end(bs);
--
2.11.0
[Qemu-block] [PATCH v3 05/15] block: Use bdrv_reopen_set_read_only() in stream_start/complete()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/stream.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 81a7ec8ece..262d280ccd 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,7 @@ typedef struct StreamBlockJob {
BlockDriverState *base;
BlockdevOnError on_error;
char *backing_file_str;
-int bs_flags;
+bool bs_read_only;
} StreamBlockJob;
static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -89,10 +89,10 @@ static void stream_clean(Job *job)
BlockDriverState *bs = blk_bs(bjob->blk);
/* Reopen the image back in read-only mode if necessary */
-if (s->bs_flags != bdrv_get_flags(bs)) {
+if (s->bs_read_only) {
/* Give up write permissions before making it read-only */
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
-bdrv_reopen(bs, s->bs_flags, NULL);
+bdrv_reopen_set_read_only(bs, true, NULL);
}
g_free(s->backing_file_str);
@@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState
*bs,
{
StreamBlockJob *s;
BlockDriverState *iter;
-int orig_bs_flags;
+int bs_read_only;
/* Make sure that the image is opened in read-write mode */
-orig_bs_flags = bdrv_get_flags(bs);
-if (!(orig_bs_flags & BDRV_O_RDWR)) {
-if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
+bs_read_only = bdrv_is_read_only(bs);
+if (bs_read_only) {
+if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
return;
}
}
@@ -261,7 +261,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base = base;
s->backing_file_str = g_strdup(backing_file_str);
-s->bs_flags = orig_bs_flags;
+s->bs_read_only = bs_read_only;
s->on_error = on_error;
trace_stream_start(bs, base, s);
@@ -269,7 +269,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
return;
fail:
-if (orig_bs_flags != bdrv_get_flags(bs)) {
-bdrv_reopen(bs, orig_bs_flags, NULL);
+if (bs_read_only) {
+bdrv_reopen_set_read_only(bs, true, NULL);
}
}
--
2.11.0
[Qemu-block] [PATCH v3 10/15] qemu-io: Put flag changes in the options QDict in reopen_f()
When reopen_f() puts a block device in the reopen queue, some of the
new options are passed using a QDict, but others ("read-only" and the
cache options) are passed as flags.
This patch puts those flags in the QDict. This way the flags parameter
becomes redundant and we'll be able to get rid of it in a subsequent
patch.
Signed-off-by: Alberto Garcia
---
qemu-io-cmds.c | 27 ++-
tests/qemu-iotests/133 | 9 +
tests/qemu-iotests/133.out | 8
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index db0b3ee5ef..b49a816cc8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
#include "qemu-io.h"
#include "sysemu/block-backend.h"
#include "block/block.h"
@@ -1978,6 +1979,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
int flags = bs->open_flags;
bool writethrough = !blk_enable_write_cache(blk);
bool has_rw_option = false;
+bool has_cache_option = false;
BlockReopenQueue *brq;
Error *local_err = NULL;
@@ -1989,6 +1991,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
error_report("Invalid cache option: %s", optarg);
return -EINVAL;
}
+has_cache_option = true;
break;
case 'o':
if (!qemu_opts_parse_noisily(&reopen_opts, optarg, 0)) {
@@ -2046,9 +2049,31 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
}
qopts = qemu_opts_find(&reopen_opts, NULL);
-opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
qemu_opts_reset(&reopen_opts);
+if (qdict_haskey(opts, BDRV_OPT_READ_ONLY)) {
+if (has_rw_option) {
+error_report("Cannot set both -r/-w and '" BDRV_OPT_READ_ONLY "'");
+qobject_unref(opts);
+return -EINVAL;
+}
+} else {
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR));
+}
+
+if (qdict_haskey(opts, BDRV_OPT_CACHE_DIRECT) ||
+qdict_haskey(opts, BDRV_OPT_CACHE_NO_FLUSH)) {
+if (has_cache_option) {
+error_report("Cannot set both -c and the cache options");
+qobject_unref(opts);
+return -EINVAL;
+}
+} else {
+qdict_put_bool(opts, BDRV_OPT_CACHE_DIRECT, flags & BDRV_O_NOCACHE);
+qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
+}
+
bdrv_subtree_drained_begin(bs);
brq = bdrv_reopen_queue(NULL, bs, opts, flags);
bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index af6b3e1dd4..14e6b3b972 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -92,6 +92,15 @@ echo
IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
"json:{'driver': 'null-co', 'size': 65536}"
+echo
+echo "=== Check that mixing -c/-r/-w and their corresponding options is
forbidden ==="
+echo
+
+$QEMU_IO -c 'reopen -r -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index f4a85aeb63..48a9d087f0 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -24,4 +24,12 @@ Cannot change the option 'driver'
format name: null-co
format name: null-co
+
+=== Check that mixing -c/-r/-w and their corresponding options is forbidden ===
+
+Cannot set both -r/-w and 'read-only'
+Cannot set both -r/-w and 'read-only'
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
*** done
--
2.11.0
[Qemu-block] [PATCH v3 06/15] block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
blockdev.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a8755bd908..f64569795c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4064,7 +4064,6 @@ void qmp_change_backing_file(const char *device,
BlockDriverState *image_bs = NULL;
Error *local_err = NULL;
bool ro;
-int open_flags;
int ret;
bs = qmp_get_root_bs(device, errp);
@@ -4106,13 +4105,10 @@ void qmp_change_backing_file(const char *device,
}
/* if not r/w, reopen to make r/w */
-open_flags = image_bs->open_flags;
ro = bdrv_is_read_only(image_bs);
if (ro) {
-bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (bdrv_reopen_set_read_only(image_bs, false, errp) != 0) {
goto out;
}
}
@@ -4128,7 +4124,7 @@ void qmp_change_backing_file(const char *device,
}
if (ro) {
-bdrv_reopen(image_bs, open_flags, &local_err);
+bdrv_reopen_set_read_only(image_bs, true, &local_err);
error_propagate(errp, local_err);
}
--
2.11.0
[Qemu-block] [PATCH v3 11/15] block: Clean up reopen_backing_file() in block/replication.c
This function is used to put the hidden and secondary disks in
read-write mode before launching the backup job, and back in read-only
mode afterwards.
This patch does the following changes:
- Use an options QDict with the "read-only" option instead of
passing the changes as flags only.
- Simplify the code (it was unnecessarily complicated and verbose).
- Fix a bug due to which the secondary disk was not being put back
in read-only mode when writable=false (because in this case
orig_secondary_flags always had the BDRV_O_RDWR flag set).
- Stop clearing the BDRV_O_INACTIVE flag.
The flags parameter to bdrv_reopen_queue() becomes redundant and we'll
be able to get rid of it in a subsequent patch.
Signed-off-by: Alberto Garcia
---
block/replication.c | 45 +
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 6349d6958e..481a225924 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -20,6 +20,7 @@
#include "block/block_backup.h"
#include "sysemu/block-backend.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
#include "replication.h"
typedef enum {
@@ -39,8 +40,8 @@ typedef struct BDRVReplicationState {
char *top_id;
ReplicationState *rs;
Error *blocker;
-int orig_hidden_flags;
-int orig_secondary_flags;
+bool orig_hidden_read_only;
+bool orig_secondary_read_only;
int error;
} BDRVReplicationState;
@@ -371,44 +372,40 @@ static void secondary_do_checkpoint(BDRVReplicationState
*s, Error **errp)
}
}
+/* This function is supposed to be called twice:
+ * first with writable = true, then with writable = false.
+ * The first call puts s->hidden_disk and s->secondary_disk in
+ * r/w mode, and the second puts them back in their original state.
+ */
static void reopen_backing_file(BlockDriverState *bs, bool writable,
Error **errp)
{
BDRVReplicationState *s = bs->opaque;
BlockReopenQueue *reopen_queue = NULL;
-int orig_hidden_flags, orig_secondary_flags;
-int new_hidden_flags, new_secondary_flags;
Error *local_err = NULL;
if (writable) {
-orig_hidden_flags = s->orig_hidden_flags =
-bdrv_get_flags(s->hidden_disk->bs);
-new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-orig_secondary_flags = s->orig_secondary_flags =
-bdrv_get_flags(s->secondary_disk->bs);
-new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) &
- ~BDRV_O_INACTIVE;
-} else {
-orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-new_hidden_flags = s->orig_hidden_flags;
-orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-new_secondary_flags = s->orig_secondary_flags;
+s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
+s->orig_secondary_read_only = bdrv_is_read_only(s->secondary_disk->bs);
}
bdrv_subtree_drained_begin(s->hidden_disk->bs);
bdrv_subtree_drained_begin(s->secondary_disk->bs);
-if (orig_hidden_flags != new_hidden_flags) {
-reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
NULL,
- new_hidden_flags);
+if (s->orig_hidden_read_only) {
+int flags = bdrv_get_flags(s->hidden_disk->bs);
+QDict *opts = qdict_new();
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
+reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
+ opts, flags);
}
-if (!(orig_secondary_flags & BDRV_O_RDWR)) {
+if (s->orig_secondary_read_only) {
+int flags = bdrv_get_flags(s->secondary_disk->bs);
+QDict *opts = qdict_new();
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- NULL, new_secondary_flags);
+ opts, flags);
}
if (reopen_queue) {
--
2.11.0
[Qemu-block] [PATCH v3 07/15] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
This patch replaces the bdrv_reopen() call that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index f64569795c..7a57572e1a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1701,8 +1701,7 @@ static void external_snapshot_commit(BlkActionState
*common)
* bdrv_reopen_multiple() across all the entries at once, because we
* don't want to abort all of them if one of them fails the reopen */
if (!atomic_read(&state->old_bs->copy_on_read)) {
-bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
-NULL);
+bdrv_reopen_set_read_only(state->old_bs, true, NULL);
}
aio_context_release(aio_context);
--
2.11.0
[Qemu-block] [PATCH v3 14/15] block: Remove assertions from update_flags_from_options()
This function takes three options (cache.direct, cache.no-flush and
read-only) from a QemuOpts object and updates the flags accordingly.
If any of those options is not set (because it was missing from the
original QDict or because it had an invalid value) then the function
aborts with a failed assertion:
$ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
block.c:1126: update_flags_from_options: Assertion `qemu_opt_find(opts,
BDRV_OPT_CACHE_DIRECT)' failed.
Aborted
This assertion is unnecessary, and it forces any caller of
bdrv_reopen() to pass all the aforementioned three options. This may
have made sense in order to remove ambiguity when bdrv_reopen() was
taking both flags and options, but that's not the case anymore.
It's also unnecessary if we want to validate the option values,
because bdrv_reopen_prepare() already takes care of that, as we can
see if we remove the assertions:
$ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
Parameter 'read-only' expects 'on' or 'off'
Signed-off-by: Alberto Garcia
---
block.c| 3 ---
tests/qemu-iotests/133 | 8
tests/qemu-iotests/133.out | 6 ++
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 039f03703b..0a0de2dc78 100644
--- a/block.c
+++ b/block.c
@@ -1118,19 +1118,16 @@ static void update_flags_from_options(int *flags,
QemuOpts *opts)
{
*flags &= ~BDRV_O_CACHE_MASK;
-assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
*flags |= BDRV_O_NO_FLUSH;
}
-assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_DIRECT, false)) {
*flags |= BDRV_O_NOCACHE;
}
*flags &= ~BDRV_O_RDWR;
-assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY));
if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) {
*flags |= BDRV_O_RDWR;
}
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index 14e6b3b972..59d5e2ea25 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -101,6 +101,14 @@ $QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
+
+echo
+echo "=== Check that invalid options are handled correctly ==="
+echo
+
+$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
+$QEMU_IO -c 'reopen -o cache.no-flush=bar' $TEST_IMG
+$QEMU_IO -c 'reopen -o cache.direct=baz' $TEST_IMG
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index 48a9d087f0..551096a9c4 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -32,4 +32,10 @@ Cannot set both -r/-w and 'read-only'
Cannot set both -c and the cache options
Cannot set both -c and the cache options
Cannot set both -c and the cache options
+
+=== Check that invalid options are handled correctly ===
+
+Parameter 'read-only' expects 'on' or 'off'
+Parameter 'cache.no-flush' expects 'on' or 'off'
+Parameter 'cache.direct' expects 'on' or 'off'
*** done
--
2.11.0
[Qemu-block] [PATCH v3 04/15] block: Use bdrv_reopen_set_read_only() in bdrv_commit()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/commit.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index a53c2d04b0..53148e610b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -391,7 +391,7 @@ int bdrv_commit(BlockDriverState *bs)
BlockDriverState *commit_top_bs = NULL;
BlockDriver *drv = bs->drv;
int64_t offset, length, backing_length;
-int ro, open_flags;
+int ro;
int64_t n;
int ret = 0;
uint8_t *buf = NULL;
@@ -410,10 +410,9 @@ int bdrv_commit(BlockDriverState *bs)
}
ro = bs->backing->bs->read_only;
-open_flags = bs->backing->bs->open_flags;
if (ro) {
-if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
+if (bdrv_reopen_set_read_only(bs->backing->bs, false, NULL)) {
return -EACCES;
}
}
@@ -523,7 +522,7 @@ ro_cleanup:
if (ro) {
/* ignoring error return here */
-bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
+bdrv_reopen_set_read_only(bs->backing->bs, true, NULL);
}
return ret;
--
2.11.0
[Qemu-block] [PATCH v3 03/15] block: Use bdrv_reopen_set_read_only() in commit_start/complete()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/commit.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index a2da5740b0..a53c2d04b0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -38,7 +38,7 @@ typedef struct CommitBlockJob {
BlockBackend *base;
BlockDriverState *base_bs;
BlockdevOnError on_error;
-int base_flags;
+bool base_read_only;
char *backing_file_str;
} CommitBlockJob;
@@ -124,8 +124,8 @@ static void commit_clean(Job *job)
/* restore base open flags here if appropriate (e.g., change the base back
* to r/o). These reopens do not need to be atomic, since we won't abort
* even on failure here */
-if (s->base_flags != bdrv_get_flags(s->base_bs)) {
-bdrv_reopen(s->base_bs, s->base_flags, NULL);
+if (s->base_read_only) {
+bdrv_reopen_set_read_only(s->base_bs, true, NULL);
}
g_free(s->backing_file_str);
@@ -264,7 +264,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
const char *filter_node_name, Error **errp)
{
CommitBlockJob *s;
-int orig_base_flags;
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
Error *local_err = NULL;
@@ -283,11 +282,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
}
/* convert base to r/w, if necessary */
-orig_base_flags = bdrv_get_flags(base);
-if (!(orig_base_flags & BDRV_O_RDWR)) {
-bdrv_reopen(base, orig_base_flags | BDRV_O_RDWR, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+s->base_read_only = bdrv_is_read_only(base);
+if (s->base_read_only) {
+if (bdrv_reopen_set_read_only(base, false, errp) != 0) {
goto fail;
}
}
@@ -363,7 +360,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
goto fail;
}
-s->base_flags = orig_base_flags;
s->backing_file_str = g_strdup(backing_file_str);
s->on_error = on_error;
--
2.11.0
[Qemu-block] [PATCH v3 08/15] block: Use bdrv_reopen_set_read_only() in the mirror driver
The 'block-commit' QMP command is implemented internally using two
different drivers. If the source image is the active layer then the
mirror driver is used (commit_active_start()), otherwise the commit
driver is used (commit_start()).
In both cases the destination image must be put temporarily in
read-write mode. This is done correctly in the latter case, but what
commit_active_start() does is copy all flags instead.
This patch replaces the bdrv_reopen() calls in that function with
bdrv_reopen_set_read_only() so that only the read-only status is
changed.
A similar change is made in mirror_exit(), which is also used by the
'drive-mirror' and 'blockdev-mirror' commands.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/mirror.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..41b6cbaad6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -672,9 +672,10 @@ static int mirror_exit_common(Job *job)
if (s->should_complete && !abort) {
BlockDriverState *to_replace = s->to_replace ?: src;
+bool ro = bdrv_is_read_only(to_replace);
-if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
-bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
+if (ro != bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, ro, NULL);
}
/* The mirror job has no requests in flight any more, but we need to
@@ -1692,13 +1693,15 @@ void commit_active_start(const char *job_id,
BlockDriverState *bs,
BlockCompletionFunc *cb, void *opaque,
bool auto_complete, Error **errp)
{
-int orig_base_flags;
+bool base_read_only;
Error *local_err = NULL;
-orig_base_flags = bdrv_get_flags(base);
+base_read_only = bdrv_is_read_only(base);
-if (bdrv_reopen(base, bs->open_flags, errp)) {
-return;
+if (base_read_only) {
+if (bdrv_reopen_set_read_only(base, false, errp) < 0) {
+return;
+}
}
mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
@@ -1717,6 +1720,8 @@ void commit_active_start(const char *job_id,
BlockDriverState *bs,
error_restore_flags:
/* ignore error and errp for bdrv_reopen, because we want to propagate
* the original error */
-bdrv_reopen(base, orig_base_flags, NULL);
+if (base_read_only) {
+bdrv_reopen_set_read_only(base, true, NULL);
+}
return;
}
--
2.11.0
[Qemu-block] [PATCH v3 02/15] block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()
This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index d09019d9c9..744509f683 100644
--- a/block.c
+++ b/block.c
@@ -1058,11 +1058,11 @@ static int bdrv_backing_update_filename(BdrvChild *c,
BlockDriverState *base,
const char *filename, Error **errp)
{
BlockDriverState *parent = c->opaque;
-int orig_flags = bdrv_get_flags(parent);
+bool read_only = bdrv_is_read_only(parent);
int ret;
-if (!(orig_flags & BDRV_O_RDWR)) {
-ret = bdrv_reopen(parent, orig_flags | BDRV_O_RDWR, errp);
+if (read_only) {
+ret = bdrv_reopen_set_read_only(parent, false, errp);
if (ret < 0) {
return ret;
}
@@ -1074,8 +1074,8 @@ static int bdrv_backing_update_filename(BdrvChild *c,
BlockDriverState *base,
error_setg_errno(errp, -ret, "Could not update backing file link");
}
-if (!(orig_flags & BDRV_O_RDWR)) {
-bdrv_reopen(parent, orig_flags, NULL);
+if (read_only) {
+bdrv_reopen_set_read_only(parent, true, NULL);
}
return ret;
--
2.11.0
[Qemu-block] [PATCH v3 09/15] block: Drop bdrv_reopen()
No one is using this function anymore, so we can safely remove it.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 21 -
include/block/block.h | 1 -
2 files changed, 22 deletions(-)
diff --git a/block.c b/block.c
index 744509f683..edb880a66b 100644
--- a/block.c
+++ b/block.c
@@ -3070,27 +3070,6 @@ cleanup:
return ret;
}
-
-/* Reopen a single BlockDriverState with the specified flags. */
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
-{
-int ret = -1;
-Error *local_err = NULL;
-BlockReopenQueue *queue;
-
-bdrv_subtree_drained_begin(bs);
-
-queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
-ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-}
-
-bdrv_subtree_drained_end(bs);
-
-return ret;
-}
-
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp)
{
diff --git a/include/block/block.h b/include/block/block.h
index 97317fdfb6..3649136689 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -300,7 +300,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue
*bs_queue,
BlockDriverState *bs,
QDict *options, int flags);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error
**errp);
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
--
2.11.0
[Qemu-block] [PATCH v3 01/15] block: Add bdrv_reopen_set_read_only()
Most callers of bdrv_reopen() only use it to switch a BlockDriverState
between read-only and read-write, so this patch adds a new function
that does just that.
We also want to get rid of the flags parameter in the bdrv_reopen()
API, so this function sets the "read-only" option and passes the
original flags (which will then be updated in bdrv_reopen_prepare()).
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 17 +
include/block/block.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/block.c b/block.c
index 7710b399a3..d09019d9c9 100644
--- a/block.c
+++ b/block.c
@@ -3091,6 +3091,23 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags,
Error **errp)
return ret;
}
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+ Error **errp)
+{
+int ret;
+BlockReopenQueue *queue;
+QDict *opts = qdict_new();
+
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+bdrv_subtree_drained_begin(bs);
+queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
+bdrv_subtree_drained_end(bs);
+
+return ret;
+}
+
static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
BdrvChild *c)
{
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..97317fdfb6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -301,6 +301,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue
*bs_queue,
QDict *options, int flags);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error
**errp);
int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+ Error **errp);
int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, Error **errp);
void bdrv_reopen_commit(BDRVReopenState *reopen_state);
--
2.11.0
Re: [Qemu-block] [Qemu-devel] [PATCH] block: drop moderated sheepdog mailing list from MAINTAINERS file
On 19/10/2018 13:25, Daniel P. Berrangé wrote: > Paolo, I think this is a patch for your misc patches queue. It looks to me than this is more block/trivial than the busy misc queue. > > On Wed, Mar 21, 2018 at 03:31:24PM +, Daniel P. Berrangé wrote: >> The sheepdog mailing list is setup to stop and queue messages from >> non-subscribers, pending moderator approval. Unfortunately it seems >> that the moderation queue is not actively dealt with. Even when messages >> are approved, the sender is never added to the whitelist, so every >> future mail the same sender continues to get stopped for moderation. >> >> MAINTAINERS entries should be responsive and not uneccessarily block >> mails from QEMU contributors, so drop the sheepdog mailing list. >> >> Signed-off-by: Daniel P. Berrangé >> --- >> MAINTAINERS | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 354a18ce49..7eea869dcd 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -1871,7 +1871,6 @@ M: Hitoshi Mitake >> M: Liu Yuan >> M: Jeff Cody >> L: qemu-block@nongnu.org >> -L: sheep...@lists.wpkg.org >> S: Supported >> F: block/sheepdog.c >> T: git git://github.com/codyprime/qemu-kvm-jtc.git block >> -- >> 2.14.3 >> > > Regards, > Daniel >
[Qemu-block] [PATCH] block: Make more block drivers compile-time configurable
From: Jeff Cody This adds configure options to control the following block drivers: * Bochs * Cloop * Dmg * Qcow (V1) * Vdi * Vvfat * qed * parallels * sheepdog Each of these defaults to being enabled. Signed-off-by: Jeff Cody Signed-off-by: Markus Armbruster --- Hmm, we got quite a few --enable-BLOCK-DRIVER now. Perhaps a single list-valued option similar --target-list would be better. Could be done on top. block/Makefile.objs | 22 --- configure | 91 + 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/block/Makefile.objs b/block/Makefile.objs index c8337bf186..1cad9fc4f1 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -1,10 +1,18 @@ -block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o +block-obj-y += raw-format.o vmdk.o vpc.o +block-obj-$(CONFIG_QCOW1) += qcow.o +block-obj-$(CONFIG_VDI) += vdi.o +block-obj-$(CONFIG_CLOOP) += cloop.o +block-obj-$(CONFIG_BOCHS) += bochs.o +block-obj-$(CONFIG_VVFAT) += vvfat.o +block-obj-$(CONFIG_DMG) += dmg.o + block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o -block-obj-y += qed.o qed-l2-cache.o qed-table.o qed-cluster.o -block-obj-y += qed-check.o +block-obj-$(CONFIG_QED) += qed.o qed-l2-cache.o qed-table.o qed-cluster.o +block-obj-$(CONFIG_QED) += qed-check.o block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o block-obj-y += quorum.o -block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o +block-obj-y += blkdebug.o blkverify.o blkreplay.o +block-obj-$(CONFIG_PARALLELS) += parallels.o block-obj-y += blklogwrites.o block-obj-y += block-backend.o snapshot.o qapi.o block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o @@ -14,7 +22,8 @@ block-obj-y += null.o mirror.o commit.o io.o create.o block-obj-y += throttle-groups.o block-obj-$(CONFIG_LINUX) += nvme.o -block-obj-y += nbd.o nbd-client.o sheepdog.o +block-obj-y += nbd.o nbd-client.o +block-obj-$(CONFIG_SHEEPDOG) += sheepdog.o block-obj-$(CONFIG_LIBISCSI) += iscsi.o block-obj-$(if $(CONFIG_LIBISCSI),y,n) += iscsi-opts.o block-obj-$(CONFIG_LIBNFS) += nfs.o @@ -45,7 +54,8 @@ gluster.o-libs := $(GLUSTERFS_LIBS) vxhs.o-libs:= $(VXHS_LIBS) ssh.o-cflags := $(LIBSSH2_CFLAGS) ssh.o-libs := $(LIBSSH2_LIBS) -block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o +block-obj-dmg-bz2$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o +block-obj-$(CONFIG_DMG) += $(block-obj-dmg-bz2-y) dmg-bz2.o-libs := $(BZIP2_LIBS) qcow.o-libs:= -lz linux-aio.o-libs := -laio diff --git a/configure b/configure index 9138af37f8..c413186c67 100755 --- a/configure +++ b/configure @@ -473,6 +473,15 @@ tcmalloc="no" jemalloc="no" replication="yes" vxhs="" +bochs="yes" +cloop="yes" +dmg="yes" +qcow1="yes" +vdi="yes" +vvfat="yes" +qed="yes" +parallels="yes" +sheepdog="yes" libxml2="" docker="no" debug_mutex="no" @@ -1413,6 +1422,42 @@ for opt do ;; --enable-vxhs) vxhs="yes" ;; + --disable-bochs) bochs="no" + ;; + --enable-bochs) bochs="yes" + ;; + --disable-cloop) cloop="no" + ;; + --enable-cloop) cloop="yes" + ;; + --disable-dmg) dmg="no" + ;; + --enable-dmg) dmg="yes" + ;; + --disable-qcow1) qcow1="no" + ;; + --enable-qcow1) qcow1="yes" + ;; + --disable-vdi) vdi="no" + ;; + --enable-vdi) vdi="yes" + ;; + --disable-vvfat) vvfat="no" + ;; + --enable-vvfat) vvfat="yes" + ;; + --disable-qed) qed="no" + ;; + --enable-qed) qed="yes" + ;; + --disable-parallels) parallels="no" + ;; + --enable-parallels) parallels="yes" + ;; + --disable-sheepdog) sheepdog="no" + ;; + --enable-sheepdog) sheepdog="yes" + ;; --disable-vhost-user) vhost_user="no" ;; --enable-vhost-user) @@ -1714,6 +1759,15 @@ disabled with --disable-FEATURE, default is enabled if available: qom-cast-debug cast debugging support tools build qemu-io, qemu-nbd and qemu-image tools vxhsVeritas HyperScale vDisk backend support + bochs bochs image format support + cloop cloop image format support + dmg dmg image format support + qcow1 qcow v1 image format support + vdi vdi image format support + vvfat vvfat image format support + qed qed image format support + parallels parallels image format support + sheepdogsheepdog block driver support crypto-afalgLinux AF_ALG crypto backend driver vhost-user vhost-user support capstonecapstone disassembler support @@ -6067,6 +6121,15 @@ echo "jemalloc support $jemalloc" echo "avx2 optimization $avx2_opt" echo "replication support $replication" echo "VxHS block device $vxhs" +echo "bochs support $bochs" +echo "cloop support $cloop" +echo "dmg support $dmg" +echo "qcow v1 support $qcow1" +echo "vdi support $vdi" +echo "vvfat support $vvfat" +echo "qed support $qed" +echo "parallels support $paral
Re: [Qemu-block] [PATCH] block: drop moderated sheepdog mailing list from MAINTAINERS file
Paolo, I think this is a patch for your misc patches queue. On Wed, Mar 21, 2018 at 03:31:24PM +, Daniel P. Berrangé wrote: > The sheepdog mailing list is setup to stop and queue messages from > non-subscribers, pending moderator approval. Unfortunately it seems > that the moderation queue is not actively dealt with. Even when messages > are approved, the sender is never added to the whitelist, so every > future mail the same sender continues to get stopped for moderation. > > MAINTAINERS entries should be responsive and not uneccessarily block > mails from QEMU contributors, so drop the sheepdog mailing list. > > Signed-off-by: Daniel P. Berrangé > --- > MAINTAINERS | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 354a18ce49..7eea869dcd 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1871,7 +1871,6 @@ M: Hitoshi Mitake > M: Liu Yuan > M: Jeff Cody > L: qemu-block@nongnu.org > -L: sheep...@lists.wpkg.org > S: Supported > F: block/sheepdog.c > T: git git://github.com/codyprime/qemu-kvm-jtc.git block > -- > 2.14.3 > Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] option: Make option help nicer to read
On 17.10.18 08:50, Markus Armbruster wrote: > Max Reitz writes: > >> This adds some whitespace into the option help (including indentation) >> and replaces '=' by ': ' (not least because '=' should be used for >> values, not types). Furthermore, the list name is no longer printed as >> part of every line, but only once in advance, and only if the caller did >> not print a caption already. >> >> Signed-off-by: Max Reitz >> --- >> include/qemu/option.h | 2 +- >> qemu-img.c | 4 +- >> util/qemu-option.c | 31 +- >> tests/qemu-iotests/082.out | 956 ++--- >> 4 files changed, 505 insertions(+), 488 deletions(-) > > 082 fails for me in master (dddb37495b8). If this patch fixes the > failure, its commit message should mention it. Ah, yeah, totally forgot that... Max > 091 and 142 also fail, but look unrelated. signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 1/5] option: Make option help nicer to read
On 16.10.18 14:12, Kevin Wolf wrote: > Am 15.10.2018 um 19:28 hat Max Reitz geschrieben: >> This adds some whitespace into the option help (including indentation) >> and replaces '=' by ': ' (not least because '=' should be used for >> values, not types). Furthermore, the list name is no longer printed as >> part of every line, but only once in advance, and only if the caller did >> not print a caption already. >> >> Signed-off-by: Max Reitz > > If this isn't a bike shedding series, then what is? So... Sure it is. >> --- a/tests/qemu-iotests/082.out >> +++ b/tests/qemu-iotests/082.out >> @@ -44,171 +44,171 @@ cluster_size: 8192 >> >> Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M >> Supported options: >> -size Virtual disk size >> -compat Compatibility level (0.10 or 1.1) >> -backing_file File name of a base image >> -backing_fmt Image format of the base image >> -encryption Encrypt the image with format 'aes'. (Deprecated in favor >> of encrypt.format=aes) >> -encrypt.format Encrypt the image, format choices: 'aes', 'luks' >> -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase >> -encrypt.cipher-alg Name of encryption cipher algorithm >> -encrypt.cipher-mode Name of encryption cipher mode >> -encrypt.ivgen-alg Name of IV generator algorithm >> -encrypt.ivgen-hash-alg Name of IV generator hash algorithm >> -encrypt.hash-alg Name of encryption hash algorithm >> -encrypt.iter-time Time to spend in PBKDF in milliseconds >> -cluster_size qcow2 cluster size >> -preallocationPreallocation mode (allowed values: off, metadata, falloc, >> full) >> -lazy_refcounts Postpone refcount updates >> -refcount_bitsWidth of a reference count entry in bits >> -nocowTurn off copy-on-write (valid only on btrfs) >> + backing_file: str - File name of a base image >> + backing_fmt: str - Image format of the base image >> + cluster_size: size - qcow2 cluster size >> + compat: str - Compatibility level (0.10 or 1.1) >> + encrypt.cipher-alg: str - Name of encryption cipher algorithm >> + encrypt.cipher-mode: str - Name of encryption cipher mode >> + encrypt.format: str - Encrypt the image, format choices: 'aes', 'luks' >> + encrypt.hash-alg: str - Name of encryption hash algorithm >> + encrypt.iter-time: num - Time to spend in PBKDF in milliseconds >> + encrypt.ivgen-alg: str - Name of IV generator algorithm >> + encrypt.ivgen-hash-alg: str - Name of IV generator hash algorithm >> + encrypt.key-secret: str - ID of secret providing qcow AES key or LUKS >> passphrase >> + encryption: bool (on/off) - Encrypt the image with format 'aes'. >> (Deprecated in favor of encrypt.format=aes) >> + lazy_refcounts: bool (on/off) - Postpone refcount updates >> + nocow: bool (on/off) - Turn off copy-on-write (valid only on btrfs) >> + preallocation: str - Preallocation mode (allowed values: off, metadata, >> falloc, full) >> + refcount_bits: num - Width of a reference count entry in bits >> + size: size - Virtual disk size > > I like the result of this series better than what we have in current > master. Looking at this patch, however, I must also say that I like the > original state better than both. I don't disagree. But having the types is indeed a worthy improvement. > How about aligning the description to the same column again to combine > the best of both worlds? That sounds good to me. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] option: Make option help nicer to read
On 16.10.18 08:46, Marc-André Lureau wrote: > Hi > > On Mon, Oct 15, 2018 at 9:34 PM Max Reitz wrote: >> >> This adds some whitespace into the option help (including indentation) >> and replaces '=' by ': ' (not least because '=' should be used for >> values, not types). > > Without strong preference, I like the '=' better: it's the expected > syntax for the command line argument. (also, --help and man pages > describe options with "foo=type", not "foo: type") Define "strong". I do believe that types are different from values, and that '=' is reserved for values and never used with types. It's not like I'll die when we keep '=', though, but it's definitely not what I'd use. Maybe "foo=", but that feels overly cluttered when compared to "foo: str". (Especially when it comes to things like "iothread=>".) >> Furthermore, the list name is no longer printed as >> part of every line, but only once in advance, and only if the caller did >> not print a caption already. >> >> Signed-off-by: Max Reitz > > looks good to me otherwise Well, if '=' is the only trouble, that's good. :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] iotests: Make nbd-fault-injector flush
On 16.10.18 20:07, Eric Blake wrote:
> On 10/15/18 9:14 AM, Max Reitz wrote:
>> When closing a connection, make the nbd-fault-injector flush the socket.
>> Without this, the output is a bit unreliable with Python 3.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/083.out | 9 +
>> tests/qemu-iotests/nbd-fault-injector.py | 1 +
>> 2 files changed, 10 insertions(+)
>
> I already had a complaint that the error message in 083.out should NOT
> be printing a message
You mean the NBD server itself, right?
> (whether the server is python 2 and auto-flushes,
> or python 3 and needs an explicit flush, the message itself is
> pointless, and the test is racy as a result). We may need to revisit
> this patch when that thread is resolved.
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg01041.html
Well, it's not like the flush hurts either way. :-)
Max
> That said, I'm not opposed to this patch, if it gets iotests to be more
> useful in the meantime.
>
>>
>> diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
>> index be6079d27e..f9af8bb691 100644
>> --- a/tests/qemu-iotests/083.out
>> +++ b/tests/qemu-iotests/083.out
>> @@ -41,6 +41,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
>> === Check disconnect after neg2 ===
>> +Unable to read from socket: Connection reset by peer
>> Connection closed
>> read failed: Input/output error
>> @@ -54,6 +55,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
>> === Check disconnect before request ===
>> +Unable to read from socket: Connection reset by peer
>> Connection closed
>> read failed: Input/output error
>> @@ -116,6 +118,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/
>> === Check disconnect after neg-classic ===
>> +Unable to read from socket: Connection reset by peer
>> Connection closed
>> read failed: Input/output error
>> @@ -161,6 +164,8 @@ can't open device
>> nbd+unix:///foo?socket=TEST_DIR/nbd.sock
>> === Check disconnect after neg2 ===
>> +Unable to read from socket: Connection reset by peer
>> +Connection closed
>> read failed: Input/output error
>> === Check disconnect 8 neg2 ===
>> @@ -173,6 +178,8 @@ can't open device
>> nbd+unix:///foo?socket=TEST_DIR/nbd.sock
>> === Check disconnect before request ===
>> +Unable to read from socket: Connection reset by peer
>> +Connection closed
>> read failed: Input/output error
>> === Check disconnect after request ===
>> @@ -234,6 +241,8 @@ can't open device
>> nbd+unix:///?socket=TEST_DIR/nbd.sock
>> === Check disconnect after neg-classic ===
>> +Unable to read from socket: Connection reset by peer
>> +Connection closed
>> read failed: Input/output error
>> *** done
>> diff --git a/tests/qemu-iotests/nbd-fault-injector.py
>> b/tests/qemu-iotests/nbd-fault-injector.py
>> index f9193c0fae..439a090eb6 100755
>> --- a/tests/qemu-iotests/nbd-fault-injector.py
>> +++ b/tests/qemu-iotests/nbd-fault-injector.py
>> @@ -112,6 +112,7 @@ class FaultInjectionSocket(object):
>> if rule.match(event, io):
>> if rule.when == 0 or bufsize is None:
>> print('Closing connection on rule match %s' %
>> rule.name)
>> + self.sock.flush()
>> sys.exit(0)
>> if rule.when != -1:
>> return rule.when
>>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 7/9] iotests: 'new' module replacement in 169
On 16.10.18 03:01, Cleber Rosa wrote:
>
>
> On 10/15/18 7:57 PM, Eduardo Habkost wrote:
>> On Mon, Oct 15, 2018 at 07:38:45PM -0400, Cleber Rosa wrote:
>>>
>>>
>>> On 10/15/18 10:14 AM, Max Reitz wrote:
iotest 169 uses the 'new' module to add methods to a class. This module
no longer exists in Python 3. Instead, we can use a lambda. Best of
all, this works in 2.7 just as well.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/169 | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index f243db9955..e5614b159d 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -23,7 +23,6 @@ import iotests
import time
import itertools
import operator
-import new
from iotests import qemu_img
@@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
def inject_test_case(klass, name, method, *args, **kwargs):
mc = operator.methodcaller(method, *args, **kwargs)
-setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
+setattr(klass, 'test_' + name, lambda self: mc(self))
for cmb in list(itertools.product((True, False), repeat=4)):
name = ('_' if cmb[0] else '_not_') + 'persistent_'
>>>
>>> This can be simplified with:
>>>
>>> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
>>> index e5614b159d..2199f14ae7 100755
>>> --- a/tests/qemu-iotests/169
>>> +++ b/tests/qemu-iotests/169
>>> @@ -22,7 +22,6 @@ import os
>>> import iotests
>>> import time
>>> import itertools
>>> -import operator
>>> from iotests import qemu_img
>>>
>>>
>>> @@ -141,18 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>>> self.check_bitmap(self.vm_b, sha256 if persistent else False)
>>>
>>>
>>> -def inject_test_case(klass, name, method, *args, **kwargs):
>>> -mc = operator.methodcaller(method, *args, **kwargs)
>>> -setattr(klass, 'test_' + name, lambda self: mc(self))
>>> -
>>> for cmb in list(itertools.product((True, False), repeat=4)):
>>> name = ('_' if cmb[0] else '_not_') + 'persistent_'
>>> name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
>>> name += '_online' if cmb[2] else '_offline'
>>> name += '_shared' if cmb[3] else '_nonshared'
>>>
>>> -inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
>>> - *list(cmb))
>>> +setattr(TestDirtyBitmapMigration, 'test_' + name,
>>> +lambda self: TestDirtyBitmapMigration.do_test_migration(
>>> +self, *list(cmb)))
>>
>> I'm not fond of the long multi-line lambda expression, but I love
>> that you got rid of operator.methodcaller(). :)
>>
>> However, this doesn't seem to work: `*list(cmb)` will be
>> evaluated only when the lambda is called, and `cmb` will be
>> always `(False, False, False, False)`.
>>
>> I was going to suggest defining a nested function inside
>> inject_test_case() to replace operator.methodcaller(), but then I
>> thought it was not worth it.
>>
>
> You're right! I missed that. Anyway, another possibility:
>
> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> index e5614b159d..cd0d9c289c 100755
> --- a/tests/qemu-iotests/169
> +++ b/tests/qemu-iotests/169
> @@ -22,7 +22,7 @@ import os
> import iotests
> import time
> import itertools
> -import operator
> +import functools
> from iotests import qemu_img
>
>
> @@ -140,20 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
> self.vm_b.launch()
> self.check_bitmap(self.vm_b, sha256 if persistent else False)
>
> -
> -def inject_test_case(klass, name, method, *args, **kwargs):
> -mc = operator.methodcaller(method, *args, **kwargs)
> -setattr(klass, 'test_' + name, lambda self: mc(self))
> -
> -for cmb in list(itertools.product((True, False), repeat=4)):
> +for cmb in itertools.product((True, False), repeat=4):
> name = ('_' if cmb[0] else '_not_') + 'persistent_'
> name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
> name += '_online' if cmb[2] else '_offline'
> name += '_shared' if cmb[3] else '_nonshared'
>
> -inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
> - *list(cmb))
> -
> +test =
> functools.partialmethod(TestDirtyBitmapMigration.do_test_migration,
> + *cmb)
> +setattr(TestDirtyBitmapMigration, 'test_' + name, test)
I'm not sure how that is any simpler, though (apart from the inlining). :-)
I mean, my personal goal is not to touch this beyond what I need to
because it's black magic to me anyway, so I'm happy with what works.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 6/9] iotests: Explicitly inherit FDs in Python
On 16.10.18 01:18, Cleber Rosa wrote:
>
>
> On 10/15/18 10:14 AM, Max Reitz wrote:
>> Python 3.2 introduced the inheritable attribute for FDs. At the same
>> time, it changed the default so that all FDs are not inheritable by
>> default, that only inheritable FDs are inherited to subprocesses, and
>> only if close_fds is explicitly set to False.
>>
>
> It's actually a change that was introduced in 3.4:
>
> https://docs.python.org/3/library/os.html#inheritance-of-file-descriptors
Oops, don't know how I got that wrong.
>> Adhere to this by setting close_fds to False when working with
>> subprocesses that may want to inherit FDs, and by trying to
>> set_inheritable() on FDs that we do want to bequeath to them.
>>
>> Signed-off-by: Max Reitz
>> ---
>> scripts/qemu.py| 13 +++--
>> scripts/qmp/qmp.py | 7 +++
>> tests/qemu-iotests/147 | 7 +++
>> 3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index f099ce7278..28366c4a67 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -142,10 +142,18 @@ class QEMUMachine(object):
>> if opts:
>> options.append(opts)
>>
>> +# This did not exist before 3.2, but since then it is
>> +# mandatory for our purpose
>> +try:
>
> Version should be 3.4 here as well.
Yes, will fix everywhere.
>> +os.set_inheritable(fd, True)
>> +except AttributeError:
>> +pass
>> +
>
> Doing hasattr(os, "set_inheritable") is cheaper.
>
> - Cleber.
Nice, I'll use that then.
Max
>> self._args.append('-add-fd')
>> self._args.append(','.join(options))
>> return self
>>
>> +# The caller needs to make sure the FD is inheritable
>> def send_fd_scm(self, fd_file_path):
>> # In iotest.py, the qmp should always use unix socket.
>> assert self._qmp.is_scm_available()
>> @@ -159,7 +167,7 @@ class QEMUMachine(object):
>> "%s" % fd_file_path]
>> devnull = open(os.path.devnull, 'rb')
>> proc = subprocess.Popen(fd_param, stdin=devnull,
>> stdout=subprocess.PIPE,
>> -stderr=subprocess.STDOUT)
>> +stderr=subprocess.STDOUT, close_fds=False)
>> output = proc.communicate()[0]
>> if output:
>> LOG.debug(output)
>> @@ -280,7 +288,8 @@ class QEMUMachine(object):
>> stdin=devnull,
>> stdout=self._qemu_log_file,
>> stderr=subprocess.STDOUT,
>> - shell=False)
>> + shell=False,
>> + close_fds=False)
>> self._post_launch()
>>
>> def wait(self):
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index 5c8cf6a056..009be8345b 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -10,6 +10,7 @@
>>
>> import json
>> import errno
>> +import os
>> import socket
>> import logging
>>
>> @@ -253,4 +254,10 @@ class QEMUMonitorProtocol(object):
>> return self.__sock.fileno()
>>
>> def is_scm_available(self):
>> +# This did not exist before 3.2, but since then it is
>> +# mandatory for our purpose
>> +try:
>> +os.set_inheritable(self.get_sock_fd(), True)
>> +except AttributeError:
>> +pass
>> return self.__sock.family == socket.AF_UNIX
>> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
>> index d2081df84b..b58455645b 100755
>> --- a/tests/qemu-iotests/147
>> +++ b/tests/qemu-iotests/147
>> @@ -229,6 +229,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>> sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
>> sockfd.connect(unix_socket)
>>
>> +# This did not exist before 3.2, but since then it is
>> +# mandatory for our purpose
>> +try:
>> +os.set_inheritable(sockfd.fileno(), True)
>> +except AttributeError:
>> +pass
>> +
>> result = self.vm.send_fd_scm(str(sockfd.fileno()))
>> self.assertEqual(result, 0, 'Failed to send socket FD')
>>
>>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 5/9] iotests: Different iterator behavior in Python 3
On 16.10.18 00:39, Cleber Rosa wrote: > > > On 10/15/18 10:14 AM, Max Reitz wrote: >> In Python 3, several functions now return iterators instead of lists. >> This includes range(), items(), map(), and filter(). This means that if >> we really want a list, we have to wrap those instances with list(). On >> the other hand, sometimes we do just want an iterator, in which case we >> have sometimes used xrange() and iteritems() which no longer exist in >> Python 3. Just change these calls to be range() and items(), which >> costs a bit of performance in Python 2, but will do the right thing in >> Python 3 (which is what is important). >> >> In one instance, we only wanted the first instance of the result of a >> filter() call. Instead of using next(filter()) which would work only in >> Python 3, or list(filter())[0] which would work everywhere but is a bit >> weird, this instance is changed to a single-line for with next() wrapped >> around, which works both in 2.7 and 3. >> >> Signed-off-by: Max Reitz >> --- >> tests/qemu-iotests/044 | 12 ++-- >> tests/qemu-iotests/056 | 2 +- >> tests/qemu-iotests/065 | 4 ++-- >> tests/qemu-iotests/124 | 4 ++-- >> tests/qemu-iotests/139 | 2 +- >> tests/qemu-iotests/163 | 6 +++--- >> 6 files changed, 15 insertions(+), 15 deletions(-) >> > > You have 2 files here which use xrange (which is a manageable size, and > whose occurrences involve a moderate size of items) to also consider: > > if sys.version_info.major == 2: >range = xrange I don't think it's necessary, but it's so easy to grep for "sys.version_info" once we want to completely switch to Python 3 that I can't find good arguments against it. O:-) Max > Defaulting to the Python 3 names, but behaving the same across Python 2 > and 3. > > To do the same for dict.iteritems() => dict.items() requires a lot more > code, so I'd stay away from it for now. Also, it looks like most of > those dicts are small in size (**kwargs and the like). > > Other than that suggestion, > > Reviewed-by: Cleber Rosa > signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 9/9] iotests: Unify log outputs between Python 2 and 3
On 16.10.18 00:26, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:53PM +0200, Max Reitz wrote:
>> When dumping an object into the log, there are differences between
>> Python 2 and 3. First, unicode strings are prefixed by 'u' in Python 2
>> (they are no longer in 3, because unicode strings are the default
>> there). [...]
>
> This could be addressed using JSON. See below[1].
>
>> [...] Second, the order of keys in dicts may differ. [...]
>
> Can be addressed using json.dumps(..., sort_keys=True).
Ah. Nice. :-)
>> [...] Third,
>> especially long numbers are longs in Python 2 and thus get an 'L'
>> suffix, which does not happen in Python 3.
>
> print() doesn't add a L suffix on Python 2.7 on my system:
>
> Python 2.7.15 (default, May 15 2018, 15:37:31)
> [GCC 7.3.1 20180303 (Red Hat 7.3.1-5)] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
print(999L)
> 999
>
> So I assume this happens only for QMP commands.
It happens for dicts:
>>> print L
>>> print {'foo':L}
{'foo': L}
> It would be addressed if using JSON, too.
OK.
>> To get around these differences, this patch introduces functions to
>> convert an object to a string that looks the same regardless of the
>> Python version: In Python 2, they decode unicode strings to byte strings
>> (normal str); in Python 3, they encode byte strings to unicode strings
>> (normal str). They also manually convert lists and dicts to strings,
>> which allows sorting the dicts by key, so we are no longer at the mercy
>> of the internal implementation when it comes to how the keys appear in
>> the output.
>>
>> This changes the output of all tests that use these logging functions.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/194.out| 22 +-
>> tests/qemu-iotests/202.out| 12 +-
>> tests/qemu-iotests/203.out| 14 +-
>> tests/qemu-iotests/206.out| 144 +-
>> tests/qemu-iotests/207.out| 52 ++--
>> tests/qemu-iotests/208.out| 8 +-
>> tests/qemu-iotests/210.out| 72 ++---
>> tests/qemu-iotests/211.out| 66 ++---
>> tests/qemu-iotests/212.out| 102 +++
>> tests/qemu-iotests/213.out| 124
>> tests/qemu-iotests/216.out| 4 +-
>> tests/qemu-iotests/218.out| 20 +-
>> tests/qemu-iotests/219.out| 526 +-
>> tests/qemu-iotests/222.out| 24 +-
>> tests/qemu-iotests/iotests.py | 42 ++-
>> 15 files changed, 634 insertions(+), 598 deletions(-)
> [...]
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index a64ea90fb4..f8dbc8cc71 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -250,10 +250,45 @@ def filter_img_info(output, filename):
>> lines.append(line)
>> return '\n'.join(lines)
>>
>> +def log_to_string_repr(obj):
>> +# Normal Python 3 strings are the unicode strings from Python 2;
>> +# and normal Python 2 strings are byte strings in Python 3. Thus,
>> +# we convert bytes -> str in py3 and unicode -> str in py2.
>> +if sys.version_info.major >= 3:
>> +if type(obj) is bytes:
>> +return repr(obj.decode('utf-8'))
>> +else:
>> +if type(obj) is unicode:
>> +return repr(obj.encode('utf-8'))
>> +elif type(obj) is long:
>> +return str(obj) # repr() would include an 'L' suffix
>> +
>> +if type(obj) is list:
>> +return '[' + ', '.join(map(log_to_string_repr, obj)) + ']'
>> +elif type(obj) is dict:
>> +return '{' + ', '.join(map(lambda k: log_to_string_repr(k) + ': ' +
>> + log_to_string_repr(obj[k]),
>> + sorted(obj.keys( + '}'
>> +else:
>> +return repr(obj)
>
> I assume this function exists only because of QMP logging,
> correct?
In practice probably yes.
> I would just use json.dumps() at qmp_log(), see below[1].
However, there is not just qmp_log(), there are places that dump objects
directly into log().
>> +
>> +def log_to_string(obj):
>> +if type(obj) is str:
>> +return obj
>> +
>> +if sys.version_info.major >= 3:
>> +if type(obj) is bytes:
>> +return obj.decode('utf-8')
>
> Do you know how many of existing log() calls actually pass a byte
> string as argument?
Frankly I hope none.
>> +else:
>> +if type(obj) is unicode:
>> +return obj.encode('utf-8')
>
> Is this really necessary? The existing code just calls
> print(msg) and it works, doesn't it?
True. But the main issue is that we still need to return immediately
for byte strings and Unicode strings, but the type "bytes" only exists
in 3.x and "un
Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3
On 16.10.18 02:12, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 08:05:02PM -0400, Cleber Rosa wrote: >> >> >> On 10/15/18 5:17 PM, Eduardo Habkost wrote: >>> On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote: There are two imports that need to be modified when running the iotests under Python 3: One is StringIO, which no longer exists; instead, the StringIO class comes from the io module, so import it from there. The other is the ConfigParser, which has just been renamed to configparser. Signed-off-by: Max Reitz --- tests/qemu-iotests/iotests.py| 8 ++-- tests/qemu-iotests/nbd-fault-injector.py | 7 +-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 7ca94e9278..a64ea90fb4 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], # We need to filter out the time taken from the output so that qemu-iotest # can reliably diff the results against master output. -import StringIO +if sys.version_info.major >= 3: +from io import StringIO +else: +from StringIO import StringIO >>> >>> Considering that io.StringIO exists on Python 2.7, a comment >>> explaining why exactly it doesn't work would be nice. Oh, it does exist? I didn't know. O:-) So I suppose it's because the test runner emits just normal strings, which in 2.x are byte strings; but io's StringIO always expects Unicode strings. StringIO, OTOH, accepts both (and returns Unicode strings once you put a Unicode string into it). >> Another possibility, that I find self explanatory: >> >> import io >> >> if sys.version_info.major >= 3: >>output = io.StringIO() >> else: >>output = io.BytesIO() > > Looks nice and clean. > > But I'm not sure all output sent to `output` when running with > Python 2 will be byte strings. What if `unittest.TextTestRunner` > tries to write unicode strings to `output`? Hm. It doesn't look this way to me in practice. And considering that it only really prints a test summary, I don't think there are edge cases where it behaves differently. So I think using BytesIO() in 2.x is better. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 0/9] iotests: Make them work for both Python 2 and 3
On 16.10.18 00:19, Philippe Mathieu-Daudé wrote: > Hi Max, > > On 15/10/2018 16:14, Max Reitz wrote: >> This series prepares the iotests to work with both Python 2 and 3. In >> some places, it adds version-specific code and decides what to do based >> on the version (for instance, whether to import the StringIO class from >> the 'io' or the 'StringIO' module), but most of the time, it just makes >> code work for both versions in general. >> >> And when we make the switch to make Python 3 mandatory, we can simply >> drop the Python 2 branches. >> >> >> Max Reitz (9): >> iotests: Make nbd-fault-injector flush >> iotests: Flush in iotests.py's QemuIoInteractive >> iotests: Use Python byte strings where appropriate >> iotests: Use // for Python integer division >> iotests: Different iterator behavior in Python 3 >> iotests: Explicitly inherit FDs in Python >> iotests: 'new' module replacement in 169 >> iotests: Modify imports for Python 3 >> iotests: Unify log outputs between Python 2 and 3 > > You forgot: > > MAINTAINERS: Add myself in the Python scripts section > > ;) Aw, please, no. You do know I don't really know Python? ;-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] iotests: Use // for Python integer division
On 15.10.18 23:13, Cleber Rosa wrote:
>
>
> On 10/15/18 10:14 AM, Max Reitz wrote:
>> In Python 3, / is always a floating-point division. We usually do not
>> want this, and as Python 2.7 understands // as well, change all integer
>> divisions to use that.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/040| 4 ++--
>> tests/qemu-iotests/044| 2 +-
>> tests/qemu-iotests/093| 18 +-
>> tests/qemu-iotests/149| 6 +++---
>> tests/qemu-iotests/151| 12 ++--
>> tests/qemu-iotests/163| 2 +-
>> tests/qemu-iotests/iotests.py | 2 +-
>> 7 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 1cb1ceeb33..b81133a474 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040
>> @@ -195,7 +195,7 @@ class TestSingleDrive(ImageCommitTestCase):
>>
>> self.assert_no_active_block_jobs()
>> result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
>> - base=backing_img, speed=(self.image_len / 4))
>> + base=backing_img, speed=(self.image_len // 4))
>> self.assert_qmp(result, 'return', {})
>> result = self.vm.qmp('device_del', id='scsi0')
>> self.assert_qmp(result, 'return', {})
>> @@ -225,7 +225,7 @@ class TestSingleDrive(ImageCommitTestCase):
>>
>> self.assert_no_active_block_jobs()
>> result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
>> - base=backing_img, speed=(self.image_len / 4))
>> + base=backing_img, speed=(self.image_len // 4))
>> self.assert_qmp(result, 'return', {})
>>
>> result = self.vm.qmp('query-block')
>> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
>> index 69e736f687..7ef5e46fe9 100755
>> --- a/tests/qemu-iotests/044
>> +++ b/tests/qemu-iotests/044
>> @@ -86,7 +86,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>> off = off + 1024 * 512
>>
>> table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
>> -for j in xrange(0, remaining / 512))
>> +for j in xrange(0, remaining // 512))
>> fd.write(table)
>>
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index 9d1971a56c..d88fbc182e 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -69,18 +69,18 @@ class ThrottleTestCase(iotests.QMPTestCase):
>> # in. The throttled requests won't be executed until we
>> # advance the virtual clock.
>> rq_size = 512
>> -rd_nr = max(params['bps'] / rq_size / 2,
>> -params['bps_rd'] / rq_size,
>> -params['iops'] / 2,
>> +rd_nr = max(params['bps'] // rq_size // 2,
>> +params['bps_rd'] // rq_size,
>> +params['iops'] // 2,
>> params['iops_rd'])
>> rd_nr *= seconds * 2
>> -rd_nr /= ndrives
>> -wr_nr = max(params['bps'] / rq_size / 2,
>> -params['bps_wr'] / rq_size,
>> -params['iops'] / 2,
>> +rd_nr //= ndrives
>> +wr_nr = max(params['bps'] // rq_size // 2,
>> +params['bps_wr'] // rq_size,
>> +params['iops'] // 2,
>> params['iops_wr'])
>> wr_nr *= seconds * 2
>> -wr_nr /= ndrives
>> +wr_nr //= ndrives
>>
>> # Send I/O requests to all drives
>> for i in range(rd_nr):
>> @@ -196,7 +196,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
>> self.configure_throttle(ndrives, settings)
>>
>> # Wait for the bucket to empty so we can do bursts
>> -wait_ns = nsec_per_sec * burst_length * burst_rate / rate
>> +wait_ns = nsec_per_sec * burst_length * burst_rate // rate
>> self.vm.qtest("clock_step %d" % wait_ns)
>>
>> # Test I/O at the max burst rate
>> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
>> index 1225334cb8..4f363f295f 100755
>> --- a/tests/qemu-iotests/149
>> +++ b/tests/qemu-iotests/149
>> @@ -314,13 +314,13 @@ def test_once(config, qemu_img=False):
>> image_size = 4 * oneTB
>> if qemu_img:
>> iotests.log("# Create image")
>> -qemu_img_create(config, image_size / oneMB)
>> +qemu_img_create(config, image_size // oneMB)
>> else:
>> iotests.log("# Create image")
>> -create_image(config, image_size / oneMB)
>> +create_image(config, image_size // oneMB)
>>
>> lowOffsetMB = 100
>> -highOffsetMB = 3 * oneTB / oneMB
>> +highOffsetMB = 3 * oneTB // oneMB
>>
>> try:
>> if not qemu_img:
>> diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
>> index fe53b9f446..1bb74d67c4 100
Re: [Qemu-block] [PATCH 6/9] iotests: Explicitly inherit FDs in Python
On 15.10.18 22:30, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:50PM +0200, Max Reitz wrote:
>> Python 3.2 introduced the inheritable attribute for FDs. At the same
>> time, it changed the default so that all FDs are not inheritable by
>> default, that only inheritable FDs are inherited to subprocesses, and
>> only if close_fds is explicitly set to False.
>>
>> Adhere to this by setting close_fds to False when working with
>> subprocesses that may want to inherit FDs, and by trying to
>> set_inheritable() on FDs that we do want to bequeath to them.
>>
>> Signed-off-by: Max Reitz
>> ---
>> scripts/qemu.py| 13 +++--
>> scripts/qmp/qmp.py | 7 +++
>> tests/qemu-iotests/147 | 7 +++
>> 3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index f099ce7278..28366c4a67 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -142,10 +142,18 @@ class QEMUMachine(object):
>> if opts:
>> options.append(opts)
>>
>> +# This did not exist before 3.2, but since then it is
>> +# mandatory for our purpose
>> +try:
>> +os.set_inheritable(fd, True)
>> +except AttributeError:
>> +pass
>> +
>
> This is add_fd(), so calling set_inheritable() automatically here
> makes sense.
>
>> self._args.append('-add-fd')
>> self._args.append(','.join(options))
>> return self
>>
>> +# The caller needs to make sure the FD is inheritable
>> def send_fd_scm(self, fd_file_path):
>> # In iotest.py, the qmp should always use unix socket.
>> assert self._qmp.is_scm_available()
>> @@ -159,7 +167,7 @@ class QEMUMachine(object):
>> "%s" % fd_file_path]
>> devnull = open(os.path.devnull, 'rb')
>> proc = subprocess.Popen(fd_param, stdin=devnull,
>> stdout=subprocess.PIPE,
>> -stderr=subprocess.STDOUT)
>> +stderr=subprocess.STDOUT, close_fds=False)
>> output = proc.communicate()[0]
>> if output:
>> LOG.debug(output)
>> @@ -280,7 +288,8 @@ class QEMUMachine(object):
>> stdin=devnull,
>> stdout=self._qemu_log_file,
>> stderr=subprocess.STDOUT,
>> - shell=False)
>> + shell=False,
>> + close_fds=False)
>> self._post_launch()
>>
>> def wait(self):
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index 5c8cf6a056..009be8345b 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -10,6 +10,7 @@
>>
>> import json
>> import errno
>> +import os
>> import socket
>> import logging
>>
>> @@ -253,4 +254,10 @@ class QEMUMonitorProtocol(object):
>> return self.__sock.fileno()
>>
>> def is_scm_available(self):
>> +# This did not exist before 3.2, but since then it is
>> +# mandatory for our purpose
>> +try:
>> +os.set_inheritable(self.get_sock_fd(), True)
>> +except AttributeError:
>> +pass
>
> Why did you decide to place this code inside is_scm_available()?
>
> For reference, this is the only caller of is_scm_available():
>
> def send_fd_scm(self, fd_file_path):
> # In iotest.py, the qmp should always use unix socket.
> assert self._qmp.is_scm_available()
> ...
>
> In addition to making a method called is_*() have an unexpected
> side-effect,
True. My idea was that a function that asks for SCM to be available
might as well make it available.
> the method won't be called at all if running with
> debugging disabled.
Well, I sure hope we don't disable debugging in the iotests. We use
assert quite a number of times there.
On the other hand, someone might want to use this outside of the iotests
but I don't even know whether that works, considering the SCM helper
program is part of the iotests.
> I suggest simply placing the os.set_inheritable() call inside
> send_fd_scm(), as close as possible to the subprocess.Popen()
> call.
Yes, I think you're right. I didn't want to put it into send_fd_scm(),
because that method is only there to send some FD over QMP/SCM; it isn't
really supposed to send the QMP socket FD somewhere. But sending
something over QMP/SCM is different from bequeathing something to a
child process (the socket_scm_helper). And since send_fd_scm() needs to
bequeath the QMP socket FD to that helper, it should be responsible for
making it inheritable.
And maybe even more importantly, whether the socket allows for SCM
really has nothing to do with whether it's inheritable. So it's
actually just wrong to put it here.
>> return self.__sock.family == socket.AF_UNIX
>> diff --git a/tes
Re: [Qemu-block] [PATCH 5/9] iotests: Different iterator behavior in Python 3
On 15.10.18 22:07, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:49PM +0200, Max Reitz wrote:
>> In Python 3, several functions now return iterators instead of lists.
>> This includes range(), items(), map(), and filter(). This means that if
>> we really want a list, we have to wrap those instances with list(). On
>> the other hand, sometimes we do just want an iterator, in which case we
>> have sometimes used xrange() and iteritems() which no longer exist in
>> Python 3. Just change these calls to be range() and items(), which
>> costs a bit of performance in Python 2, but will do the right thing in
>> Python 3 (which is what is important).
>>
>> In one instance, we only wanted the first instance of the result of a
>> filter() call. Instead of using next(filter()) which would work only in
>> Python 3, or list(filter())[0] which would work everywhere but is a bit
>> weird, this instance is changed to a single-line for with next() wrapped
>> around, which works both in 2.7 and 3.
>>
>> Signed-off-by: Max Reitz
>
> Reviewed-by: Eduardo Habkost
>
> Minor comments below:
[...]
>> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
>> index 72aa9707c7..a339bf6069 100755
>> --- a/tests/qemu-iotests/065
>> +++ b/tests/qemu-iotests/065
>> @@ -59,7 +59,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
>> :data.index('')]
>> for field in data:
>> self.assertTrue(re.match('^ {4}[^ ]', field) is not None)
>> -data = map(lambda line: line.strip(), data)
>> +data = list(map(lambda line: line.strip(), data))
>
> I would find this more readable:
>
> data = [line.strip() for line in data]
>
> Not a blocker, though.
Yes, I agree, that looks better.
>> self.assertEqual(data, self.human_compare)
>>
>> class TestQMP(TestImageInfoSpecific):
>> @@ -80,7 +80,7 @@ class TestQMP(TestImageInfoSpecific):
>>
>> def test_qmp(self):
>> result = self.vm.qmp('query-block')['return']
>> -drive = filter(lambda drive: drive['device'] == 'drive0', result)[0]
>> +drive = next(drive for drive in result if drive['device'] ==
>> 'drive0')
>
> I didn't understand what you meant by "single-line for" on the
> commit message, until I saw the generator expression here. :)
Maybe I should at least put quotes around the "for", because the
combination of "for with" really makes it difficult to read...
> This will raise an exception if there's no drive0 in the list,
> but that was already true on the original code.
>
>
>> data = drive['inserted']['image']['format-specific']
>> self.assertEqual(data['type'], iotests.imgfmt)
>> self.assertEqual(data['data'], self.compare)
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 3ea4ac53f5..9f189e3b54 100755
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -39,7 +39,7 @@ def try_remove(img):
>> def transaction_action(action, **kwargs):
>> return {
>> 'type': action,
>> -'data': dict((k.replace('_', '-'), v) for k, v in
>> kwargs.iteritems())
>> +'data': dict((k.replace('_', '-'), v) for k, v in kwargs.items())
>> }
>>
>>
>> @@ -134,7 +134,7 @@ class TestIncrementalBackupBase(iotests.QMPTestCase):
>> def img_create(self, img, fmt=iotests.imgfmt, size='64M',
>> parent=None, parentFormat=None, **kwargs):
>> optargs = []
>> -for k,v in kwargs.iteritems():
>> +for k,v in kwargs.items():
>> optargs = optargs + ['-o', '%s=%s' % (k,v)]
>> args = ['create', '-f', fmt] + optargs + [img, size]
>> if parent:
>> diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
>> index cc7fe337f3..e00f10b8c8 100755
>> --- a/tests/qemu-iotests/139
>> +++ b/tests/qemu-iotests/139
>> @@ -51,7 +51,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
>> # Check whether a BlockDriverState exists
>> def checkBlockDriverState(self, node, must_exist = True):
>> result = self.vm.qmp('query-named-block-nodes')
>> -nodes = filter(lambda x: x['node-name'] == node, result['return'])
>> +nodes = list(filter(lambda x: x['node-name'] == node,
>> result['return']))
>
> I'd prefer a list expression:
>
> nodes = [x for x in result['return'] if x['node-name'] == node]
>
> Also not a blocker.
I don't mind either way, so why not.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 3/9] iotests: Use Python byte strings where appropriate
On 15.10.18 21:53, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:47PM +0200, Max Reitz wrote:
>> Since byte strings are no longer the default in Python 3, we have to
>> explicitly use them where we need to, which is mostly when working with
>> structures. It also means that we need to open a file in binary mode
>> when we want to use structures.
>>
>> On the other hand, we have to accomodate for the fact that some
>> functions (still) work with byte strings but we want to use unicode
>> strings (in Python 3 at least, and it does not matter in Python 2).
>> This includes base64 encoding, but it is most notable when working with
>> the subprocess module: Either we set univeral_newlines to True so that
>> the default streams are opened in text mode (hence this parameter is
>> aliased as "text" as of 3.7), or, if that is not possible, we have to
>> decode the output to a normal string.
>>
>> Signed-off-by: Max Reitz
> [...]
>> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
>> index 9e0cad76f9..1225334cb8 100755
>> --- a/tests/qemu-iotests/149
>> +++ b/tests/qemu-iotests/149
>> @@ -79,7 +79,7 @@ class LUKSConfig(object):
>>
>> def first_password_base64(self):
>> (pw, slot) = self.first_password()
>> -return base64.b64encode(pw)
>> +return base64.b64encode(pw.encode('ascii')).decode('ascii')
>
> Would we want to have a test case for non-ascii passwords in the
> future? In that case you would probably need to make
> self.passwords[] contain byte strings instead of text.
I remember someone once providing a non-ASCII initial password to some
system for me. I remember the system was running Windows, and I was
running Linux, so the system expected ISO-8859-1, while I was sending UTF-8.
The moral of the story is that you probably don't want non-ASCII
passwords. And if we do want to test them, well, we'll need to decide
on an encoding then (or use byte strings, as you suggest).
> In either case, using 'ascii' as the encoding everywhere will
> ensure the code will not try to be too smart about string
> encodings if that happens. I like that.
>
> Reviewed-by: Eduardo Habkost
Thanks for reviewing!
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3
On 15.10.18 20:59, Cleber Rosa wrote:
>
>
> On 10/15/18 10:14 AM, Max Reitz wrote:
>> There are two imports that need to be modified when running the iotests
>> under Python 3: One is StringIO, which no longer exists; instead, the
>> StringIO class comes from the io module, so import it from there. The
>> other is the ConfigParser, which has just been renamed to configparser.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/iotests.py| 8 ++--
>> tests/qemu-iotests/nbd-fault-injector.py | 7 +--
>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7ca94e9278..a64ea90fb4 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'],
>> supported_cache_modes=[],
>>
>> # We need to filter out the time taken from the output so that
>> qemu-iotest
>> # can reliably diff the results against master output.
>> -import StringIO
>> +if sys.version_info.major >= 3:
>> +from io import StringIO
>> +else:
>> +from StringIO import StringIO
>> +
>> if debug:
>> output = sys.stdout
>> verbosity = 2
>> sys.argv.remove('-d')
>> else:
>> -output = StringIO.StringIO()
>> +output = StringIO()
>>
>> logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>>
>> diff --git a/tests/qemu-iotests/nbd-fault-injector.py
>> b/tests/qemu-iotests/nbd-fault-injector.py
>> index d45e2e0a6a..6b2d659dee 100755
>> --- a/tests/qemu-iotests/nbd-fault-injector.py
>> +++ b/tests/qemu-iotests/nbd-fault-injector.py
>> @@ -48,7 +48,10 @@ import sys
>> import socket
>> import struct
>> import collections
>> -import ConfigParser
>> +if sys.version_info.major >= 3:
>> +import configparser
>> +else:
>> +import ConfigParser as configparser
>>
>> FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
>>
>> @@ -225,7 +228,7 @@ def parse_config(config):
>> return rules
>>
>> def load_rules(filename):
>> -config = ConfigParser.RawConfigParser()
>> +config = configparser.RawConfigParser()
>> with open(filename, 'rt') as f:
>> config.readfp(f, filename)
>> return parse_config(config)
>>
>
> This may be a type of culture clash (on my side, due to not enough QEMU
> culture), but shouldn't this be applied before anything else on this series?
>
> I mean, PATCH 1/9 is supposed to fix the reliability aspects of
> nbd-fault-injector under Python 3, but without this patch, it won't
> actually run on Python 3.
I don't mind, I followed no specific order. Well, patch 9 is patch 9
because it is the largest one, so I didn't want to discourage people
before the end of the series. :-)
Max
signature.asc
Description: OpenPGP digital signature
