The following changes since commit 64f0ad8ad8e13257e7c912df470d46784b55c3fd:

Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2020-07-02' into staging (2020-07-02 15:54:09 +0100)

are available in the Git repository at:

for you to fetch changes up to 4f071a9460886667fde061c05b79dc786cc22e3c:

iotests: Fix 051 output after qdev_init_nofail() removal (2020-07-03 10:06:29 +0200)

- qemu-img convert: Don't pre-zero images (removes nowadays

counterproductive optimisation)

- qemu-storage-daemon: Fix object-del, cleaner shutdown

- vvfat: Check that the guest doesn't escape the given host directory

with read-write vvfat drives

- vvfat: Fix crash by out-of-bounds array writes for read-write drives

- iotests fixes

Kevin Wolf (3):

qemu-img convert: Don't pre-zero images

vvfat: Check that updated filenames are valid

vvfat: Fix array_remove_slice()

Max Reitz (1):

iotests.py: Do not wait() before communicate()

Philippe Mathieu-Daudé (1):

iotests: Fix 051 output after qdev_init_nofail() removal

Stefan Hajnoczi (2):

qemu-storage-daemon: remember to add qemu_object_opts

qemu-storage-daemon: add missing cleanup calls

Since commit 5a37b60a61c, qemu-img create will pre-zero the target image

if it isn't already zero-initialised (most importantly, for host block

devices, but also iscsi etc.), so that writing explicit zeros wouldn't

be necessary later.

This could speed up the operation significantly, in particular when the

source image file was only sparsely populated. However, it also means

that some block are written twice: Once when pre-zeroing them, and then

when they are overwritten with actual data. On a full image, the

pre-zeroing is wasted work because everything will be overwritten.

In practice, write_zeroes typically turns out faster than writing

explicit zero buffers, but slow enough that first zeroing everything and

then overwriting parts can be a significant net loss.

Meanwhile, qemu-img convert was rewritten in 690c7301600 and zero blocks

are now written to the target using bdrv_co_pwrite_zeroes() if the

target could be pre-zeroed. This way we already make use of the faster

write_zeroes operation, but avoid writing any blocks twice.

Remove the pre-zeroing because these days this former optimisation has

actually turned into a pessimisation in the common case.

Reported-by: Nir Soffer <nsoffer@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200622151203.35624-1-kwolf@redhat.com>

Tested-by: Nir Soffer <nsoffer@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

qemu-img.c | 9 ---------

1 file changed, 9 deletions(-)

@@ -XXX,XX +XXX,XX @@ static int convert_do_copy(ImgConvertState *s)

s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));

- if (!s->has_zero_init && !s->target_has_backing &&

- bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))

- {

- ret = blk_make_zero(s->target, BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK);

- if (ret == 0) {

- s->has_zero_init = true;

- }

/* Allocate buffer for copied data. For compressed images, only one cluster

* can be copied at a time. */

if (s->compressed) {

2.25.4

FAT allows only a restricted set of characters in file names, and for

some of the illegal characters, it's actually important that we catch

them: If filenames can contain '/', the guest can construct filenames

containing "../" and escape from the assigned vvfat directory. The same

problem could arise if ".." was ever accepted as a literal filename.

Fix this by adding a check that all filenames are valid in

check_directory_consistency().

Reported-by: Nathan Huckleberry <nhuck15@gmail.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200623175534.38286-2-kwolf@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

block/vvfat.c | 25 ++++++++++++++++++++++++-

1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c

--- a/block/vvfat.c

+++ b/block/vvfat.c

@@ -XXX,XX +XXX,XX @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin)

direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff);

+static bool valid_filename(const unsigned char *name)

+ unsigned char c;

+ if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) {

+ return false;

+ }

+ for (; (c = *name); name++) {

+ if (!((c >= '0' && c <= '9') ||

+ (c >= 'A' && c <= 'Z') ||

+ (c >= 'a' && c <= 'z') ||

+ c > 127 ||

+ strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != NULL))

+ {

+ return false;

+ }

+ }

