Re: [Qemu-block] [Qemu-devel] [PATCH] block: Make more block drivers compile-time configurable
Max Reitz writes: > On 05.11.18 16:25, Markus Armbruster wrote: >> Max Reitz writes: >> >>> On 19.10.18 13:34, Markus Armbruster wrote: 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 >>> >>> [...] >>> @@ -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) >>> >>> This defines "block-obj-dmg-bz2m" or "block-obj-dmg-bz2n", so >>> "block-obj-dmg-bz2-y" is never defined (note both the missing hyphen and >>> the "m" vs. "y"). >>> >>> How about: >>> >>> block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o >> >> As far as I can tell, CONFIG_BZIP2 is either undefined or "y". Thus, >> block-obj-dmg-bz2-y is either left undefined or set to dmg-bz2.o. > > Yes. > >> Perhaps the '+=' be ':=', but we seem to use '+=' pretty >> indiscriminately. > > Yep. I don't know. Whatever works, and both do, so... > >>> block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y) >> >> As far as I can tell, CONFIG_DMG is also either undefined or "y". So, >> this adds dmg-bz2.o to block-obj-m if both CONFIG_BZIP2 and CONFIG_DMG >> are enabled. > > Yes. > >> Shouldn't it be added to block-obj-y, like dmg.o, or am I confused? > > The behavior before this patch was to add it to block-obj-m. > 27685a8dd08c051fa6d641fe46106fc0dfa51073 has the explanation: We want > the bz2 part to be a module so you can launch qemu even without libbz2 > around. Only when you use dmg will it load that module. > > (And if you dig deeper, it was 88d88798b7efe that (intentionally) broke > that intended behavior, until it was restored by the above commit.) Thanks, v2 sent with your fix.
[Qemu-block] [PATCH v2] 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 --- v2: Fix handling of dmg-bz2.o [Max] 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..46d585cfd0 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-$(CONFIG_BZIP2) += dmg-bz2.o +block-obj-$(if $(CONFIG_DMG),m,n) += $(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 74e313a810..5b1d83ea26 100755 --- a/configure +++ b/configure @@ -470,6 +470,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" @@ -1416,6 +1425,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) @@ -1718,6 +1763,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 @@ -6043,6 +6097,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 $parallels" +echo "sheepdog support $sheepdog" echo "capstone $capstone" echo "docker$docker"
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
Eduardo Habkost writes: > The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis > seems to provide an older version. Change the existing rules to > use command output instead of exit code, to make it compatible > with older GNU make versions. > > Signed-off-by: Eduardo Habkost > --- > I think that's the cause of the Travis failures. I have > submitted a test job right now, at: > https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 > Let's see if it fixes the issue. > --- > tests/Makefile.include | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index d2e577eabb..074eece558 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results > # information please refer to "avocado --help". > AVOCADO_SHOW=none > > -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' > >/dev/null 2>&1) > -ifeq ($(.SHELLSTATUS),0) > +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= > (3, 0) else 0)') > +ifeq ($(PYTHON3), 1) > $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > $(call quiet-command, \ > $(PYTHON) -m venv --system-site-packages $@, \ PEP 394 recommends software distributions install Python 3 into the default path as python3, and users use that instead of python, except for programs that are source compatible with both 2 and 3. So, is finding out whether python is a Python 3 really appropriate? Why can't we just use python3 and be done with it? If we can't: isn't this a configure problem?
Re: [Qemu-block] [PATCH 7/7] qcow2: do decompression in threads
On 01/11/2018 19:27, Vladimir Sementsov-Ogievskiy wrote:
> -/* See qcow2_compress definition for parameters description */
> -static ssize_t qcow2_co_compress(BlockDriverState *bs,
> - void *dest, size_t dest_size,
> - const void *src, size_t src_size)
> +static ssize_t qcow2_co_do_compress(BlockDriverState *bs,
> +void *dest, size_t dest_size,
> +const void *src, size_t src_size,
> +Qcow2CompressFunc func)
> {
> BDRVQcow2State *s = bs->opaque;
> BlockAIOCB *acb;
> @@ -3838,6 +3842,7 @@ static ssize_t qcow2_co_compress(BlockDriverState *bs,
> .dest_size = dest_size,
> .src = src,
> .src_size = src_size,
> +.func = func,
> };
>
> while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
> @@ -3860,6 +3865,22 @@ static ssize_t qcow2_co_compress(BlockDriverState *bs,
> return arg.ret;
> }
>
Not a blocker for this patch, but I'll note that qcow2_co_do_compress
could use thread_pool_submit_co. Also, qcow2_co_do_compress should be a
"coroutine_fn".
Paolo
Re: [Qemu-block] [Qemu-devel] [PULL 12/15] Acceptance tests: add make rule for running them
On 31/10/2018 01:31, Eduardo Habkost wrote: > From: Cleber Rosa > > The acceptance (aka functional, aka Avocado-based) tests are > Python files located in "tests/acceptance" that need to be run > with the Avocado libs and test runner. > > Let's provide a convenient way for QEMU developers to run them, > by making use of the tests-venv with the required setup. > > Also, while the Avocado test runner will take care of creating a > location to save test results to, it was understood that it's better > if the results are kept within the build tree. > > Signed-off-by: Cleber Rosa > Acked-by: Stefan Hajnoczi > Acked-by: Wainer dos Santos Moschetta > Reviewed-by: Caio Carrara > Message-Id: <20181018153134.8493-3-cr...@redhat.com> > Signed-off-by: Eduardo Habkost > --- > docs/devel/testing.rst | 43 +- > tests/requirements.txt | 1 + > tests/Makefile.include | 21 +++-- > 3 files changed, 58 insertions(+), 7 deletions(-) > > diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst > index a227754f86..18e2c0868a 100644 > --- a/docs/devel/testing.rst > +++ b/docs/devel/testing.rst > @@ -545,10 +545,39 @@ Tests based on ``avocado_qemu.Test`` can easily: > - > http://avocado-framework.readthedocs.io/en/latest/api/test/avocado.html#avocado.Test > - > http://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html > > -Installation > - > +Running tests > +- > + > +You can run the acceptance tests simply by executing: > + > +.. code:: > + > + make check-acceptance > + > +This involves the automatic creation of Python virtual environment > +within the build tree (at ``tests/venv``) which will have all the > +right dependencies, and will save tests results also within the > +build tree (at ``tests/results``). > > -To install Avocado and its dependencies, run: > +Note: the build environment must be using a Python 3 stack, and have > +the ``venv`` and ``pip`` packages installed. If necessary, make sure > +``configure`` is called with ``--python=`` and that those modules are > +available. On Debian and Ubuntu based systems, depending on the > +specific version, they may be on packages named ``python3-venv`` and > +``python3-pip``. > + > +The scripts installed inside the virtual environment may be used > +without an "activation". For instance, the Avocado test runner > +may be invoked by running: > + > + .. code:: > + > + tests/venv/bin/avocado run $OPTION1 $OPTION2 tests/acceptance/ > + > +Manual Installation > +--- > + > +To manually install Avocado and its dependencies, run: > > .. code:: > > @@ -689,11 +718,15 @@ The exact QEMU binary to be used on QEMUMachine. > Uninstalling Avocado > > > -If you've followed the installation instructions above, you can easily > -uninstall Avocado. Start by listing the packages you have installed:: > +If you've followed the manual installation instructions above, you can > +easily uninstall Avocado. Start by listing the packages you have > +installed:: > >pip list --user > > And remove any package you want with:: > >pip uninstall > + > +If you've used ``make check-acceptance``, the Python virtual environment > where > +Avocado is installed will be cleaned up as part of ``make check-clean``. > diff --git a/tests/requirements.txt b/tests/requirements.txt > index d39f9d1576..64c6e27a94 100644 > --- a/tests/requirements.txt > +++ b/tests/requirements.txt > @@ -1,3 +1,4 @@ > # Add Python module requirements, one per line, to be installed > # in the tests/venv Python virtual environment. For more info, > # refer to: https://pip.pypa.io/en/stable/user_guide/#id1 > +avocado-framework==65.0 > diff --git a/tests/Makefile.include b/tests/Makefile.include > index eabc1da2f3..d2e577eabb 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -11,6 +11,7 @@ check-help: > @echo " $(MAKE) check-qapi-schemaRun QAPI schema tests" > @echo " $(MAKE) check-block Run block tests" > @echo " $(MAKE) check-tcgRun TCG tests" > + @echo " $(MAKE) check-acceptance Run all acceptance (functional) > tests" > @echo " $(MAKE) check-report.htmlGenerates an HTML test report" > @echo " $(MAKE) check-venv Creates a Python venv for tests" > @echo " $(MAKE) check-clean Clean the tests" > @@ -902,10 +903,15 @@ check-decodetree: > > # Python venv for running tests > > -.PHONY: check-venv > +.PHONY: check-venv check-acceptance > > TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv > TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt > +TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results > +# Controls the output generated by Avocado when running tests. > +# Any number of command separated loggers are accepted. For more > +# information please refer to "avocado --help". > +AVOCADO_SHOW=none > > $(shell $(PYTHON) -c 'import sys; assert
Re: [Qemu-block] ping Re: [PATCH v4 00/11] backup-top filter driver for backup
On 11/06/2018 12:21 PM, Kevin Wolf wrote: > Am 02.11.2018 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben: >> ping >> >> 15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote: >>> Hi all! >>> >>> These series introduce backup-top driver. It's a filter-node, which >>> do copy-before-write operation. Mirror uses filter-node for handling >>> guest writes, let's move to filter-node (from write-notifiers) for >>> backup too (patch 16) >>> >>> v4: >>> fixes, rewrite driver to be implicit, drop new interfaces and >>> don't move to BdrvDirtyBitmap for now, as it's not obvious will >>> it be really needed and don't relate to these series more. >>> >>> v3 was "[PATCH v3 00/18] fleecing-hook driver for backup" >>> >>> v2 was "[RFC v2] new, node-graph-based fleecing and backup" >>> >>> These series are based on >>> [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area >>> and >>> [PATCH 0/2] replication: drop extra sync > > Before we can move forward here, we need to deal with the dependencies. > I merged the second one now (because there wasn't any feedback from the > replication maintainers), but the first one looks like it was going > through John's or Eric's tree? > I was expecting it to go through Eric's because it was predicated on NBD patches that he was staging. Is that no longer true? --js
Re: [Qemu-block] [PATCH v4 03/11] block: allow serialized reads to intersect
Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben: > Otherwise, if we have serialized read-part in copy_range from backing > file to its parent if CoW take place, this CoW's sub-reads will > intersect with firstly created serialized read request. > > Anyway, reads should not influence on disk view, let's allow them to > intersect. > > Signed-off-by: Vladimir Sementsov-Ogievskiy What about reads that internally trigger writes, such as copy-on-read? Kevin
Re: [Qemu-block] [PATCH v4 04/11] block: improve should_update_child
Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> As it already said in the comment, we don't want to create loops in
> parent->child relations. So, when we try to append @to to @c, we should
> check that @c is not in @to children subtree, and we should check it
> recursively, not only the first level. The patch provides BFS-based
> search, to check the relations.
>
> This is needed for further fleecing-hook filter usage: we need to
> append it to source, when the hook is already a parent of target, and
> source may be in a backing chain of target (fleecing-scheme). So, on
> appending, the hook should not became a child (direct or through
> children subtree) of the target.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block.c | 32 +++-
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index c298ca6a19..7f605b0bf0 100644
> --- a/block.c
> +++ b/block.c
> @@ -3432,7 +3432,7 @@ void bdrv_close_all(void)
>
> static bool should_update_child(BdrvChild *c, BlockDriverState *to)
> {
> -BdrvChild *to_c;
> +GList *queue = NULL, *pos;
>
> if (c->role->stay_at_node) {
> return false;
> @@ -3468,13 +3468,35 @@ static bool should_update_child(BdrvChild *c,
> BlockDriverState *to)
> * if A is a child of B, that means we cannot replace A by B there
> * because that would create a loop. Silently detaching A from B
> * is also not really an option. So overall just leaving A in
> - * place there is the most sensible choice. */
> -QLIST_FOREACH(to_c, >children, next) {
> -if (to_c == c) {
> -return false;
> + * place there is the most sensible choice.
> + *
> + * upd: If the child @c belongs to the @to's children, or children of
> it's
> + * children and so on - this would create a loop to. To prevent it let's
> do
> + * a BFS search on @to children subtree.
> + */
I don't like the "upd:" thing. Let me try to rephrase the addition:
We would also create a loop in any cases where @c is only indirectly
referenced by @to. Prevent this by returning false if @c is found
(by breadth-first search) anywhere in the whole subtree of @to.
Also, even though the coding style says 80 characters per line, the
existing comment seems to use only 70 characters. Let's try to stay
consistent here, otherwise it looks a bit odd.
> +pos = queue = g_list_append(queue, to);
> +while (pos) {
> +BlockDriverState *v = pos->data;
> +BdrvChild *c2;
> +
> +QLIST_FOREACH(c2, >children, next) {
> +if (c2 == c) {
> +g_list_free(queue);
> +return false;
> +}
> +
> +if (g_list_find(queue, c2->bs)) {
> +continue;
> +}
> +
> +queue = g_list_append(queue, c2->bs);
> }
> +
> +pos = pos->next;
> }
>
> +g_list_free(queue);
> return true;
> }
The functional change looks good to me.
Kevin
Re: [Qemu-block] [PATCH v4 11/11] block/backup: use backup-top instead of write notifiers
Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Drop write notifiers and use filter node instead. Changes:
>
> 1. copy-before-writes now handled by filter node, so, drop all
>is_write_notifier arguments.
>
> 2. we don't have intersecting requests, so their handling is dropped.
> Instead, synchronization works as follows:
> when backup or backup-top starts copying of some area it firstly
> clears copy-bitmap bits, and nobody touches areas, not marked with
> dirty bits in copy-bitmap, so there is no intersection. Also, backup
> job copy operations are surrounded by bdrv region lock, which is
> actually serializing request, to not interfer with guest writes and
> not read changed data from source (before reading we clear
> corresponding bit in copy-bitmap, so, this area is not more handled by
> backup-top).
>
> 3. To sync with in-flight requests we now just drain hook node, we
> don't need rw-lock.
>
> 4. After the whole backup loop (top, full, incremental modes), we need
> to check for not copied clusters, which were under backup-top operation
> and we skipped them, but backup-top operation failed, error returned to
> the guest and dirty bits set back.
>
> 5. Don't create additional blk, use backup-top children for copy
> operations.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
Haven't reviewed it yet, but gcc complains correctly here:
block/backup.c: In function 'backup_job_create':
block/backup.c:717:9: error: 'backup_top' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
bdrv_backup_top_drop(backup_top);
^~~~
cc1: all warnings being treated as errors
> @@ -653,24 +648,29 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
>
> copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
>
> +/* bdrv_get_device_name will not help to find device name starting from
> + * @bs after backup-top append, so let's calculate job_id before. Do
> + * it in the same way like block_job_create
> + */
> +if (job_id == NULL && !(creation_flags & JOB_INTERNAL)) {
> +job_id = bdrv_get_device_name(bs);
> +}
> +
> +backup_top = bdrv_backup_top_append(bs, target, copy_bitmap, errp);
> +if (!backup_top) {
> +return NULL;
This needs to be goto error, just like in the bdrv_getlength() failure a
few lines above (which is where the uninitialised backup_top warning
comes from).
Kevin
Re: [Qemu-block] ping Re: [PATCH v4 00/11] backup-top filter driver for backup
Am 02.11.2018 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben: > ping > > 15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote: > > Hi all! > > > > These series introduce backup-top driver. It's a filter-node, which > > do copy-before-write operation. Mirror uses filter-node for handling > > guest writes, let's move to filter-node (from write-notifiers) for > > backup too (patch 16) > > > > v4: > > fixes, rewrite driver to be implicit, drop new interfaces and > > don't move to BdrvDirtyBitmap for now, as it's not obvious will > > it be really needed and don't relate to these series more. > > > > v3 was "[PATCH v3 00/18] fleecing-hook driver for backup" > > > > v2 was "[RFC v2] new, node-graph-based fleecing and backup" > > > > These series are based on > > [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area > > and > > [PATCH 0/2] replication: drop extra sync Before we can move forward here, we need to deal with the dependencies. I merged the second one now (because there wasn't any feedback from the replication maintainers), but the first one looks like it was going through John's or Eric's tree? Kevin
Re: [Qemu-block] ping2 Re: [PATCH 0/2] replication: drop extra sync
Am 31.10.2018 um 10:30 hat Vladimir Sementsov-Ogievskiy geschrieben: > ping2 > > Hi, it's a first step to backup refactoring and improving. Just get rid > of this extra and unnatural synchronization. The replication maintainers don't seem to have an opinion about this, so thanks, applied to block-next (for 3.2). Kevin
Re: [Qemu-block] [PATCH] blockdev: handle error on block latency histogram set error
Am 05.11.2018 um 04:04 hat zhenwei pi geschrieben:
> Function block_latency_histogram_set may return error, but qapi ignore this.
> This can be reproduced easily by qmp command:
> virsh qemu-monitor-command INSTANCE
> '{"execute":"x-block-latency-histogram-set",
> "arguments":{"device":"drive-virtio-disk1","boundaries":[10,200,40]}}'
> In fact this command does not work, but we still get success result.
>
> qmp_x_block_latency_histogram_set is a batch setting API, report error ASAP.
>
> Signed-off-by: zhenwei pi
Thanks, applied to the block branch.
Kevin
Re: [Qemu-block] [PATCH v3 0/4] Adding LZFSE compression support for DMG block driver.
Am 05.11.2018 um 16:08 hat Julio Faracco geschrieben: > Since Mac OS X El Capitain (v10.11), Apple uses LZFSE compression to > generate compressed DMGs as an alternative to BZIP2. Possible, Apple > want to keep this algorithm as default in long term. Some years ago, > Apple opened the LZFSE algorithm to opensource and the main source (or > the most active repo), can be found at: https://github.com/lzfse/lzfse > > v1-v2: Fixing some error handlings from dmg-lzfse.c > v2-v3: Master rebasing suggestion from Stefan. Thanks, applied to the block-next branch (for 3.2). Stefan, if you have some more comments, we can still modify/dequeue the patches, but this seems to address all of my review comments for v1. Kevin
Re: [Qemu-block] [PATCH for-3.1] blockdev: Consistently use snapshot_node_name in external_snapshot_prepare()
Am 01.11.2018 um 17:30 hat Peter Maydell geschrieben: > In the function external_snapshot_prepare() we have a > BlockdevSnapshotSync struct, which has the usual combination > of has_snapshot_node_name and snapshot_node_name fields for an > optional field. We set up a local variable > const char *snapshot_node_name = > s->has_snapshot_node_name ? s->snapshot_node_name : NULL; > > and then mostly use "if (!snapshot_node_name)" for checking > whether we have a snapshot node name. The exception is that in > one place we check s->has_snapshot_node_name instead. This > confuses Coverity (CID 1396473), which thinks it might be > possible to get here with s->has_snapshot_node_name true but > snapshot_node_name NULL, and warns that the call to > qdict_put_str() will segfault in that case. > > Make the code consistent and unconfuse Coverity by using > the same check for this conditional that we do in the rest > of the surrounding code. > > Signed-off-by: Peter Maydell Thanks, applied to the block branch. Kevin
Re: [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read
06.11.2018 18:30, Alberto Garcia wrote:
> On Tue 06 Nov 2018 04:13:58 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> 06.11.2018 18:06, Alberto Garcia wrote:
>>> On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>>
+buf = g_try_malloc(csize);
+if (!buf) {
+return -ENOMEM;
+}
+iov.iov_base = buf;
+iov.iov_len = csize;
+qemu_iovec_init_external(_qiov, , 1);
-iov.iov_base = s->cluster_data;
-iov.iov_len = csize;
-qemu_iovec_init_external(_qiov, , 1);
+out_buf = qemu_blockalign(bs, s->cluster_size);
>>> You should also check whether out_buf is NULL, shouldn't you?
>> No, it will abort on fail. qemu_try_blockalign result should be
>> checked.
> Is there any reason why some parts of the QEMU code use qemu_blockalign
> and others qemu_try_blockalign() ? From what I can see it seems to be up
> to whoever wrote it...
>
> Berto
As I understand, the good reason to use _try_ versions, is when we are
allocating some size, taken from user input, so it may be unpredictable
large (hm, or just any really large allocation), so, I use try_malloc
for compressed size, which may be very large in somehow corrupted image.
And it looks a common practice to use not-failing (aborting) allocation
functions for cluster_size allocations in qcow2.c
--
Best regards,
Vladimir
[Qemu-block] [PATCH 1/1] virtio-blk: rename iov to out_iov in virtio_blk_handle_request()
In virtio_blk_handle_request(), in_iov is used for input header while iov
is used for output header. Rename iov to out_iov to pair output header's
name with in_iov to avoid confusing people when reading source code.
Signed-off-by: Dongli Zhang
---
hw/block/virtio-blk.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 83cf5c0..fb0d74d 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -482,7 +482,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
{
uint32_t type;
struct iovec *in_iov = req->elem.in_sg;
-struct iovec *iov = req->elem.out_sg;
+struct iovec *out_iov = req->elem.out_sg;
unsigned in_num = req->elem.in_num;
unsigned out_num = req->elem.out_num;
VirtIOBlock *s = req->dev;
@@ -493,13 +493,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
return -1;
}
-if (unlikely(iov_to_buf(iov, out_num, 0, >out,
+if (unlikely(iov_to_buf(out_iov, out_num, 0, >out,
sizeof(req->out)) != sizeof(req->out))) {
virtio_error(vdev, "virtio-blk request outhdr too short");
return -1;
}
-iov_discard_front(, _num, sizeof(req->out));
+iov_discard_front(_iov, _num, sizeof(req->out));
if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
virtio_error(vdev, "virtio-blk request inhdr too short");
@@ -526,7 +526,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
>out.sector);
if (is_write) {
-qemu_iovec_init_external(>qiov, iov, out_num);
+qemu_iovec_init_external(>qiov, out_iov, out_num);
trace_virtio_blk_handle_write(vdev, req, req->sector_num,
req->qiov.size / BDRV_SECTOR_SIZE);
} else {
--
2.7.4
Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] nvme: don't unref ctrl_mem when device unrealized
On Sun, 28 Oct 2018 23:29:40 -0700
Li Qiang wrote:
> Currently, when hotplug/unhotplug nvme device, it will cause an
> assert in object.c. Following is the backtrack:
>
> ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)
>
> Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7fffcbd32700 (LWP 18844)]
> 0x7fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) bt
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> qom/object.c:981
> /home/liqiang02/qemu-upstream/qemu/memory.c:1732
> /home/liqiang02/qemu-upstream/qemu/memory.c:285
> util/qemu-thread-posix.c:504
> /lib/x86_64-linux-gnu/libpthread.so.0
>
> This is caused by memory_region_unref in nvme_exit.
>
> Remove it to make the PCIdevice refcount correct.
>
> Signed-off-by: Li Qiang
nvme device holds a reference to ctrl_mem MemoryRegion as a parent
so MemoryRegion will be destroyed later during destruction of
nvme object when its cildren are un-parented.
Reviewed-by: Igor Mammedov
> ---
> hw/block/nvme.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..359a06d0ad 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
> g_free(n->namespaces);
> g_free(n->cq);
> g_free(n->sq);
> -if (n->cmbsz) {
> -memory_region_unref(>ctrl_mem);
> -}
>
> msix_uninit_exclusive_bar(pci_dev);
> }
Re: [Qemu-block] [Qemu-devel] [PATCH v6 06/10] hw/m68k: add Nubus support
On 2018-11-02 16:22, Mark Cave-Ayland wrote: > From: Laurent Vivier > > Co-developed-by: Mark Cave-Ayland > Signed-off-by: Mark Cave-Ayland > Signed-off-by: Laurent Vivier > --- > hw/Makefile.objs| 1 + > hw/nubus/Makefile.objs | 4 + > hw/nubus/mac-nubus-bridge.c | 45 > hw/nubus/nubus-bridge.c | 34 ++ > hw/nubus/nubus-bus.c| 111 +++ > hw/nubus/nubus-device.c | 215 > > include/hw/nubus/mac-nubus-bridge.h | 24 > include/hw/nubus/nubus.h| 69 > 8 files changed, 503 insertions(+) > create mode 100644 hw/nubus/Makefile.objs > create mode 100644 hw/nubus/mac-nubus-bridge.c > create mode 100644 hw/nubus/nubus-bridge.c > create mode 100644 hw/nubus/nubus-bus.c > create mode 100644 hw/nubus/nubus-device.c > create mode 100644 include/hw/nubus/mac-nubus-bridge.h > create mode 100644 include/hw/nubus/nubus.h Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 6 November 2018 at 14:13, Eduardo Habkost wrote: > The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis > seems to provide an older version. Change the existing rules to > use command output instead of exit code, to make it compatible > with older GNU make versions. > > Signed-off-by: Eduardo Habkost > --- > I think that's the cause of the Travis failures. I have > submitted a test job right now, at: > https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 > Let's see if it fixes the issue. > --- > tests/Makefile.include | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index d2e577eabb..074eece558 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results > # information please refer to "avocado --help". > AVOCADO_SHOW=none > > -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' > >/dev/null 2>&1) > -ifeq ($(.SHELLSTATUS),0) > +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= > (3, 0) else 0)') > +ifeq ($(PYTHON3), 1) > $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > $(call quiet-command, \ > $(PYTHON) -m venv --system-site-packages $@, \ > -- Thanks, applied to master as a build fix. -- PMM
Re: [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read
On Tue 06 Nov 2018 04:13:58 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2018 18:06, Alberto Garcia wrote:
>> On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> +buf = g_try_malloc(csize);
>>> +if (!buf) {
>>> +return -ENOMEM;
>>> +}
>>> +iov.iov_base = buf;
>>> +iov.iov_len = csize;
>>> +qemu_iovec_init_external(_qiov, , 1);
>>>
>>> -iov.iov_base = s->cluster_data;
>>> -iov.iov_len = csize;
>>> -qemu_iovec_init_external(_qiov, , 1);
>>> +out_buf = qemu_blockalign(bs, s->cluster_size);
>> You should also check whether out_buf is NULL, shouldn't you?
>
> No, it will abort on fail. qemu_try_blockalign result should be
> checked.
Is there any reason why some parts of the QEMU code use qemu_blockalign
and others qemu_try_blockalign() ? From what I can see it seems to be up
to whoever wrote it...
Berto
Re: [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read
06.11.2018 18:06, Alberto Garcia wrote:
> On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>
>> +buf = g_try_malloc(csize);
>> +if (!buf) {
>> +return -ENOMEM;
>> +}
>> +iov.iov_base = buf;
>> +iov.iov_len = csize;
>> +qemu_iovec_init_external(_qiov, , 1);
>>
>> -iov.iov_base = s->cluster_data;
>> -iov.iov_len = csize;
>> -qemu_iovec_init_external(_qiov, , 1);
>> +out_buf = qemu_blockalign(bs, s->cluster_size);
> You should also check whether out_buf is NULL, shouldn't you?
No, it will abort on fail. qemu_try_blockalign result should be checked.
>
> Berto
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH 5/7] qcow2: use byte-based read in qcow2_decompress_cluster
06.11.2018 16:53, Alberto Garcia wrote:
> On Thu 01 Nov 2018 07:27:36 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index e9d24b801e..950b9f7ec6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3956,14 +3956,15 @@ fail:
>> int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> -int ret, csize, nb_csectors, sector_offset;
>> +int ret, csize, nb_csectors;
>> uint64_t coffset;
>> +struct iovec iov;
>> +QEMUIOVector local_qiov;
>>
>> coffset = cluster_offset & s->cluster_offset_mask;
>> if (s->cluster_cache_offset != coffset) {
>> nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask)
>> + 1;
>> -sector_offset = coffset & 511;
>> -csize = nb_csectors * 512 - sector_offset;
>> +csize = nb_csectors * 512 - (coffset & 511);
>>
>> /* Allocate buffers on first decompress operation, most images are
>>* uncompressed and the memory overhead can be avoided. The
>> buffers
>> @@ -3981,14 +3982,17 @@ int qcow2_decompress_cluster(BlockDriverState *bs,
>> uint64_t cluster_offset)
>> s->cluster_cache = g_malloc(s->cluster_size);
>> }
>>
>> +iov.iov_base = s->cluster_data;
>> +iov.iov_len = csize;
>> +qemu_iovec_init_external(_qiov, , 1);
>> +
>> BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
>> -ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
>> -nb_csectors);
>> +ret = bdrv_co_preadv(bs->file, coffset, csize, _qiov,
>> 0);
> I think you should annotate the function with coroutine_fn or use
> bdrv_pread() instead.
>
> Berto
it is called only from qcow2_co_preadv, so it's ok to move to
already-in-coroutine behaviour. I'll add coroutine_fn if new version is
needed, or it can be added inflight.
--
Best regards,
Vladimir
Re: [Qemu-block] [Qemu-devel] [PATCH v6 02/10] hw/m68k: implement ADB bus support for via
On 2018-11-02 16:22, Mark Cave-Ayland wrote: > From: Laurent Vivier > > Co-developed-by: Mark Cave-Ayland > Signed-off-by: Mark Cave-Ayland > Signed-off-by: Laurent Vivier > Reviewed-by: Hervé Poussineau > --- > hw/misc/mac_via.c | 190 > ++ > include/hw/misc/mac_via.h | 7 ++ > 2 files changed, 197 insertions(+) Looks ok to me at a quick glance now (can't say much about the details since I don't know the ADB stuff at all), so FWIW, a weak: Reviewed-by: Thomas Huth
Re: [Qemu-block] [Qemu-devel] [PATCH v6 04/10] hw/m68k: add macfb video card
On 2018-11-02 16:22, Mark Cave-Ayland wrote: > From: Laurent Vivier > > Co-developed-by: Mark Cave-Ayland > Signed-off-by: Mark Cave-Ayland > Signed-off-by: Laurent Vivier > Reviewed-by: Hervé Poussineau > --- > arch_init.c| 4 + > hw/display/Makefile.objs | 1 + > hw/display/macfb.c | 419 > + > include/hw/display/macfb.h | 43 + > qemu-options.hx| 2 +- > vl.c | 3 +- > 6 files changed, 470 insertions(+), 2 deletions(-) > create mode 100644 hw/display/macfb.c > create mode 100644 include/hw/display/macfb.h Again, due to lack of knowledge about the Mac hardware, just a weak: Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read
On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> +buf = g_try_malloc(csize);
> +if (!buf) {
> +return -ENOMEM;
> +}
> +iov.iov_base = buf;
> +iov.iov_len = csize;
> +qemu_iovec_init_external(_qiov, , 1);
>
> -iov.iov_base = s->cluster_data;
> -iov.iov_len = csize;
> -qemu_iovec_init_external(_qiov, , 1);
> +out_buf = qemu_blockalign(bs, s->cluster_size);
You should also check whether out_buf is NULL, shouldn't you?
Berto
Re: [Qemu-block] [Qemu-devel] [PATCH v6 00/10] hw/m68k: add Apple Machintosh Quadra 800 machine
On 2018-11-04 14:41, Mark Cave-Ayland wrote:
> On 04/11/2018 06:53, no-re...@patchew.org wrote:
>
>> Hi,
>>
>> This series seems to have some coding style problems. See output below for
>> more information:
>>
>> Type: series
>> Message-id: 20181102152257.20637-1-mark.cave-ayl...@ilande.co.uk
>> Subject: [Qemu-devel] [PATCH v6 00/10] hw/m68k: add Apple Machintosh Quadra
>> 800 machine
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>>
>> BASE=base
>> n=1
>> total=$(git log --oneline $BASE.. | wc -l)
>> failed=0
>>
>> git config --local diff.renamelimit 0
>> git config --local diff.renames True
>> git config --local diff.algorithm histogram
>>
>> commits="$(git log --format=%H --reverse $BASE..)"
>> for c in $commits; do
>> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -;
>> then
>> failed=1
>> echo
>> fi
>> n=$((n+1))
>> done
>>
>> exit $failed
>> === TEST SCRIPT END ===
>>
>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>> Switched to a new branch 'test'
>> 9245542a6d hw/m68k: define Macintosh Quadra 800
>> 4b2499b92a dp8393x: manage big endian bus
>> 9a13b06b9f hw/m68k: add a dummy SWIM floppy controller
>> bdf5c3725c hw/m68k: add Nubus support for macfb video card
>> 6feff81d33 hw/m68k: add Nubus support
>> b24a39f653 esp: add pseudo-DMA as used by Macintosh
>> 970394cb59 hw/m68k: add macfb video card
>> 30013c1b4b escc: introduce a selector for the register bit
>> 3274bd6559 hw/m68k: implement ADB bus support for via
>> b78fd7f12a hw/m68k: add via support
>>
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/10: hw/m68k: add via support...
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #26:
>> new file mode 100644
>>
>> ERROR: space prohibited after that '&&' (ctx:WxW)
>> #348: FILE: hw/misc/mac_via.c:318:
>> +if (!(v1s->last_b & VIA1B_vRTCClk) && (s->b & VIA1B_vRTCClk)) {
>> ^
>
> False positive from checkpatch?
Looks like a false positive, indeed.
Thomas
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
Hi Peter, Can you apply this patch as a CI bug-fix? Thanks, Phil. On 6/11/18 15:27, Philippe Mathieu-Daudé wrote: On 6/11/18 15:13, Eduardo Habkost wrote: The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis seems to provide an older version. Change the existing rules to use command output instead of exit code, to make it compatible with older GNU make versions. You were quicker, I just found out :) Signed-off-by: Eduardo Habkost --- I think that's the cause of the Travis failures. I have submitted a test job right now, at: https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 Let's see if it fixes the issue. You can run it locally: $ make docker-image-travis ... $ docker run --rm -it -v $(pwd):$(pwd) -w $(pwd) \ -u 0 --entrypoint=/bin/bash qemu:travis root@ede09efe22fd:qemu# cd build/docker_travis/ root@ede09efe22fd:qemu/build/docker_travis# make AVOCADO_SHOW=app check-acceptance VENV qemu/build/docker_travis/tests/venv The virtual environment was not created successfully because ensurepip is not available. On Debian/Ubuntu systems, you need to install the python3-venv package using the following command. apt-get install python3-venv You may need to use sudo with that command. After installing the python3-venv package, recreate your virtual environment. root@ede09efe22fd:qemu/build/docker_travis# apt install python3-pip python3.4-venv root@ede09efe22fd:qemu/build/docker_travis# make AVOCADO_SHOW=app check-acceptance VENV qemu/build/docker_travis/tests/venv PIP qemu/tests/requirements.txt AVOCADO tests/acceptance JOB ID : 5e5529e79456c388e80323acdc71f3887341a498 JOB LOG : qemu/build/docker_travis/tests/results/job-2018-11-06T14.26-5e5529e/job.log (1/6) qemu/tests/acceptance/boot_linux_console.py:BootLinuxConsole.test: CANCEL: No QEMU binary defined or found in the source tree (0.01 s) (2/6) qemu/tests/acceptance/version.py:Version.test_qmp_human_info_version: CANCEL: No QEMU binary defined or found in the source tree (0.01 s) (3/6) qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) (4/6) qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc_change_password: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) (5/6) qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password_requires_a_password: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) (6/6) qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) RESULTS : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 6 JOB TIME : 0.45 s Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé --- tests/Makefile.include | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index d2e577eabb..074eece558 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results # information please refer to "avocado --help". AVOCADO_SHOW=none -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >/dev/null 2>&1) -ifeq ($(.SHELLSTATUS),0) +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= (3, 0) else 0)') +ifeq ($(PYTHON3), 1) $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(call quiet-command, \ $(PYTHON) -m venv --system-site-packages $@, \
Re: [Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 6/11/18 15:13, Eduardo Habkost wrote: The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis seems to provide an older version. Change the existing rules to use command output instead of exit code, to make it compatible with older GNU make versions. You were quicker, I just found out :) Signed-off-by: Eduardo Habkost --- I think that's the cause of the Travis failures. I have submitted a test job right now, at: https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 Let's see if it fixes the issue. You can run it locally: $ make docker-image-travis ... $ docker run --rm -it -v $(pwd):$(pwd) -w $(pwd) \ -u 0 --entrypoint=/bin/bash qemu:travis root@ede09efe22fd:qemu# cd build/docker_travis/ root@ede09efe22fd:qemu/build/docker_travis# make AVOCADO_SHOW=app check-acceptance VENVqemu/build/docker_travis/tests/venv The virtual environment was not created successfully because ensurepip is not available. On Debian/Ubuntu systems, you need to install the python3-venv package using the following command. apt-get install python3-venv You may need to use sudo with that command. After installing the python3-venv package, recreate your virtual environment. root@ede09efe22fd:qemu/build/docker_travis# apt install python3-pip python3.4-venv root@ede09efe22fd:qemu/build/docker_travis# make AVOCADO_SHOW=app check-acceptance VENVqemu/build/docker_travis/tests/venv PIP qemu/tests/requirements.txt AVOCADO tests/acceptance JOB ID : 5e5529e79456c388e80323acdc71f3887341a498 JOB LOG: qemu/build/docker_travis/tests/results/job-2018-11-06T14.26-5e5529e/job.log (1/6) qemu/tests/acceptance/boot_linux_console.py:BootLinuxConsole.test: CANCEL: No QEMU binary defined or found in the source tree (0.01 s) (2/6) qemu/tests/acceptance/version.py:Version.test_qmp_human_info_version: CANCEL: No QEMU binary defined or found in the source tree (0.01 s) (3/6) qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) (4/6) qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc_change_password: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) (5/6) qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password_requires_a_password: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) (6/6) qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password: CANCEL: No QEMU binary defined or found in the source tree (0.00 s) RESULTS: PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 6 JOB TIME : 0.45 s Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé --- tests/Makefile.include | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index d2e577eabb..074eece558 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results # information please refer to "avocado --help". AVOCADO_SHOW=none -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >/dev/null 2>&1) -ifeq ($(.SHELLSTATUS),0) +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= (3, 0) else 0)') +ifeq ($(PYTHON3), 1) $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(call quiet-command, \ $(PYTHON) -m venv --system-site-packages $@, \
Re: [Qemu-block] [PATCH 7/7] qcow2: do decompression in threads
On Thu 01 Nov 2018 07:27:38 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Do decompression in threads, like it is already done for compression. > This improves asynchronous compressed reads performance. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
[Qemu-block] [PATCH] tests: Fix Python 3 detection on older GNU make versions
The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis seems to provide an older version. Change the existing rules to use command output instead of exit code, to make it compatible with older GNU make versions. Signed-off-by: Eduardo Habkost --- I think that's the cause of the Travis failures. I have submitted a test job right now, at: https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 Let's see if it fixes the issue. --- tests/Makefile.include | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index d2e577eabb..074eece558 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results # information please refer to "avocado --help". AVOCADO_SHOW=none -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >/dev/null 2>&1) -ifeq ($(.SHELLSTATUS),0) +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= (3, 0) else 0)') +ifeq ($(PYTHON3), 1) $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(call quiet-command, \ $(PYTHON) -m venv --system-site-packages $@, \ -- 2.18.0.rc1.1.g3f1ff2140
Re: [Qemu-block] [PATCH 5/7] qcow2: use byte-based read in qcow2_decompress_cluster
On Thu 01 Nov 2018 07:27:36 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e9d24b801e..950b9f7ec6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3956,14 +3956,15 @@ fail:
> int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
> {
> BDRVQcow2State *s = bs->opaque;
> -int ret, csize, nb_csectors, sector_offset;
> +int ret, csize, nb_csectors;
> uint64_t coffset;
> +struct iovec iov;
> +QEMUIOVector local_qiov;
>
> coffset = cluster_offset & s->cluster_offset_mask;
> if (s->cluster_cache_offset != coffset) {
> nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) +
> 1;
> -sector_offset = coffset & 511;
> -csize = nb_csectors * 512 - sector_offset;
> +csize = nb_csectors * 512 - (coffset & 511);
>
> /* Allocate buffers on first decompress operation, most images are
> * uncompressed and the memory overhead can be avoided. The buffers
> @@ -3981,14 +3982,17 @@ int qcow2_decompress_cluster(BlockDriverState *bs,
> uint64_t cluster_offset)
> s->cluster_cache = g_malloc(s->cluster_size);
> }
>
> +iov.iov_base = s->cluster_data;
> +iov.iov_len = csize;
> +qemu_iovec_init_external(_qiov, , 1);
> +
> BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
> -ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
> -nb_csectors);
> +ret = bdrv_co_preadv(bs->file, coffset, csize, _qiov,
> 0);
I think you should annotate the function with coroutine_fn or use
bdrv_pread() instead.
Berto
Re: [Qemu-block] [Qemu-devel] [PULL 11/15] Bootstrap Python venv for tests
On 6/11/18 14:10, Peter Maydell wrote: On 31 October 2018 at 00:31, Eduardo Habkost wrote: From: Cleber Rosa A number of QEMU tests are written in Python, and may benefit from an untainted Python venv. By using make rules, tests that depend on specific Python libs can set that rule as a requirement, along with rules that require the presence or installation of specific libraries. The tests/requirements.txt is supposed to contain the Python requirements that should be added to the venv created by check-venv. Signed-off-by: Cleber Rosa Tested-by: Philippe Mathieu-Daudé Acked-by: Stefan Hajnoczi Acked-by: Wainer dos Santos Moschetta Reviewed-by: Caio Carrara Message-Id: <20181018153134.8493-2-cr...@redhat.com> Signed-off-by: Eduardo Habkost --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -12,6 +12,7 @@ check-help: @echo " $(MAKE) check-block Run block tests" @echo " $(MAKE) check-tcgRun TCG tests" @echo " $(MAKE) check-report.htmlGenerates an HTML test report" + @echo " $(MAKE) check-venv Creates a Python venv for tests" @echo " $(MAKE) check-clean Clean the tests" @echo @echo "Please note that HTML reports do not regenerate if the unit tests" @@ -899,6 +900,30 @@ check-decodetree: ./check.sh "$(PYTHON)" "$(SRC_PATH)/scripts/decodetree.py", \ TEST, decodetree.py) +# Python venv for running tests + +.PHONY: check-venv + +TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv +TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt + +$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >/dev/null 2>&1) +ifeq ($(.SHELLSTATUS),0) +$(TESTS_VENV_DIR): $(TESTS_VENV_REQ) + $(call quiet-command, \ +$(PYTHON) -m venv --system-site-packages $@, \ +VENV, $@) + $(call quiet-command, \ +$(TESTS_VENV_DIR)/bin/python -m pip -q install -r $(TESTS_VENV_REQ), \ +PIP, $(TESTS_VENV_REQ)) + $(call quiet-command, touch $@) +else +$(TESTS_VENV_DIR): + $(error "venv directory for tests requires Python 3") +endif + +check-venv: $(TESTS_VENV_DIR) Hi -- this seems to be causing one of the travis configs to fail: https://travis-ci.org/qemu/qemu/jobs/451311466 The config includes "--python=/usr/bin/python3", but the build fails with CHK version_gen.h /home/travis/build/qemu/qemu/tests/Makefile.include:928: *** "venv directory for tests requires Python 3". Stop. Would you mind having a look at what's happening there ? I tested the failure using 'make check-venv PYTHON=python2' and the success using 'make check-venv PYTHON=python3' but didn't think of the default... The quicker fix is to force PYTHON in .travis.yml, I'll prepare a patch. thanks -- PMM
Re: [Qemu-block] [PULL 11/15] Bootstrap Python venv for tests
On 31 October 2018 at 00:31, Eduardo Habkost wrote: > From: Cleber Rosa > > A number of QEMU tests are written in Python, and may benefit > from an untainted Python venv. > > By using make rules, tests that depend on specific Python libs > can set that rule as a requirement, along with rules that require > the presence or installation of specific libraries. > > The tests/requirements.txt is supposed to contain the Python > requirements that should be added to the venv created by check-venv. > > Signed-off-by: Cleber Rosa > Tested-by: Philippe Mathieu-Daudé > Acked-by: Stefan Hajnoczi > Acked-by: Wainer dos Santos Moschetta > Reviewed-by: Caio Carrara > Message-Id: <20181018153134.8493-2-cr...@redhat.com> > Signed-off-by: Eduardo Habkost > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -12,6 +12,7 @@ check-help: > @echo " $(MAKE) check-block Run block tests" > @echo " $(MAKE) check-tcgRun TCG tests" > @echo " $(MAKE) check-report.htmlGenerates an HTML test report" > + @echo " $(MAKE) check-venv Creates a Python venv for tests" > @echo " $(MAKE) check-clean Clean the tests" > @echo > @echo "Please note that HTML reports do not regenerate if the unit > tests" > @@ -899,6 +900,30 @@ check-decodetree: >./check.sh "$(PYTHON)" "$(SRC_PATH)/scripts/decodetree.py", \ >TEST, decodetree.py) > > +# Python venv for running tests > + > +.PHONY: check-venv > + > +TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv > +TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt > + > +$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' > >/dev/null 2>&1) > +ifeq ($(.SHELLSTATUS),0) > +$(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > + $(call quiet-command, \ > +$(PYTHON) -m venv --system-site-packages $@, \ > +VENV, $@) > + $(call quiet-command, \ > +$(TESTS_VENV_DIR)/bin/python -m pip -q install -r > $(TESTS_VENV_REQ), \ > +PIP, $(TESTS_VENV_REQ)) > + $(call quiet-command, touch $@) > +else > +$(TESTS_VENV_DIR): > + $(error "venv directory for tests requires Python 3") > +endif > + > +check-venv: $(TESTS_VENV_DIR) Hi -- this seems to be causing one of the travis configs to fail: https://travis-ci.org/qemu/qemu/jobs/451311466 The config includes "--python=/usr/bin/python3", but the build fails with CHK version_gen.h /home/travis/build/qemu/qemu/tests/Makefile.include:928: *** "venv directory for tests requires Python 3". Stop. Would you mind having a look at what's happening there ? thanks -- PMM
Re: [Qemu-block] [PATCH v3] file-posix: Use error API properly
Am 01.11.2018 um 07:29 hat Fam Zheng geschrieben: > Use error_report for situations that affect user operation (i.e. we're > actually returning error), and warn_report/warn_report_err when some > less critical error happened but the user operation can still carry on. > > For raw_normalize_devicepath, add Error parameter to propagate to > its callers. > > Suggested-by: Markus Armbruster > Signed-off-by: Fam Zheng Thanks, applied to the block branch. Kevin
[Qemu-block] [PATCH] block/nvme: call blk_drain in NVMe reset code to avoid lockups
When blk_flush called in NVMe reset path S/C queues are already freed
which means that re-entering AIO handling loop having some IO requests
unfinished will lockup or crash as their SG structures being potentially
reused. Call blk_drain before freeing the queues to avoid this nasty
scenario.
Signed-off-by: Igor Druzhinin
---
hw/block/nvme.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb..cdf836e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -797,6 +797,8 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
{
int i;
+blk_drain(n->conf.blk);
+
for (i = 0; i < n->num_queues; i++) {
if (n->sq[i] != NULL) {
nvme_free_sq(n->sq[i], n);
--
2.7.4
Re: [Qemu-block] [PATCH 1/2] The discard flag for block stream operation
Berto,
Well noted about the "after implementation".
Kindly,
Andrey Shinkevich
On 05.11.2018 19:08, Alberto Garcia wrote:
> On Wed 31 Oct 2018 05:47:19 PM CET, Andrey Shinkevich
> wrote:
>> Adding a parameter to QMP block-stream command to allow discarding
>> blocks in the backing chain while blocks are being copied to the
>> active layer.
>>
>> Signed-off-by: Andrey Shinkevich
> I haven't checked the other patch yet, but if you're going to add new
> API (this new 'discard' parameter) I suggest you to do it _after_ the
> implementation.
>
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2329,6 +2329,9 @@
>> #
>> # @speed: the maximum speed, in bytes per second
>> #
>> +# @discard: true to delete blocks duplicated in old backing files.
>> +# (default: false). Since 3.1.
>> +#
>> # @on-error: the action to take on an error (default report).
>> #'stop' and 'enospc' can only be used if the block device
>> #supports io-status (see BlockInfo). Since 1.3.
>> @@ -2361,7 +2364,7 @@
>> { 'command': 'block-stream',
>> 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>> '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
>> -'*on-error': 'BlockdevOnError',
>> +'*discard': 'bool', '*on-error': 'BlockdevOnError',
>> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
> Berto
Re: [Qemu-block] [PATCH 1/2] The discard flag for block stream operation
OK, David,
I will implement that with the next series.
Kindly,
Andrey Shinkevich
On 31.10.2018 20:38, Dr. David Alan Gilbert wrote:
> * Andrey Shinkevich (andrey.shinkev...@virtuozzo.com) wrote:
>> Adding a parameter to QMP block-stream command to allow discarding
>> blocks in the backing chain while blocks are being copied to the
>> active layer.
>>
>> Signed-off-by: Andrey Shinkevich
>> ---
>> block/stream.c| 2 +-
>> blockdev.c| 8 +++-
>> hmp-commands.hx | 4 ++--
>> hmp.c | 4 +++-
>> include/block/block_int.h | 2 +-
>> qapi/block-core.json | 5 -
>> 6 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 81a7ec8..db81df4 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -221,7 +221,7 @@ static const BlockJobDriver stream_job_driver = {
>>
>> void stream_start(const char *job_id, BlockDriverState *bs,
>> BlockDriverState *base, const char *backing_file_str,
>> - int creation_flags, int64_t speed,
>> + int creation_flags, int64_t speed, bool discard,
>> BlockdevOnError on_error, Error **errp)
>> {
>> StreamBlockJob *s;
>> diff --git a/blockdev.c b/blockdev.c
>> index 574adbc..04aecf5 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3122,6 +3122,7 @@ void qmp_block_stream(bool has_job_id, const char
>> *job_id, const char *device,
>> bool has_base_node, const char *base_node,
>> bool has_backing_file, const char *backing_file,
>> bool has_speed, int64_t speed,
>> + bool has_discard, bool discard,
>> bool has_on_error, BlockdevOnError on_error,
>> bool has_auto_finalize, bool auto_finalize,
>> bool has_auto_dismiss, bool auto_dismiss,
>> @@ -3138,6 +3139,10 @@ void qmp_block_stream(bool has_job_id, const char
>> *job_id, const char *device,
>> on_error = BLOCKDEV_ON_ERROR_REPORT;
>> }
>>
>> +if (!has_discard) {
>> +discard = false;
>> +}
>> +
>> bs = bdrv_lookup_bs(device, device, errp);
>> if (!bs) {
>> return;
>> @@ -3202,7 +3207,8 @@ void qmp_block_stream(bool has_job_id, const char
>> *job_id, const char *device,
>> }
>>
>> stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>> - job_flags, has_speed ? speed : 0, on_error, _err);
>> + job_flags, has_speed ? speed : 0,
>> + discard, on_error, _err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> goto out;
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index db0c681..b455e0d 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -95,8 +95,8 @@ ETEXI
>>
>> {
>> .name = "block_stream",
>> -.args_type = "device:B,speed:o?,base:s?",
>> -.params = "device [speed [base]]",
>> +.args_type = "device:B,speed:o?,base:s?,discard:o?",
> I think that 'o?' is wrong - see the table at the top of monitor.c, 'o'
> is octets, ? is optional - so speed here is an optional byte count, I
> think your 'discard' is just an optional flag.
> So I think you'd be better with a flag, like the -f on block_job_cancel.
>
>> +.params = "device [speed [base]] [discard]",
>> .help = "copy data from a backing file into a block device",
>> .cmd= hmp_block_stream,
>> },
>> diff --git a/hmp.c b/hmp.c
>> index 7828f93..c63e806 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1920,9 +1920,11 @@ void hmp_block_stream(Monitor *mon, const QDict
>> *qdict)
>> const char *device = qdict_get_str(qdict, "device");
>> const char *base = qdict_get_try_str(qdict, "base");
>> int64_t speed = qdict_get_try_int(qdict, "speed", 0);
>> +bool discard = qdict_get_try_bool(qdict, "discard", false);
>>
>> qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
>> - false, NULL, qdict_haskey(qdict, "speed"), speed, true,
>> + false, NULL, qdict_haskey(qdict, "speed"), speed,
>> + qdict_haskey(qdict, "discard"), discard, true,
> Since you've got the default 'false' on the bool discard = above, I
> wonder if that can just be true, discard, true?
>
> Dave
>
>>BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
>>);
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 92ecbd8..e531d03 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -970,7 +970,7 @@ int is_windows_drive(const char *filename);
>>*/
>> void stream_start(const char *job_id, BlockDriverState *bs,
>>
Re: [Qemu-block] [PATCH v2 1/1] include: Add a comment to explain the origin of sizes' lookup table
Am 06.11.2018 um 09:56 hat Leonid Bloch geschrieben:
> Hi Phil, Hi Eric,
>
> (Eric, for some reason you weren't CC'd to this thread - sorry.)
>
> On 11/5/18 5:58 PM, Philippe Mathieu-Daudé wrote:
> > Hi Leonid,
> >
> > On 4/11/18 19:07, Leonid Bloch wrote:
> >> The lookup table for power-of-two sizes was added in commit 540b8492618eb
> >> for the purpose of having convenient shortcuts for these sizes in cases
> >> when the literal number has to be present at compile time, and
> >> expressions as '(1 * KiB)' can not be used. One such case is the
> >> stringification of sizes. Beyond that, it is convenient to use these
> >> shortcuts for all power-of-two sizes, even if they don't have to be
> >> literal numbers.
> >>
> >> Despite its convenience, this table introduced 55 lines of "dumb" code,
> >> the purpose and origin of which are obscure without reading the message
> >> of the commit which introduced it. This patch fixes that by adding a
> >> comment to the code itself with a brief explanation for the reasoning
> >> behind this table. This comment includes the short AWK script that
> >> generated the table, so that anyone who's interested could make sure
> >> that the values in it are correct (otherwise these values look as if
> >> they were typed manually).
> >>
> >> Signed-off-by: Leonid Bloch
> >> ---
> >> include/qemu/units.h | 18 ++
> >> 1 file changed, 18 insertions(+)
> >>
> >> diff --git a/include/qemu/units.h b/include/qemu/units.h
> >> index 68a7758650..1c959d182e 100644
> >> --- a/include/qemu/units.h
> >> +++ b/include/qemu/units.h
> >> @@ -17,6 +17,24 @@
> >> #define PiB (INT64_C(1) << 50)
> >> #define EiB (INT64_C(1) << 60)
> >> +/*
> >> + * The following lookup table is intended to be used when a literal
> >> string of
> >> + * the number of bytes is required (for example if it needs to be
> >> stringified).
> >> + * It can also be used for generic shortcuts of power-of-two sizes.
> >
> > ^ ok
> >
> > v this part is not useful in the tree IMHO.
> >
> >> + * This table is generated using the AWK script below:
> >> + *
> >> + * BEGIN {
> >> + * suffix="KMGTPE";
> >> + * for(i=10; i<64; i++) {
> >> + * val=2**i;
> >> + * s=substr(suffix, int(i/10), 1);
> >> + * n=2**(i%10);
> >> + * pad=21-int(log(n)/log(10));
> >> + * printf("#define S_%d%siB %*d\n", n, s, pad, val);
> >> + * }
> >> + * }
> >> + */
> >
> > If it is generated and the stringified are also generated, why not
> > generate this once via the ./configure, since it is used by machines and
> > not by humans?
>
> You beat me to it! I was just thinking about that last evening. Indeed
> it's not so elegant to put generated code in a file that is intended for
> human handling. Generating by ./configure would be prettier.
We can still do this on top (and probably only for 3.2 as we're in
freeze now and it's neither a bug fix nor a documentation improvement or
test).
> A runtime solution that interprets hard-coded expression strings, like
> Eric suggested, can also be a possibility, but it seems more complicated
> than just generating these at the configuration stage. Plus one gets the
> S_* constants that can be used in many places, not only where an
> explicit number is needed. What do you think?
I think this is not a good use of our time when we want to use QAPI in
the long run anyway.
> A question though: if it to be generated by ./configure, where do you
> suggest to put the generated values?
We already generate a lot of files, you could check where these end up.
But I suppose it might just be the root of the build directory (unless
an include/ exists there?)
> And also: is it OK to assume there's AWK (or another scripting
> language) on the system for generating these?
git grep says that configure already calls awk, so it's probably okay.
Of course, the existing test would just disable libgcrypt instead of
completely failing, so it's still a bit different.
As for other scripting languages, configure itself is written in shell,
and we also assume that Python is available (used e.g. by the QAPI
generators). At least these two are safe, too.
Kevin
Re: [Qemu-block] [PATCH 4/7] qcow2: refactor decompress_buffer
On Thu 01 Nov 2018 07:27:35 PM CET, Vladimir Sementsov-Ogievskiy wrote: > - make it look more like a pair of qcow2_compress - rename the function > and its parameters > - drop extra out_len variable, check filling of output buffer by strm > structure itself > - fix code style > - add some documentation > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [PATCH v2 1/1] include: Add a comment to explain the origin of sizes' lookup table
Hi Phil, Hi Eric,
(Eric, for some reason you weren't CC'd to this thread - sorry.)
On 11/5/18 5:58 PM, Philippe Mathieu-Daudé wrote:
> Hi Leonid,
>
> On 4/11/18 19:07, Leonid Bloch wrote:
>> The lookup table for power-of-two sizes was added in commit 540b8492618eb
>> for the purpose of having convenient shortcuts for these sizes in cases
>> when the literal number has to be present at compile time, and
>> expressions as '(1 * KiB)' can not be used. One such case is the
>> stringification of sizes. Beyond that, it is convenient to use these
>> shortcuts for all power-of-two sizes, even if they don't have to be
>> literal numbers.
>>
>> Despite its convenience, this table introduced 55 lines of "dumb" code,
>> the purpose and origin of which are obscure without reading the message
>> of the commit which introduced it. This patch fixes that by adding a
>> comment to the code itself with a brief explanation for the reasoning
>> behind this table. This comment includes the short AWK script that
>> generated the table, so that anyone who's interested could make sure
>> that the values in it are correct (otherwise these values look as if
>> they were typed manually).
>>
>> Signed-off-by: Leonid Bloch
>> ---
>> include/qemu/units.h | 18 ++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/include/qemu/units.h b/include/qemu/units.h
>> index 68a7758650..1c959d182e 100644
>> --- a/include/qemu/units.h
>> +++ b/include/qemu/units.h
>> @@ -17,6 +17,24 @@
>> #define PiB (INT64_C(1) << 50)
>> #define EiB (INT64_C(1) << 60)
>> +/*
>> + * The following lookup table is intended to be used when a literal
>> string of
>> + * the number of bytes is required (for example if it needs to be
>> stringified).
>> + * It can also be used for generic shortcuts of power-of-two sizes.
>
> ^ ok
>
> v this part is not useful in the tree IMHO.
>
>> + * This table is generated using the AWK script below:
>> + *
>> + * BEGIN {
>> + * suffix="KMGTPE";
>> + * for(i=10; i<64; i++) {
>> + * val=2**i;
>> + * s=substr(suffix, int(i/10), 1);
>> + * n=2**(i%10);
>> + * pad=21-int(log(n)/log(10));
>> + * printf("#define S_%d%siB %*d\n", n, s, pad, val);
>> + * }
>> + * }
>> + */
>
> If it is generated and the stringified are also generated, why not
> generate this once via the ./configure, since it is used by machines and
> not by humans?
You beat me to it! I was just thinking about that last evening. Indeed
it's not so elegant to put generated code in a file that is intended for
human handling. Generating by ./configure would be prettier.
A runtime solution that interprets hard-coded expression strings, like
Eric suggested, can also be a possibility, but it seems more complicated
than just generating these at the configuration stage. Plus one gets the
S_* constants that can be used in many places, not only where an
explicit number is needed. What do you think?
A question though: if it to be generated by ./configure, where do you
suggest to put the generated values? And also: is it OK to assume
there's AWK (or another scripting language) on the system for generating
these?
Thanks,
Leonid.