+ return true;

static uint8_t to_valid_short_char(gunichar c)

{

c = g_unichar_toupper(c);

if ((c >= '0' && c <= '9') ||

(c >= 'A' && c <= 'Z') ||

- strchr("$%'-_@~`!(){}^#&", c) != 0) {

+ strchr("$%'-_@~`!(){}^#&", c) != NULL) {

return c;

} else {

return 0;

@@ -XXX,XX +XXX,XX @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))

}

lfn.checksum = 0x100; /* cannot use long name twice */

+ if (!valid_filename(lfn.name)) {

+ fprintf(stderr, "Invalid file name\n");

+ goto fail;

+ }

if (path_len + 1 + lfn.len >= PATH_MAX) {

fprintf(stderr, "Name too long: %s/%s\n", path, lfn.name);

goto fail;

2.25.4

array_remove_slice() calls array_roll() with array->next - 1 as the

destination index. This is only correct for count == 1, otherwise we're

writing past the end of the array. array->next - count would be correct.

However, this is the only place ever calling array_roll(), so this

rather complicated operation isn't even necessary.

Fix the problem and simplify the code by replacing it with a single

memmove() call. array_roll() can now be removed.

Reported-by: Nathan Huckleberry <nhuck15@gmail.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200623175534.38286-3-kwolf@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

block/vvfat.c | 42 +++++-------------------------------------

1 file changed, 5 insertions(+), 37 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c

--- a/block/vvfat.c

+++ b/block/vvfat.c

@@ -XXX,XX +XXX,XX @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int

return array->pointer+index*array->item_size;

}

-/* this performs a "roll", so that the element which was at index_from becomes

- * index_to, but the order of all other elements is preserved. */

-static inline int array_roll(array_t* array,int index_to,int index_from,int count)

-{

- char* buf;

- char* from;

- char* to;

- int is;

-

- if(!array ||

- index_to<0 || index_to>=array->next ||

- index_from<0 || index_from>=array->next)

- return -1;

-

- if(index_to==index_from)

- return 0;

-

- is=array->item_size;

- from=array->pointer+index_from*is;

- to=array->pointer+index_to*is;

- buf=g_malloc(is*count);

- memcpy(buf,from,is*count);

-

- if(index_to<index_from)

- memmove(to+is*count,to,from-to);

- else

- memmove(from,from+is*count,to-from);

-

- memcpy(to,buf,is*count);

-

- g_free(buf);

-

- return 0;

-}

-

static inline int array_remove_slice(array_t* array,int index, int count)

{

assert(index >=0);

assert(count > 0);

assert(index + count <= array->next);

- if(array_roll(array,array->next-1,index,count))

- return -1;

+ memmove(array->pointer + index * array->item_size,

+ array->pointer + (index + count) * array->item_size,

+ (array->next - index - count) * array->item_size);

array->next -= count;

2.25.4

From: Max Reitz <mreitz@redhat.com>

Waiting on a process for which we have a pipe will stall if the process

outputs more data than fits into the OS-provided buffer. We must use

communicate() before wait(), and in fact, communicate() perfectly

replaces wait() already.

We have to drop the stderr=subprocess.STDOUT parameter from

subprocess.Popen() in qemu_nbd_early_pipe(), because stderr is passed on

to the child process, so if we do not drop this parameter, communicate()

will hang (because the pipe is not closed).

Signed-off-by: Max Reitz <mreitz@redhat.com>

Message-Id: <20200630083711.40567-1-mreitz@redhat.com>

tests/qemu-iotests/iotests.py | 34 +++++++++++++++++-----------------

1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py

--- a/tests/qemu-iotests/iotests.py

+++ b/tests/qemu-iotests/iotests.py

@@ -XXX,XX +XXX,XX @@ def qemu_img_pipe(*args):

stdout=subprocess.PIPE,

stderr=subprocess.STDOUT,

universal_newlines=True)

- exitcode = subp.wait()

- if exitcode < 0:

+ output = subp.communicate()[0]

+ if subp.returncode < 0:

sys.stderr.write('qemu-img received signal %i: %s\n'

- % (-exitcode, ' '.join(qemu_img_args + list(args))))

- return subp.communicate()[0]

+ % (-subp.returncode,

+ ' '.join(qemu_img_args + list(args))))

+ return output

def qemu_img_log(*args):

result = qemu_img_pipe(*args)

@@ -XXX,XX +XXX,XX @@ def qemu_io(*args):

subp = subprocess.Popen(args, stdout=subprocess.PIPE,

stderr=subprocess.STDOUT,

universal_newlines=True)

- exitcode = subp.wait()

- if exitcode < 0:

+ output = subp.communicate()[0]

+ if subp.returncode < 0:

sys.stderr.write('qemu-io received signal %i: %s\n'

- % (-exitcode, ' '.join(args)))

- return subp.communicate()[0]

+ % (-subp.returncode, ' '.join(args)))

+ return output

def qemu_io_log(*args):

result = qemu_io(*args)

@@ -XXX,XX +XXX,XX @@ def qemu_nbd_early_pipe(*args):

and its output in case of an error'''

subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),

stdout=subprocess.PIPE,

- stderr=subprocess.STDOUT,

universal_newlines=True)

- exitcode = subp.wait()

- if exitcode < 0:

+ output = subp.communicate()[0]

+ if subp.returncode < 0:

sys.stderr.write('qemu-nbd received signal %i: %s\n' %

- (-exitcode,

+ (-subp.returncode,

' '.join(qemu_nbd_args + ['--fork'] + list(args))))

- return exitcode, subp.communicate()[0] if exitcode else ''

+ return subp.returncode, output if subp.returncode else ''

def qemu_nbd_popen(*args):

'''Run qemu-nbd in daemon mode and return the parent's exit code'''

@@ -XXX,XX +XXX,XX @@ def qemu_pipe(*args):

subp = subprocess.Popen(args, stdout=subprocess.PIPE,

stderr=subprocess.STDOUT,

universal_newlines=True)

- exitcode = subp.wait()

- if exitcode < 0:

+ output = subp.communicate()[0]

+ if subp.returncode < 0:

sys.stderr.write('qemu received signal %i: %s\n' %

- (-exitcode, ' '.join(args)))

- return subp.communicate()[0]

+ (-subp.returncode, ' '.join(args)))

+ return output

def supported_formats(read_only=False):

'''Set 'read_only' to True to check ro-whitelist

2.25.4

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Commit 96927c744 replaced qdev_init_nofail() call by

isa_realize_and_unref() which has a different error

message. Update the test output accordingly.

Gitlab CI error after merging b77b5b3dc7:

https://gitlab.com/qemu-project/qemu/-/jobs/597414772#L4375

Reported-by: Thomas Huth <thuth@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: John Snow <jsnow@redhat.com>

Reviewed-by: Thomas Huth <thuth@redhat.com>

Message-Id: <20200616154949.6586-1-philmd@redhat.com>

Message-Id: <20200624140446.15380-2-alex.bennee@linaro.org>

tests/qemu-iotests/051.pc.out | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out

--- a/tests/qemu-iotests/051.pc.out

+++ b/tests/qemu-iotests/051.pc.out

@@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive if=ide

QEMU X.Y.Z monitor - type 'help' for more information

-(qemu) QEMU_PROG: Initialization of device ide-hd failed: Device needs media, but drive is empty

+(qemu) QEMU_PROG: Device needs media, but drive is empty

Testing: -drive if=virtio

QEMU X.Y.Z monitor - type 'help' for more information

@@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on

QEMU X.Y.Z monitor - type 'help' for more information

-(qemu) QEMU_PROG: Initialization of device ide-hd failed: Block node is read-only

+(qemu) QEMU_PROG: Block node is read-only

Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on

QEMU X.Y.Z monitor - type 'help' for more information

2.25.4