Re: [Qemu-block] [PATCH v7 08/38] libqos: Track QTestState with QPCIBus
On 11.09.2017 19:19, Eric Blake wrote: > When initializing a QPCIBus, track which QTestState the bus is > associated with (so that a later patch can then explicitly use > that test state for all communication on the bus, rather than > blindly relying on global_qtest). Update the initialization > functions to take another parameter, and update all callers to > pass in state (for now, most callers get away with passing the > current global_qtest as the current state, although this required > fixing the order of initialization to ensure qtest_start() is > called before qpci_init*() in rtl8139-test, and provided an > opportunity to pass in the allocator in e1000e-test). > > Touch up some allocations to use g_new0() rather than g_malloc() > while in the area, and simplify some code (all implementations > of QOSOps provide a .init_allocator() that never fails). > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 09/38] libqos: Track QTestState with QVirtioBus
On 11.09.2017 19:19, Eric Blake wrote: > When initializing a QVirtioDevice (which always has an associated > QVirtioBus), we want to track which QTestState to use for all > I/O processed through that bus and device. Copy the paradigm > used for QPCIBus, and track the test state at the bus level; this > in turn requires a separate bus object per device (and associated > cleanup) rather than just sharing a const version of the dispatch > table. I fail to see why we need a separate bus object here for each device. The bus is only available one time, not multiple times, isn't it? So there should also only be one bus object floating around, not multiple ones... or do I miss something? Thomas
Re: [Qemu-block] [PATCH v7 10/38] libqos: Move/rename qpci_unplug_acpi_device_test() to pci.c
On 11.09.2017 19:19, Eric Blake wrote:
> Commit 2f8b2767 originally added qpci_plug_device_test() and
> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c.
> Later, commit cf716b31 moved one half of the pair to pci.c
> when adding PPC64 support. Keep the implementations of the
> two functions together, and shorten the name to
> qpci_unplug_device_test(), since all callers use the two
> functions in tandem.
>
> Signed-off-by: Eric Blake
> ---
> tests/libqos/pci.h | 2 +-
> tests/e1000e-test.c | 2 +-
> tests/ivshmem-test.c| 2 +-
> tests/libqos/pci-pc.c | 23 ---
> tests/libqos/pci.c | 23 +++
> tests/virtio-blk-test.c | 2 +-
> tests/virtio-net-test.c | 2 +-
> tests/virtio-rng-test.c | 2 +-
> 8 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index 429c382282..fdda7eca6e 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -111,5 +111,5 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
>
> void qpci_plug_device_test(const char *driver, const char *id,
> uint8_t slot, const char *opts);
> -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
> +void qpci_unplug_device_test(const char *id, uint8_t slot);
> #endif
> diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> index d8085d944e..4c663a3019 100644
> --- a/tests/e1000e-test.c
> +++ b/tests/e1000e-test.c
> @@ -461,7 +461,7 @@ static void test_e1000e_hotplug(gconstpointer data)
> qtest_start("-device e1000e");
>
> qpci_plug_device_test("e1000e", "e1000e_net", slot, NULL);
> -qpci_unplug_acpi_device_test("e1000e_net", slot);
> +qpci_unplug_device_test("e1000e_net", slot);
>
> qtest_end();
> }
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index 37763425ee..8c9ed6a568 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -427,7 +427,7 @@ static void test_ivshmem_hotplug(void)
>
> qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
> if (strcmp(arch, "ppc64") != 0) {
> -qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
> +qpci_unplug_device_test("iv1", PCI_SLOT_HP);
> }
>
> qtest_end();
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index e267fd1a44..6305d142a5 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -19,9 +19,6 @@
> #include "qemu-common.h"
>
>
> -#define ACPI_PCIHP_ADDR 0xae00
> -#define PCI_EJ_BASE 0x0008
> -
> typedef struct QPCIBusPC
> {
> QPCIBus bus;
> @@ -156,23 +153,3 @@ void qpci_free_pc(QPCIBus *bus)
>
> g_free(s);
> }
> -
> -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> -{
> -QDict *response;
> -char *cmd;
> -
> -cmd = g_strdup_printf("{'execute': 'device_del',"
> - " 'arguments': {"
> - " 'id': '%s'"
> - "}}", id);
> -response = qmp(cmd);
> -g_free(cmd);
> -g_assert(response);
> -g_assert(!qdict_haskey(response, "error"));
> -QDECREF(response);
> -
> -outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
> -
> -qmp_eventwait("DEVICE_DELETED");
> -}
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 2dcdeade2a..9f36ec73ef 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -16,6 +16,9 @@
> #include "hw/pci/pci_regs.h"
> #include "qemu/host-utils.h"
>
> +#define ACPI_PCIHP_ADDR 0xae00
> +#define PCI_EJ_BASE 0x0008
> +
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void
> *data),
> void *data)
> @@ -412,3 +415,23 @@ void qpci_plug_device_test(const char *driver, const
> char *id,
> g_assert(!qdict_haskey(response, "error"));
> QDECREF(response);
> }
> +
> +void qpci_unplug_device_test(const char *id, uint8_t slot)
> +{
> +QDict *response;
> +char *cmd;
> +
> +cmd = g_strdup_printf("{'execute': 'device_del',"
> + " 'arguments': {"
> + " 'id': '%s'"
> + "}}", id);
> +response = qmp(cmd);
> +g_free(cmd);
> +g_assert(response);
> +g_assert(!qdict_haskey(response, "error"));
> +QDECREF(response);
> +
> +outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
> +
> +qmp_eventwait("DEVICE_DELETED");
> +}
No, that's a bad idea. ACPI and that outb() is clearly something
specific to x86, so this should not reside in pci.c but in pci-pc.c
We might be able to unify this - I've had a similar patch here:
https://patchwork.kernel.org/patch/9905031/
... but I think this needs some more careful thinking and discussion, so
I'd suggest that you remove this from your already huge patch series for
now and we fix it later instead.
Thomas
Re: [Qemu-block] [PATCH v7 12/38] libqos: Use explicit QTestState for virtio operations
On 11.09.2017 19:19, Eric Blake wrote: > Now that QVirtioDevice and QVirtQueue point back to QVirtioBus, > we can reuse the explicit QTestState stored there rather than > relying on implicit global_qtest. We also have to pass QTestState > through a few functions that can't trace back through > QVirtioDevice, and update those callers. > > Drop some useless casts while touching things. > > Signed-off-by: Eric Blake > --- > tests/libqos/virtio.h | 6 ++-- > tests/libqos/virtio-mmio.c | 57 ++- > tests/libqos/virtio-pci.c | 8 ++--- > tests/libqos/virtio.c | 84 > ++ > tests/virtio-blk-test.c| 11 +++--- > 5 files changed, 94 insertions(+), 72 deletions(-) Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 13/38] libqos: Use explicit QTestState for fw_cfg operations
On 11.09.2017 19:19, Eric Blake wrote: > Drop one more client of global_qtest by teaching all fw_cfg test > functionality (invoked through alloc-pc) to pass in an explicit > QTestState, adjusting all callers. In particular, fw_cfg-test > had to reorder things to create the test state prior to creating > the fw_cfg (and drop a pointless strdup in the meantime), but that > test now no longer depends on global_qtest. > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 16/38] libqos: Use explicit QTestState for ahci operations
On 11.09.2017 19:20, Eric Blake wrote: > Drop one more client of global_qtest by teaching all ahci test > functionality to pass in an explicit QTestState. The state was > already available, so no callers had to be adjusted. > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] [PATCH v7 26/38] libqtest: Merge qtest_end() into qtest_quit()
On 11.09.2017 19:20, Eric Blake wrote: > Rather than have two similar shutdown functions, where one requires > the use of global_qtest in the header, it is better to have a single > shutdown function that still takes care of cleaning up global_qtest > if it is set. All callers are updated. > > Signed-off-by: Eric Blake Reviewed-by: Thomas Huth
Re: [Qemu-block] loading bitmaps in invalidate_cache fails
Am 11.09.2017 um 18:51 hat Vladimir Sementsov-Ogievskiy geschrieben: > Hi Kevin! > > I'm confused with relations of permissions and invalidation, can you please > help? > > Now dirty bitmaps are loaded in invalidate_cache. Here is a problem with > migration: > > 1. destination starts (inactive) > > 2. load bitmaps readonly > > ... > > 3. invalidate_cache: here we should make our loaded bitmaps RW, ie set > BdrvDirtyBitmap->readonly > > to false and set IN_USE bit in the image. But the latter goes into > "bdrv_aligned_pwritev: Assertion `child->perm & BLK_PERM_WRITE' failed", > > because in bdrv_invalidate_cache we call bdrv_set_perm after > drv->bdrv_invalidate_cache. > > > What is the true way of fixing this? It's all still a bit of a mess. :-( I think it makes a lot of sense that we should activate the lower layers first, so the order in bdrv_invalidate_cache() looks wrong. It should be something like this: 1. invalidate_cache() for the children 2. Update permissions for non-BDRV_O_INACTIVE 3. Call drv->bdrv_invalidate_cache() I'm currently working on some fixes related to bdrv_reopen() where things become tricky because the updated permissions shouldn't depend on the current state, but on the state after the operation has finished. You get something similar here, but I think just making sure that we clear BDRV_O_INACTIVE before updating the permissions is enough here. The only thing to be careful is that in error cases, we need to revert both the flag and the permissions then. Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH 01/12] qemu-iotests: remove dead code
Am 10.08.2017 um 00:43 hat Paolo Bonzini geschrieben: > > > - Original Message - > > From: "Eric Blake" > > To: "Paolo Bonzini" , qemu-de...@nongnu.org > > Cc: kw...@redhat.com, qemu-block@nongnu.org > > Sent: Thursday, August 10, 2017 12:18:54 AM > > Subject: Re: [Qemu-devel] [PATCH 01/12] qemu-iotests: remove dead code > > > > On 08/09/2017 04:54 PM, Paolo Bonzini wrote: > > > This includes shell function, shell variables, command line options > > > (randomize.awk does not exist) and conditions that can never be true > > > (./qemu does not exist anymore). > > > > Can we point to a commit id where we stopped making ./qemu? > > commit 9aed1e036dc0de49d08d713f9e5c4655e94acb56 > Author: Anthony Liguori > Date: Mon Aug 29 09:55:36 2011 -0500 > > Rename qemu -> qemu-system-i386 > > This has been discussed before in the past. The special casing really > makes no > sense anymore. This seems like a good change to make for 1.0. > > Signed-off-by: Anthony Liguori This is not related to ./qemu in the qemu-iotests directory. It's just the name of the binary that is created in i386-softmmu/, but that has never been the working directory for qemu-iotests. > > Is it still worth supporting a local symlink? > > Not sure who would have one... I have always been using symlinks in the qemu-iotests directory. And, as you probably expect now, ./qemu does exist in my setup. Now, I must admit that I haven't actually made real use of it recently because the symlinks only point to the binaries that qemu-iotests would pick up anyway. But when running qemu-iotests against a different qemu version or installed binaries instead of whatever is in the build tree, I always found the symlinks more convenient that setting up a bunch of environment variables. So maybe supporting them isn't completely useless. Kevin
Re: [Qemu-block] [PATCH v2 3/4] block: convert crypto driver to bdrv_co_preadv|pwritev
On Thu, Aug 31, 2017 at 10:08:00AM -0500, Eric Blake wrote:
> On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> > Make the crypto driver implement the bdrv_co_preadv|pwritev
> > callbacks, and also use bdrv_co_preadv|pwritev for I/O
> > with the protocol driver beneath.
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > block/crypto.c | 103
> > +
> > 1 file changed, 53 insertions(+), 50 deletions(-)
> >
>
> > static coroutine_fn int
> > -block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> > - int remaining_sectors, QEMUIOVector *qiov)
> > +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t
> > bytes,
> > + QEMUIOVector *qiov, int flags)
>
> > {
> > BlockCrypto *crypto = bs->opaque;
> > -int cur_nr_sectors; /* number of sectors in current iteration */
> > +uint64_t cur_bytes; /* number of bytes in current iteration */
> > uint64_t bytes_done = 0;
> > uint8_t *cipher_data = NULL;
> > QEMUIOVector hd_qiov;
> > int ret = 0;
> > -size_t payload_offset =
> > -qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
> > +size_t payload_offset =
> > qcrypto_block_get_payload_offset(crypto->block);
>
> Pre-existing: is size_t the right type, or can we overflow a 64-bit
> offset on a 32-bit host?
No, it is bad. I'm fixing that as a separate patch, since it is a good
to cleanup.
>
> > +uint64_t sector_num = offset / BDRV_SECTOR_SIZE;
> > +
> > +assert((offset % BDRV_SECTOR_SIZE) == 0);
> > +assert((bytes % BDRV_SECTOR_SIZE) == 0);
>
> The osdep.h macros might be nicer than open-coding; furthermore, if
> desired, you could shorten to:
>
> assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
Yep, makes sense.
> > static coroutine_fn int
> > -block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
> > - int remaining_sectors, QEMUIOVector *qiov)
> > +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t
> > bytes,
> > +QEMUIOVector *qiov, int flags)
> > {
>
> Hmm - you don't set supported_write_flags. But presumably, if the
> underlying BDS supports BDRV_REQUEST_FUA, then crypto can likewise
> support that flag by passing it through to the underlying device after
> encryption.
Something to be added as a separate patch
> > @@ -611,8 +613,9 @@ BlockDriver bdrv_crypto_luks = {
> > .bdrv_truncate = block_crypto_truncate,
> > .create_opts= &block_crypto_create_opts_luks,
> >
> > -.bdrv_co_readv = block_crypto_co_readv,
> > -.bdrv_co_writev = block_crypto_co_writev,
> > +.bdrv_refresh_limits = block_crypto_refresh_limits,
> > +.bdrv_co_preadv = block_crypto_co_preadv,
> > +.bdrv_co_pwritev = block_crypto_co_pwritev,
> > .bdrv_getlength = block_crypto_getlength,
> > .bdrv_get_info = block_crypto_get_info_luks,
> > .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
>
> Looks weird when = isn't consistently aligned, but we use more than one
> space. My preference is to just use one space everywhere, as adding a
> longer name to the list doesn't require a mass re-format of all other
> entries; but I'm not opposed when people like the aligned = for
> legibility. So up to you if you do anything in response to my nit.
Normally I stick with one space, so don't know what I was thinking
when i wrote this.
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] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()
On 11.09.2017 19:20, Eric Blake wrote:
> We have several callers that were formatting the argument strings
> themselves; consolidate this effort by adding new convenience
> functions directly in libqtest, and update all call-sites that
> can benefit from it.
[...]
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index e8c2e11817..b535d7768f 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args)
> return global_qtest = s;
> }
>
> +QTestState *qtest_vstartf(const char *fmt, va_list ap)
> +{
> +char *args = g_strdup_vprintf(fmt, ap);
> +QTestState *s;
> +
> +s = qtest_start(args);
> +global_qtest = NULL;
Don't you need a g_free(args) here?
> +return s;
> +}
> +
> +QTestState *qtest_startf(const char *fmt, ...)
> +{
> +va_list ap;
> +QTestState *s;
> +
> +va_start(ap, fmt);
> +s = qtest_vstartf(fmt, ap);
> +va_end(ap);
> +return s;
> +}
[...]
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index 0c5fcdcc44..12bc526ad6 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -14,16 +14,8 @@
> static void test_device(gconstpointer data)
> {
> const char *model = data;
> -QTestState *s;
> -char *args;
>
> -args = g_strdup_printf("-device %s", model);
> -s = qtest_start(args);
> -
> -if (s) {
> -qtest_quit(s);
> -}
> -g_free(args);
> +qtest_quit(qtest_startf("-device %s", model));
Just my personal taste, but I think I'd be nicer to keep this on
separate lines:
QTestState *s;
s = qtest_startf("-device %s", model);
qtest_quit(s);
> }
[...]
> diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
> index bdc8a67d57..fc9ea84d66 100644
> --- a/tests/eepro100-test.c
> +++ b/tests/eepro100-test.c
> @@ -13,18 +13,9 @@
> static void test_device(gconstpointer data)
> {
> const char *model = data;
> -QTestState *s;
> -char *args;
> -
> -args = g_strdup_printf("-device %s", model);
> -s = qtest_start(args);
>
> /* Tests only initialization so far. TODO: Implement functional tests */
> -
> -if (s) {
> -qtest_quit(s);
> -}
> -g_free(args);
> +qtest_quit(qtest_startf("-device %s", model));
> }
dito
[...]
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index fa21d26935..e2f6c68be8 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -12,20 +12,14 @@
> #include "qemu/osdep.h"
> #include "libqtest.h"
>
> -static char *make_cli(const char *generic_cli, const char *test_cli)
> -{
> -return g_strdup_printf("%s %s", generic_cli ? generic_cli : "",
> test_cli);
> -}
> -
> static void test_mon_explicit(const void *data)
> {
> char *s;
> -char *cli;
> +const char *args = data;
>
> -cli = make_cli(data, "-smp 8 "
> - "-numa node,nodeid=0,cpus=0-3 "
> - "-numa node,nodeid=1,cpus=4-7 ");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 "
> +"-numa node,nodeid=0,cpus=0-3 "
> +"-numa node,nodeid=1,cpus=4-7 ", args);
>
> s = hmp("info numa");
> g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
> @@ -33,16 +27,14 @@ static void test_mon_explicit(const void *data)
> g_free(s);
>
> qtest_quit(global_qtest);
> -g_free(cli);
> }
>
> static void test_mon_default(const void *data)
> {
> char *s;
> -char *cli;
> +const char *args = data;
>
> -cli = make_cli(data, "-smp 8 -numa node -numa node");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 -numa node -numa node", args);
>
> s = hmp("info numa");
> g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
> @@ -50,18 +42,16 @@ static void test_mon_default(const void *data)
> g_free(s);
>
> qtest_quit(global_qtest);
> -g_free(cli);
> }
>
> static void test_mon_partial(const void *data)
> {
> char *s;
> -char *cli;
> +const char *args = data;
>
> -cli = make_cli(data, "-smp 8 "
> - "-numa node,nodeid=0,cpus=0-1 "
> - "-numa node,nodeid=1,cpus=4-5 ");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 "
> +"-numa node,nodeid=0,cpus=0-1 "
> +"-numa node,nodeid=1,cpus=4-5 ", args);
Does GCC emit a warning if you'd used data here directly? Otherwise I
think you could simply do this without the local args variable...
Thomas
Re: [Qemu-block] [PATCH 04/12] qemu-iotests: limit non-_PROG-suffixed variables to common.rc
Am 09.08.2017 um 23:55 hat Paolo Bonzini geschrieben: > These are never used by "check", with one exception that does not need > $QEMU_OPTIONS. Keep them in common.rc, which will be soon included only > by the tests. > > Signed-off-by: Paolo Bonzini > diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common > index 50720f080f..f58e56fc40 100644 > --- a/tests/qemu-iotests/common > +++ b/tests/qemu-iotests/common > @@ -454,11 +454,3 @@ fi > # > list=`sort $tmp.list` > rm -f $tmp.list $tmp.tmp $tmp.sed > - > -[ "$QEMU" = "" ] && _fatal "qemu not found" > -[ "$QEMU_IMG" = "" ] && _fatal "qemu-img not found" > -[ "$QEMU_IO" = "" ] && _fatal "qemu-io not found" > - > -if [ "$IMGPROTO" = "nbd" ] ; then > -[ "$QEMU_NBD" = "" ] && _fatal "qemu-nbd not found" > -fi Hm, does this mean that instead of ./check failing when a binary is missing, we try each test case now and each one fails with the same error message? *tries it out* Okay, it's already broken today because the strings are never empty but contain the name of the wrapper functions, but it's still bad behaviour. Instead of just telling me that the binary is missing like it used to work, I get tons of test case diffs. Kevin
Re: [Qemu-block] [PATCH v7 29/38] libqtest: Merge qtest_init() into qtest_start()
On 11.09.2017 19:20, Eric Blake wrote:
> Remove the trivial wrapper qtest_init(), and change qtest_start()
> to no longer implicitly set global_qtest, to make it obvious in the
> rest of the testsuite where we are still relying on global_qtest.
> Everything now uses qtest_start() (and friends) and qtest_quit(),
> and explicitly tracks the QTestState between the two (although in
> many cases, this tracking is still done through global_qtest).
> Doing this makes it easier to see what remaining cleanups will be
> needed if we don't want an implicit dependency on global state.
[...]
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 817e3a5580..2a21bf4605 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -27,7 +27,7 @@ extern QTestState *global_qtest;
> * qtest_start_without_qmp_handshake:
> * @extra_args: other arguments to pass to QEMU.
> *
> - * Returns: #QTestState instance. Does not affect #global_qtest.
> + * Returns: #QTestState instance, handshaking not yet completed.
I'd rather add a description a la:
* Start QEMU, but do not execute the QMP handshake yet.
*
* Returns: #QTestState instance
> */
> QTestState *qtest_start_without_qmp_handshake(const char *extra_args);
>
> @@ -35,10 +35,7 @@ QTestState *qtest_start_without_qmp_handshake(const char
> *extra_args);
> * qtest_start:
> * @args: other arguments to pass to QEMU
> *
> - * Start QEMU and assign the resulting #QTestState to #global_qtest.
> - * The global variable is used by "shortcut" functions documented below.
> *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.
I'd rather change the description instead of removing it:
* Start QEMU and execute the initial QMP handshake
*
* Returns: #QTestState instance.
> */
> QTestState *qtest_start(const char *args);
>
> @@ -47,10 +44,7 @@ QTestState *qtest_start(const char *args);
> * @fmt...: Format for creating other arguments to pass to QEMU, formatted
> * like sprintf().
> *
> - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(),
> - * #global_qtest is left at NULL).
> - *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.
dito
> */
> QTestState *qtest_startf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>
> @@ -60,30 +54,11 @@ QTestState *qtest_startf(const char *fmt, ...)
> GCC_FMT_ATTR(1, 2);
> * like vsprintf().
> * @ap: Format arguments.
> *
> - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(),
> - * #global_qtest is left at NULL).
> - *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.
dito
> */
> QTestState *qtest_vstartf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>
> /**
> - * qtest_init:
> - * @extra_args: other arguments to pass to QEMU.
> - *
> - * Returns: #QTestState instance. Does not affect #global_qtest.
> - */
> -static inline QTestState *qtest_init(const char *extra_args)
> -{
> -QTestState *s;
> -
> -assert(!global_qtest);
> -s = qtest_start(extra_args);
> -global_qtest = NULL;
> -return s;
> -}
[...]
> diff --git a/tests/display-vga-test.c b/tests/display-vga-test.c
> index 8667330e3c..d638f15ec3 100644
> --- a/tests/display-vga-test.c
> +++ b/tests/display-vga-test.c
> @@ -12,39 +12,33 @@
>
> static void pci_cirrus(void)
> {
> -qtest_start("-vga none -device cirrus-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device cirrus-vga"));
I'd prefer to keep this on separate lines ... but that's again just my
personal taste. (also for the othe changes below)
> }
>
> static void pci_stdvga(void)
> {
> -qtest_start("-vga none -device VGA");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device VGA"));
> }
>
> static void pci_secondary(void)
> {
> -qtest_start("-vga none -device secondary-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device secondary-vga"));
> }
>
> static void pci_multihead(void)
> {
> -qtest_start("-vga none -device VGA -device secondary-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device VGA -device secondary-vga"));
> }
>
> static void pci_virtio_gpu(void)
> {
> -qtest_start("-vga none -device virtio-gpu-pci");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device virtio-gpu-pci"));
> }
>
> #ifdef CONFIG_VIRTIO_VGA
> static void pci_virtio_vga(void)
> {
> -qtest_start("-vga none -device virtio-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device virtio-vga"));
> }
> #endif
[...]
> diff --git a/tests/ne2000-test.c b/tests/ne2000-test.c
> index cae83c5c4c..8e6f7b07c6 100644
> --- a/tests/ne2000-test.c
> +++ b/tests/ne2000-test.c
> @@ -22,7 +22,7 @@ int main(int argc, char **argv)
> g_test_init(&argc, &argv, NULL);
> qt
Re: [Qemu-block] [PATCH 07/12] qemu-iotests: disintegrate more parts of common.config
Am 09.08.2017 um 23:55 hat Paolo Bonzini geschrieben: > Split "check" parts from tests part. > > For the directory setup, the actual computation of directories goes > in "check", while the sanity checks go in the tests. > > Signed-off-by: Paolo Bonzini Same comment as for patch 1, we may want to keep this working. Kevin
Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_clock_set() > qtest_clock_step() > qtest_clock_step_next() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake > --- > tests/libqtest.h | 50 > tests/libqtest.c | 6 ++-- > tests/e1000e-test.c | 2 +- > tests/fdc-test.c | 4 +-- > tests/ide-test.c | 2 +- > tests/libqos/virtio.c| 8 +++--- > tests/rtc-test.c | 74 > > tests/rtl8139-test.c | 10 +++ > tests/tco-test.c | 22 +++--- > tests/test-arm-mptimer.c | 25 +--- > tests/wdt_ib700-test.c | 12 > 11 files changed, 90 insertions(+), 125 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 5651b77d2f..26d5f37bc9 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -417,17 +417,17 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, > void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size); > > /** > - * qtest_clock_step_next: > + * clock_step_next: > * @s: #QTestState instance to operate on. > * > * Advance the QEMU_CLOCK_VIRTUAL to the next deadline. > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_step_next(QTestState *s); > +int64_t clock_step_next(QTestState *s); > > /** > - * qtest_clock_step: > + * clock_step: > * @s: QTestState instance to operate on. > * @step: Number of nanoseconds to advance the clock by. > * > @@ -435,10 +435,10 @@ int64_t qtest_clock_step_next(QTestState *s); > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_step(QTestState *s, int64_t step); > +int64_t clock_step(QTestState *s, int64_t step); > > /** > - * qtest_clock_set: > + * clock_set: > * @s: QTestState instance to operate on. > * @val: Nanoseconds value to advance the clock to. > * > @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_set(QTestState *s, int64_t val); > +int64_t clock_set(QTestState *s, int64_t val); Could we please keep the "qtest" prefix here and rather get rid of the other ones? Even if it's more to type, I prefer to have a proper prefix here so that it is clear at the first sight that the functions belong to the qtest framework. Thomas
Re: [Qemu-block] [PATCH v7 32/38] libqtest: Merge qtest_irq*() with irq*()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_get_irq() > qtest_irq_intercept_in() > qtest_irq_intercept_out() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake > --- > tests/libqtest.h | 47 ++- > tests/libqtest.c | 6 +++--- > tests/fdc-test.c | 48 > > tests/ide-test.c | 17 + > tests/ipmi-bt-test.c | 6 +++--- > tests/ipmi-kcs-test.c| 8 > tests/libqos/libqos-pc.c | 2 +- > tests/rtc-test.c | 10 +- > tests/tco-test.c | 2 +- > tests/wdt_ib700-test.c | 8 > 10 files changed, 60 insertions(+), 94 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 26d5f37bc9..8398c0fd07 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -176,33 +176,33 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); > char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap); > > /** > - * qtest_get_irq: > + * get_irq: > * @s: #QTestState instance to operate on. > * @num: Interrupt to observe. > * > * Returns: The level of the @num interrupt. > */ > -bool qtest_get_irq(QTestState *s, int num); > +bool get_irq(QTestState *s, int num); > > /** > - * qtest_irq_intercept_in: > + * irq_intercept_in: > * @s: #QTestState instance to operate on. > * @string: QOM path of a device. > * > * Associate qtest irqs with the GPIO-in pins of the device > * whose path is specified by @string. > */ > -void qtest_irq_intercept_in(QTestState *s, const char *string); > +void irq_intercept_in(QTestState *s, const char *string); > > /** > - * qtest_irq_intercept_out: > + * irq_intercept_out: > * @s: #QTestState instance to operate on. > * @string: QOM path of a device. > * > * Associate qtest irqs with the GPIO-out pins of the device > * whose path is specified by @string. > */ > -void qtest_irq_intercept_out(QTestState *s, const char *string); > +void irq_intercept_out(QTestState *s, const char *string); Could we please also keep the qtest prefix here? Thomas
Re: [Qemu-block] [PATCH v7 33/38] libqtest: Merge qtest_{in, out}[bwl]() with {in, out}[bwl]()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_outb() > qtest_outw() > qtest_outl() > qtest_inb() > qtest_inw() > qtest_inl() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake > --- > tests/libqtest.h| 99 > ++--- > tests/multiboot/libc.h | 2 +- > tests/libqtest.c| 14 +++ > tests/boot-order-test.c | 4 +- > tests/endianness-test.c | 12 +++--- > tests/fdc-test.c| 77 -- > tests/hd-geo-test.c | 4 +- > tests/ipmi-bt-test.c| 12 +++--- > tests/ipmi-kcs-test.c | 8 ++-- > tests/libqos/fw_cfg.c | 4 +- > tests/libqos/pci-pc.c | 44 +++--- > tests/libqos/pci.c | 2 +- > tests/m48t59-test.c | 8 ++-- > tests/multiboot/libc.c | 2 +- > tests/pvpanic-test.c| 4 +- > tests/rtc-test.c| 8 ++-- > tests/wdt_ib700-test.c | 8 ++-- > 17 files changed, 120 insertions(+), 192 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 8398c0fd07..520f745e7b 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -205,61 +205,61 @@ void irq_intercept_in(QTestState *s, const char > *string); > void irq_intercept_out(QTestState *s, const char *string); > > /** > - * qtest_outb: > + * outb: > * @s: #QTestState instance to operate on. > * @addr: I/O port to write to. > * @value: Value being written. > * > * Write an 8-bit value to an I/O port. > */ > -void qtest_outb(QTestState *s, uint16_t addr, uint8_t value); > +void outb(QTestState *s, uint16_t addr, uint8_t value); Could we please also keep the qtest prefix here? ... same applies for all your other following "Merge ..." patches in this series... Thomas
[Qemu-block] [PATCH v3 0/7] Misc improvements to crypto block driver
This is a followup to v1: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00781.html v2: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06464.html This collection of patches first improves the performance of the crypto block driver and then does various cleanups to improve ongoing maint work. Changed in v3: - Support passthrough of BDRV_REQ_FUA (Eric) - Fix potential truncation of payload offset values (Eric) - Use encryption scheme sector size instead of BDRV_SECTOR_SIZE (Kevin) - Use QEMU_IS_ALIGNED where appropriate (Eric) - Remove unused 'sector_num' variable (Eric) - Fix whitespace alignment (Eric) - Fix math error in qcow conversion (Eric) Daniel P. Berrange (7): block: use 1 MB bounce buffers for crypto instead of 16KB crypto: expose encryption sector size in APIs block: fix data type casting for crypto payload offset block: don't use constant 512 as sector size in crypto driver block: convert crypto driver to bdrv_co_preadv|pwritev block: convert qcrypto_block_encrypt|decrypt to take bytes offset block: support passthrough of BDRV_REQ_FUA in crypto driver block/crypto.c | 134 +++-- block/qcow.c | 7 ++- block/qcow2-cluster.c | 8 ++- block/qcow2.c | 4 +- crypto/block-luks.c| 18 --- crypto/block-qcow.c| 13 +++-- crypto/block.c | 26 +++--- crypto/blockpriv.h | 5 +- include/crypto/block.h | 29 --- 9 files changed, 150 insertions(+), 94 deletions(-) -- 2.13.5
[Qemu-block] [PATCH v3 3/7] block: fix data type casting for crypto payload offset
The crypto APIs report the offset of the data payload as an uint64_t
type, but the block driver is casting to size_t or ssize_t which will
potentially truncate.
Most of the block APIs use int64_t for offsets meanwhile, so even if
using uint64_t in the crypto block driver we are still at risk of
truncation.
Change the block crypto driver to use uint64_t, but add asserts that
the value is less than INT64_MAX.
Signed-off-by: Daniel P. Berrange
---
block/crypto.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index cc8afe0e0d..d68cbac2ac 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -364,8 +364,9 @@ static int block_crypto_truncate(BlockDriverState *bs,
int64_t offset,
PreallocMode prealloc, Error **errp)
{
BlockCrypto *crypto = bs->opaque;
-size_t payload_offset =
+uint64_t payload_offset =
qcrypto_block_get_payload_offset(crypto->block);
+assert(payload_offset < (INT64_MAX - offset));
offset += payload_offset;
@@ -391,8 +392,9 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
uint8_t *cipher_data = NULL;
QEMUIOVector hd_qiov;
int ret = 0;
-size_t payload_offset =
+uint64_t payload_offset =
qcrypto_block_get_payload_offset(crypto->block) / 512;
+assert(payload_offset < (INT64_MAX / 512));
qemu_iovec_init(&hd_qiov, qiov->niov);
@@ -458,8 +460,9 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t
sector_num,
uint8_t *cipher_data = NULL;
QEMUIOVector hd_qiov;
int ret = 0;
-size_t payload_offset =
+uint64_t payload_offset =
qcrypto_block_get_payload_offset(crypto->block) / 512;
+assert(payload_offset < (INT64_MAX / 512));
qemu_iovec_init(&hd_qiov, qiov->niov);
@@ -520,7 +523,9 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
BlockCrypto *crypto = bs->opaque;
int64_t len = bdrv_getlength(bs->file->bs);
-ssize_t offset = qcrypto_block_get_payload_offset(crypto->block);
+uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
+assert(offset < INT64_MAX);
+assert(offset < len);
len -= offset;
--
2.13.5
[Qemu-block] [PATCH v3 5/7] block: convert crypto driver to bdrv_co_preadv|pwritev
Make the crypto driver implement the bdrv_co_preadv|pwritev
callbacks, and also use bdrv_co_preadv|pwritev for I/O
with the protocol driver beneath.
Signed-off-by: Daniel P. Berrange
---
block/crypto.c | 104 +++--
1 file changed, 56 insertions(+), 48 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 49d6d4c058..d004e9cef4 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -383,19 +383,23 @@ static void block_crypto_close(BlockDriverState *bs)
#define BLOCK_CRYPTO_MAX_SECTORS 2048
static coroutine_fn int
-block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
- int remaining_sectors, QEMUIOVector *qiov)
+block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
{
BlockCrypto *crypto = bs->opaque;
-int cur_nr_sectors; /* number of sectors in current iteration */
+uint64_t cur_bytes; /* number of bytes in current iteration */
uint64_t bytes_done = 0;
uint8_t *cipher_data = NULL;
QEMUIOVector hd_qiov;
int ret = 0;
uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
-uint64_t payload_offset =
-qcrypto_block_get_payload_offset(crypto->block) / sector_size;
-assert(payload_offset < (INT64_MAX / 512));
+size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
+uint64_t sector_num = offset / sector_size;
+
+assert(!flags);
+assert(payload_offset < INT64_MAX);
+assert(QEMU_IS_ALIGNED(offset, sector_size));
+assert(QEMU_IS_ALIGNED(bytes, sector_size));
qemu_iovec_init(&hd_qiov, qiov->niov);
@@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
goto cleanup;
}
-while (remaining_sectors) {
-cur_nr_sectors = remaining_sectors;
+while (bytes) {
+cur_bytes = bytes;
-if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
-cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
+if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {
+cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;
}
qemu_iovec_reset(&hd_qiov);
-qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * sector_size);
+qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);
-ret = bdrv_co_readv(bs->file,
-payload_offset + sector_num,
-cur_nr_sectors, &hd_qiov);
+ret = bdrv_co_preadv(bs->file, payload_offset + offset + bytes_done,
+ cur_bytes, &hd_qiov, 0);
if (ret < 0) {
goto cleanup;
}
-if (qcrypto_block_decrypt(crypto->block,
- sector_num,
- cipher_data, cur_nr_sectors * sector_size,
- NULL) < 0) {
+if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
+ cur_bytes, NULL) < 0) {
ret = -EIO;
goto cleanup;
}
-qemu_iovec_from_buf(qiov, bytes_done,
-cipher_data, cur_nr_sectors * sector_size);
+qemu_iovec_from_buf(qiov, bytes_done, cipher_data, cur_bytes);
-remaining_sectors -= cur_nr_sectors;
-sector_num += cur_nr_sectors;
-bytes_done += cur_nr_sectors * sector_size;
+sector_num += cur_bytes / sector_size;
+bytes -= cur_bytes;
+bytes_done += cur_bytes;
}
cleanup:
@@ -452,19 +452,23 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
static coroutine_fn int
-block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
- int remaining_sectors, QEMUIOVector *qiov)
+block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+QEMUIOVector *qiov, int flags)
{
BlockCrypto *crypto = bs->opaque;
-int cur_nr_sectors; /* number of sectors in current iteration */
+uint64_t cur_bytes; /* number of bytes in current iteration */
uint64_t bytes_done = 0;
uint8_t *cipher_data = NULL;
QEMUIOVector hd_qiov;
int ret = 0;
uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
-uint64_t payload_offset =
-qcrypto_block_get_payload_offset(crypto->block) / sector_size;
-assert(payload_offset < (INT64_MAX / 512));
+uint64_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
+uint64_t sector_num = offset / sector_size;
+
+assert(!flags);
+assert(payload_offset < INT64_MAX);
+assert(QEMU_IS_ALIGNED(offset, sector_size));
+assert(QEMU_IS_ALIGNED(bytes, sector_size));
qemu_iovec_init(&hd_qiov, qiov->niov);
@@ -479,37 +483,33 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t
sector_num,
goto cleanup;
}
-while (
[Qemu-block] [PATCH v3 6/7] block: convert qcrypto_block_encrypt|decrypt to take bytes offset
Instead of sector offset, take the bytes offset when encrypting
or decrypting data.
Signed-off-by: Daniel P. Berrange
---
block/crypto.c | 12
block/qcow.c | 7 +--
block/qcow2-cluster.c | 8 +++-
block/qcow2.c | 4 ++--
crypto/block-luks.c| 12
crypto/block-qcow.c| 12
crypto/block.c | 20 ++--
crypto/blockpriv.h | 4 ++--
include/crypto/block.h | 14 --
9 files changed, 54 insertions(+), 39 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index d004e9cef4..1f77095336 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -394,7 +394,6 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
int ret = 0;
uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
-uint64_t sector_num = offset / sector_size;
assert(!flags);
assert(payload_offset < INT64_MAX);
@@ -430,15 +429,14 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
goto cleanup;
}
-if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
- cur_bytes, NULL) < 0) {
+if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
+ cipher_data, cur_bytes, NULL) < 0) {
ret = -EIO;
goto cleanup;
}
qemu_iovec_from_buf(qiov, bytes_done, cipher_data, cur_bytes);
-sector_num += cur_bytes / sector_size;
bytes -= cur_bytes;
bytes_done += cur_bytes;
}
@@ -463,7 +461,6 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
int ret = 0;
uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
uint64_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
-uint64_t sector_num = offset / sector_size;
assert(!flags);
assert(payload_offset < INT64_MAX);
@@ -492,8 +489,8 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
qemu_iovec_to_buf(qiov, bytes_done, cipher_data, cur_bytes);
-if (qcrypto_block_encrypt(crypto->block, sector_num, cipher_data,
- cur_bytes, NULL) < 0) {
+if (qcrypto_block_encrypt(crypto->block, offset + bytes_done,
+ cipher_data, cur_bytes, NULL) < 0) {
ret = -EIO;
goto cleanup;
}
@@ -507,7 +504,6 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
goto cleanup;
}
-sector_num += cur_bytes / sector_size;
bytes -= cur_bytes;
bytes_done += cur_bytes;
}
diff --git a/block/qcow.c b/block/qcow.c
index f450b00cfc..d242301ed2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -478,7 +478,9 @@ static int get_cluster_offset(BlockDriverState *bs,
for(i = 0; i < s->cluster_sectors; i++) {
if (i < n_start || i >= n_end) {
memset(s->cluster_data, 0x00, 512);
-if (qcrypto_block_encrypt(s->crypto, start_sect +
i,
+if (qcrypto_block_encrypt(s->crypto,
+ (start_sect + i) *
+ BDRV_SECTOR_SIZE,
s->cluster_data,
BDRV_SECTOR_SIZE,
NULL) < 0) {
@@ -668,7 +670,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs,
int64_t sector_num,
}
if (bs->encrypted) {
assert(s->crypto);
-if (qcrypto_block_decrypt(s->crypto, sector_num, buf,
+if (qcrypto_block_decrypt(s->crypto,
+ sector_num * BDRV_SECTOR_SIZE, buf,
n * BDRV_SECTOR_SIZE, NULL) < 0) {
ret = -EIO;
break;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 0d4824993c..09ae0b6ecc 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -396,15 +396,13 @@ static bool coroutine_fn
do_perform_cow_encrypt(BlockDriverState *bs,
{
if (bytes && bs->encrypted) {
BDRVQcow2State *s = bs->opaque;
-int64_t sector = (s->crypt_physical_offset ?
+int64_t offset = (s->crypt_physical_offset ?
(cluster_offset + offset_in_cluster) :
- (src_cluster_offset + offset_in_cluster))
- >> BDRV_SECTOR_BITS;
+ (src_cluster_offset + offset_in_cluster));
assert((offset_in_c
[Qemu-block] [PATCH v3 4/7] block: don't use constant 512 as sector size in crypto driver
Use the qcrypto_block_get_sector_size() value in the block
crypto driver instead of hardcoding 512 as the sector size.
Signed-off-by: Daniel P. Berrange
---
block/crypto.c | 34 ++
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index d68cbac2ac..49d6d4c058 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -392,8 +392,9 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
uint8_t *cipher_data = NULL;
QEMUIOVector hd_qiov;
int ret = 0;
+uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
uint64_t payload_offset =
-qcrypto_block_get_payload_offset(crypto->block) / 512;
+qcrypto_block_get_payload_offset(crypto->block) / sector_size;
assert(payload_offset < (INT64_MAX / 512));
qemu_iovec_init(&hd_qiov, qiov->niov);
@@ -401,9 +402,9 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
/* Bounce buffer because we don't wish to expose cipher text
* in qiov which points to guest memory.
*/
-cipher_data =
-qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
- qiov->size));
+cipher_data = qemu_try_blockalign(
+bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * sector_size,
+ qiov->size));
if (cipher_data == NULL) {
ret = -ENOMEM;
goto cleanup;
@@ -417,7 +418,7 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
}
qemu_iovec_reset(&hd_qiov);
-qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
+qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * sector_size);
ret = bdrv_co_readv(bs->file,
payload_offset + sector_num,
@@ -428,18 +429,18 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
sector_num,
if (qcrypto_block_decrypt(crypto->block,
sector_num,
- cipher_data, cur_nr_sectors * 512,
+ cipher_data, cur_nr_sectors * sector_size,
NULL) < 0) {
ret = -EIO;
goto cleanup;
}
qemu_iovec_from_buf(qiov, bytes_done,
-cipher_data, cur_nr_sectors * 512);
+cipher_data, cur_nr_sectors * sector_size);
remaining_sectors -= cur_nr_sectors;
sector_num += cur_nr_sectors;
-bytes_done += cur_nr_sectors * 512;
+bytes_done += cur_nr_sectors * sector_size;
}
cleanup:
@@ -460,8 +461,9 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t
sector_num,
uint8_t *cipher_data = NULL;
QEMUIOVector hd_qiov;
int ret = 0;
+uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
uint64_t payload_offset =
-qcrypto_block_get_payload_offset(crypto->block) / 512;
+qcrypto_block_get_payload_offset(crypto->block) / sector_size;
assert(payload_offset < (INT64_MAX / 512));
qemu_iovec_init(&hd_qiov, qiov->niov);
@@ -469,9 +471,9 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t
sector_num,
/* Bounce buffer because we're not permitted to touch
* contents of qiov - it points to guest memory.
*/
-cipher_data =
-qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
- qiov->size));
+cipher_data = qemu_try_blockalign(
+bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * sector_size,
+ qiov->size));
if (cipher_data == NULL) {
ret = -ENOMEM;
goto cleanup;
@@ -485,18 +487,18 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t
sector_num,
}
qemu_iovec_to_buf(qiov, bytes_done,
- cipher_data, cur_nr_sectors * 512);
+ cipher_data, cur_nr_sectors * sector_size);
if (qcrypto_block_encrypt(crypto->block,
sector_num,
- cipher_data, cur_nr_sectors * 512,
+ cipher_data, cur_nr_sectors * sector_size,
NULL) < 0) {
ret = -EIO;
goto cleanup;
}
qemu_iovec_reset(&hd_qiov);
-qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
+qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * sector_size);
ret = bdrv_co_writev(bs->file,
payload_offset + sector_num,
@@ -507,7 +509,7 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t
sector_num,
remaining_sectors -= cur_nr_sectors;
sector_num += cur_nr_sectors;
-bytes_done += cur_nr_sectors * 512;
+bytes_done += cur_nr_sectors * sector_size;
}
[Qemu-block] [PATCH v3 1/7] block: use 1 MB bounce buffers for crypto instead of 16KB
Using 16KB bounce buffers creates a significant performance penalty for I/O to encrypted volumes on storage which high I/O latency (rotating rust & network drives), because it triggers lots of fairly small I/O operations. On tests with rotating rust, and cache=none|directsync, write speed increased from 2MiB/s to 32MiB/s, on a par with that achieved by the in-kernel luks driver. With other cache modes the in-kernel driver is still notably faster because it is able to report completion of the I/O request before any encryption is done, while the in-QEMU driver must encrypt the data before completion. Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Daniel P. Berrange --- block/crypto.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/block/crypto.c b/block/crypto.c index 58ef6f2f52..cc8afe0e0d 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs) } -#define BLOCK_CRYPTO_MAX_SECTORS 32 +#define BLOCK_CRYPTO_MAX_SECTORS 2048 static coroutine_fn int block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, @@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, qemu_iovec_init(&hd_qiov, qiov->niov); -/* Bounce buffer so we have a linear mem region for - * entire sector. XXX optimize so we avoid bounce - * buffer in case that qiov->niov == 1 +/* Bounce buffer because we don't wish to expose cipher text + * in qiov which points to guest memory. */ cipher_data = qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512, @@ -464,9 +463,8 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num, qemu_iovec_init(&hd_qiov, qiov->niov); -/* Bounce buffer so we have a linear mem region for - * entire sector. XXX optimize so we avoid bounce - * buffer in case that qiov->niov == 1 +/* Bounce buffer because we're not permitted to touch + * contents of qiov - it points to guest memory. */ cipher_data = qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512, -- 2.13.5
[Qemu-block] [PATCH v3 2/7] crypto: expose encryption sector size in APIs
While current encryption schemes all have a fixed sector size of
512 bytes, this is not guaranteed to be the case in future. Expose
the sector size in the APIs so the block layer can remove assumptions
about fixed 512 byte sectors.
Signed-off-by: Daniel P. Berrange
---
crypto/block-luks.c| 6 --
crypto/block-qcow.c| 1 +
crypto/block.c | 6 ++
crypto/blockpriv.h | 1 +
include/crypto/block.h | 15 +++
5 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 36bc856084..a9062bb0f2 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -846,8 +846,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
}
}
+block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
block->payload_offset = luks->header.payload_offset *
-QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+block->sector_size;
luks->cipher_alg = cipheralg;
luks->cipher_mode = ciphermode;
@@ -1240,8 +1241,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
+block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
block->payload_offset = luks->header.payload_offset *
-QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+block->sector_size;
/* Reserve header space to match payload offset */
initfunc(block, block->payload_offset, opaque, &local_err);
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index a456fe338b..4dd594a9ba 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -80,6 +80,7 @@ qcrypto_block_qcow_init(QCryptoBlock *block,
goto fail;
}
+block->sector_size = QCRYPTO_BLOCK_QCOW_SECTOR_SIZE;
block->payload_offset = 0;
return 0;
diff --git a/crypto/block.c b/crypto/block.c
index c382393d9a..a7a9ad240e 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -170,6 +170,12 @@ uint64_t qcrypto_block_get_payload_offset(QCryptoBlock
*block)
}
+uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block)
+{
+return block->sector_size;
+}
+
+
void qcrypto_block_free(QCryptoBlock *block)
{
if (!block) {
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 0edb810e22..d227522d88 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -36,6 +36,7 @@ struct QCryptoBlock {
QCryptoHashAlgorithm kdfhash;
size_t niv;
uint64_t payload_offset; /* In bytes */
+uint64_t sector_size; /* In bytes */
};
struct QCryptoBlockDriver {
diff --git a/include/crypto/block.h b/include/crypto/block.h
index f0e543bee1..13232b2472 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -241,6 +241,21 @@ QCryptoHashAlgorithm
qcrypto_block_get_kdf_hash(QCryptoBlock *block);
uint64_t qcrypto_block_get_payload_offset(QCryptoBlock *block);
/**
+ * qcrypto_block_get_sector_size:
+ * @block: the block encryption object
+ *
+ * Get the size of sectors used for payload encryption. A new
+ * IV is used at the start of each sector. The encryption
+ * sector size is not required to match the sector size of the
+ * underlying storage. For example LUKS will always use a 512
+ * byte sector size, even if the volume is on a disk with 4k
+ * sectors.
+ *
+ * Returns: the sector in bytes
+ */
+uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
+
+/**
* qcrypto_block_free:
* @block: the block encryption object
*
--
2.13.5
[Qemu-block] [PATCH v3 7/7] block: support passthrough of BDRV_REQ_FUA in crypto driver
The BDRV_REQ_FUA flag can trivially be allowed in the crypt driver
as a passthrough to the underlying block driver.
Signed-off-by: Daniel P. Berrange
---
block/crypto.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 1f77095336..d1c28d62c1 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -279,6 +279,9 @@ static int block_crypto_open_generic(QCryptoBlockFormat
format,
return -EINVAL;
}
+bs->supported_write_flags = BDRV_REQ_FUA &
+bs->file->bs->supported_write_flags;
+
opts = qemu_opts_create(opts_spec, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
if (local_err) {
@@ -462,7 +465,7 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
uint64_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
-assert(!flags);
+assert(!(flags & ~BDRV_REQ_FUA));
assert(payload_offset < INT64_MAX);
assert(QEMU_IS_ALIGNED(offset, sector_size));
assert(QEMU_IS_ALIGNED(bytes, sector_size));
@@ -499,7 +502,7 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t
offset, uint64_t bytes,
qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);
ret = bdrv_co_pwritev(bs->file, payload_offset + offset + bytes_done,
- cur_bytes, &hd_qiov, 0);
+ cur_bytes, &hd_qiov, flags);
if (ret < 0) {
goto cleanup;
}
--
2.13.5
Re: [Qemu-block] [PATCH v3 5/7] block: convert crypto driver to bdrv_co_preadv|pwritev
On Tue, Sep 12, 2017 at 12:28:53PM +0100, Daniel P. Berrange wrote:
> Make the crypto driver implement the bdrv_co_preadv|pwritev
> callbacks, and also use bdrv_co_preadv|pwritev for I/O
> with the protocol driver beneath.
>
> Signed-off-by: Daniel P. Berrange
> ---
> block/crypto.c | 104
> +++--
> 1 file changed, 56 insertions(+), 48 deletions(-)
>
> diff --git a/block/crypto.c b/block/crypto.c
> index 49d6d4c058..d004e9cef4 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -383,19 +383,23 @@ static void block_crypto_close(BlockDriverState *bs)
> #define BLOCK_CRYPTO_MAX_SECTORS 2048
>
> static coroutine_fn int
> -block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> - int remaining_sectors, QEMUIOVector *qiov)
> +block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> + QEMUIOVector *qiov, int flags)
> {
> BlockCrypto *crypto = bs->opaque;
> -int cur_nr_sectors; /* number of sectors in current iteration */
> +uint64_t cur_bytes; /* number of bytes in current iteration */
> uint64_t bytes_done = 0;
> uint8_t *cipher_data = NULL;
> QEMUIOVector hd_qiov;
> int ret = 0;
> uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
> -uint64_t payload_offset =
> -qcrypto_block_get_payload_offset(crypto->block) / sector_size;
> -assert(payload_offset < (INT64_MAX / 512));
> +size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
Opps, rebase merge error - that should be uint64_t - this is what the
patchew failure complained about.
> +uint64_t sector_num = offset / sector_size;
> +
> +assert(!flags);
> +assert(payload_offset < INT64_MAX);
> +assert(QEMU_IS_ALIGNED(offset, sector_size));
> +assert(QEMU_IS_ALIGNED(bytes, sector_size));
>
> qemu_iovec_init(&hd_qiov, qiov->niov);
>
> @@ -410,37 +414,33 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
> sector_num,
> goto cleanup;
> }
>
> -while (remaining_sectors) {
> -cur_nr_sectors = remaining_sectors;
> +while (bytes) {
> +cur_bytes = bytes;
>
> -if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
> -cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
> +if (cur_bytes > (BLOCK_CRYPTO_MAX_SECTORS * sector_size)) {
> +cur_bytes = BLOCK_CRYPTO_MAX_SECTORS * sector_size;
> }
>
> qemu_iovec_reset(&hd_qiov);
> -qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * sector_size);
> +qemu_iovec_add(&hd_qiov, cipher_data, cur_bytes);
>
> -ret = bdrv_co_readv(bs->file,
> -payload_offset + sector_num,
> -cur_nr_sectors, &hd_qiov);
> +ret = bdrv_co_preadv(bs->file, payload_offset + offset + bytes_done,
> + cur_bytes, &hd_qiov, 0);
> if (ret < 0) {
> goto cleanup;
> }
>
> -if (qcrypto_block_decrypt(crypto->block,
> - sector_num,
> - cipher_data, cur_nr_sectors * sector_size,
> - NULL) < 0) {
> +if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
> + cur_bytes, NULL) < 0) {
> ret = -EIO;
> goto cleanup;
> }
>
> -qemu_iovec_from_buf(qiov, bytes_done,
> -cipher_data, cur_nr_sectors * sector_size);
> +qemu_iovec_from_buf(qiov, bytes_done, cipher_data, cur_bytes);
>
> -remaining_sectors -= cur_nr_sectors;
> -sector_num += cur_nr_sectors;
> -bytes_done += cur_nr_sectors * sector_size;
> +sector_num += cur_bytes / sector_size;
> +bytes -= cur_bytes;
> +bytes_done += cur_bytes;
> }
>
> cleanup:
> @@ -452,19 +452,23 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t
> sector_num,
>
>
> static coroutine_fn int
> -block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
> - int remaining_sectors, QEMUIOVector *qiov)
> +block_crypto_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t
> bytes,
> +QEMUIOVector *qiov, int flags)
> {
> BlockCrypto *crypto = bs->opaque;
> -int cur_nr_sectors; /* number of sectors in current iteration */
> +uint64_t cur_bytes; /* number of bytes in current iteration */
> uint64_t bytes_done = 0;
> uint8_t *cipher_data = NULL;
> QEMUIOVector hd_qiov;
> int ret = 0;
> uint64_t sector_size = qcrypto_block_get_sector_size(crypto->block);
> -uint64_t payload_offset =
> -qcrypto_block_get_payload_offset(crypto->block) / sector_size;
> -assert(payload_offset < (INT64_MAX / 512));
> +uint64_t payload_offset =
> qcrypto_block_get_payload_
Re: [Qemu-block] [PATCH 04/12] qemu-iotests: limit non-_PROG-suffixed variables to common.rc
On 12/09/2017 12:31, Kevin Wolf wrote: > Hm, does this mean that instead of ./check failing when a binary is > missing, we try each test case now and each one fails with the same > error message? > > *tries it out* > > Okay, it's already broken today because the strings are never empty but > contain the name of the wrapper functions, but it's still bad behaviour. > Instead of just telling me that the binary is missing like it used to > work, I get tons of test case diffs. So the patch is still dead code, isn't it? Paolo
Re: [Qemu-block] [PATCH 07/12] qemu-iotests: disintegrate more parts of common.config
On 12/09/2017 12:40, Kevin Wolf wrote: > Am 09.08.2017 um 23:55 hat Paolo Bonzini geschrieben: >> Split "check" parts from tests part. >> >> For the directory setup, the actual computation of directories goes >> in "check", while the sanity checks go in the tests. >> >> Signed-off-by: Paolo Bonzini > > Same comment as for patch 1, we may want to keep this working. Can you explain what is the use case? For local symlinks I (sort of) understand it, but not here. This as far as I understand is code that never runs. My preferred alternatives would be one of these: - add a patch 13 that restores the local symlink feature on top of the cleaned up code. - later, rewrite "check" in Python now that it is clear what code is part of it and what code is part of the tests. or: - leave local symlinks broken - when "check" is rewritten in Python, add a configuration mechanism based on .ini file syntax that replaces the local symlinks. What do you think? Paolo
Re: [Qemu-block] [PATCH 04/12] qemu-iotests: limit non-_PROG-suffixed variables to common.rc
Am 12.09.2017 um 14:28 hat Paolo Bonzini geschrieben: > On 12/09/2017 12:31, Kevin Wolf wrote: > > Hm, does this mean that instead of ./check failing when a binary is > > missing, we try each test case now and each one fails with the same > > error message? > > > > *tries it out* > > > > Okay, it's already broken today because the strings are never empty but > > contain the name of the wrapper functions, but it's still bad behaviour. > > Instead of just telling me that the binary is missing like it used to > > work, I get tons of test case diffs. > > So the patch is still dead code, isn't it? Yes. But instead of moving it to a place where this ugly failure mode becomes intentional, we should just fix the check and keep doing it once at the start of ./check. Kevin
Re: [Qemu-block] [PATCH 04/12] qemu-iotests: limit non-_PROG-suffixed variables to common.rc
On 12/09/2017 14:51, Kevin Wolf wrote: > Am 12.09.2017 um 14:28 hat Paolo Bonzini geschrieben: >> On 12/09/2017 12:31, Kevin Wolf wrote: >>> Hm, does this mean that instead of ./check failing when a binary is >>> missing, we try each test case now and each one fails with the same >>> error message? >>> >>> *tries it out* >>> >>> Okay, it's already broken today because the strings are never empty but >>> contain the name of the wrapper functions, but it's still bad behaviour. >>> Instead of just telling me that the binary is missing like it used to >>> work, I get tons of test case diffs. >> >> So the patch is still dead code, isn't it? > > Yes. But instead of moving it to a place where this ugly failure mode > becomes intentional, we should just fix the check and keep doing it once > at the start of ./check. Ok, that is better indeed. Paolo
Re: [Qemu-block] [PATCH 07/12] qemu-iotests: disintegrate more parts of common.config
Am 12.09.2017 um 14:31 hat Paolo Bonzini geschrieben:
> On 12/09/2017 12:40, Kevin Wolf wrote:
> > Am 09.08.2017 um 23:55 hat Paolo Bonzini geschrieben:
> >> Split "check" parts from tests part.
> >>
> >> For the directory setup, the actual computation of directories goes
> >> in "check", while the sanity checks go in the tests.
> >>
> >> Signed-off-by: Paolo Bonzini
> >
> > Same comment as for patch 1, we may want to keep this working.
>
> Can you explain what is the use case? For local symlinks I (sort of)
> understand it, but not here. This as far as I understand is code that
> never runs.
Sorry, somehow I replied to the wrong patch...
This was meant as a comment for patch 9 ('do not search for binaries in
the current directory').
> My preferred alternatives would be one of these:
>
> - add a patch 13 that restores the local symlink feature on top of the
> cleaned up code.
>
> - later, rewrite "check" in Python now that it is clear what code is
> part of it and what code is part of the tests.
Do you really think that removing and the reintroducing the feature is
easier than just keeping it in the first place?
> or:
>
> - leave local symlinks broken
>
> - when "check" is rewritten in Python, add a configuration mechanism
> based on .ini file syntax that replaces the local symlinks.
>
> What do you think?
I don't mind the solution as long as after the series, it is still
working. I think this means option 1.
Kevin
Re: [Qemu-block] [PATCH 07/12] qemu-iotests: disintegrate more parts of common.config
On 12/09/2017 14:57, Kevin Wolf wrote:
> Am 12.09.2017 um 14:31 hat Paolo Bonzini geschrieben:
>> On 12/09/2017 12:40, Kevin Wolf wrote:
>>> Am 09.08.2017 um 23:55 hat Paolo Bonzini geschrieben:
Split "check" parts from tests part.
For the directory setup, the actual computation of directories goes
in "check", while the sanity checks go in the tests.
Signed-off-by: Paolo Bonzini
>>>
>>> Same comment as for patch 1, we may want to keep this working.
>>
>> Can you explain what is the use case? For local symlinks I (sort of)
>> understand it, but not here. This as far as I understand is code that
>> never runs.
>
> Sorry, somehow I replied to the wrong patch...
>
> This was meant as a comment for patch 9 ('do not search for binaries in
> the current directory').
>
>> My preferred alternatives would be one of these:
>>
>> - add a patch 13 that restores the local symlink feature on top of the
>> cleaned up code.
>>
>> - later, rewrite "check" in Python now that it is clear what code is
>> part of it and what code is part of the tests.
>
> Do you really think that removing and the reintroducing the feature is
> easier than just keeping it in the first place?
Nah, sorry, I was confused -- I thought the feature was in some common.*
file, but it's straight in "check". I'll get rid of patch 9 and the
'./qemu' hunk of patch 1.
Paolo
Re: [Qemu-block] [PATCH v7 09/38] libqos: Track QTestState with QVirtioBus
On 09/12/2017 02:21 AM, Thomas Huth wrote: > On 11.09.2017 19:19, Eric Blake wrote: >> When initializing a QVirtioDevice (which always has an associated >> QVirtioBus), we want to track which QTestState to use for all >> I/O processed through that bus and device. Copy the paradigm >> used for QPCIBus, and track the test state at the bus level; this >> in turn requires a separate bus object per device (and associated >> cleanup) rather than just sharing a const version of the dispatch >> table. > I fail to see why we need a separate bus object here for each device. > The bus is only available one time, not multiple times, isn't it? So > there should also only be one bus object floating around, not multiple > ones... or do I miss something? You are right that there is only one bus for the machine - the problem is that the object representing the bus is dynamically created on the fly at the point where the device is first grabbed, rather than up front as part of creating the machine (in other words, the device search is not performed on a pre-existing bus object). Contrast qpci_device_find() (lookup based on a pre-existing bus) with qvirtio_mmio_init_device() (lookup that did not use a pre-existing bus); also note that qvirtio_pci_device_find() takes a pre-existing QPCIBus (since on that architecture, the QVirtioBus is itself a device on the QPCIBus). I suppose that we could indeed refactor the code to require callers to create the QVirtioBus object up front, and then make the device lookup (both qvirtio_mmio_init_device() and qvirtio_pci_device_find()) take a QVirtioBus parameter instead of QTestState or QPCIBus. That's slightly more work, but I'm happy to attempt it if you think it will be better. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v7 10/38] libqos: Move/rename qpci_unplug_acpi_device_test() to pci.c
On 09/12/2017 02:29 AM, Thomas Huth wrote: > On 11.09.2017 19:19, Eric Blake wrote: >> Commit 2f8b2767 originally added qpci_plug_device_test() and >> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c. >> Later, commit cf716b31 moved one half of the pair to pci.c >> when adding PPC64 support. Keep the implementations of the >> two functions together, and shorten the name to >> qpci_unplug_device_test(), since all callers use the two >> functions in tandem. >> > > No, that's a bad idea. ACPI and that outb() is clearly something > specific to x86, so this should not reside in pci.c but in pci-pc.c > > We might be able to unify this - I've had a similar patch here: > > https://patchwork.kernel.org/patch/9905031/ > > ... but I think this needs some more careful thinking and discussion, so > I'd suggest that you remove this from your already huge patch series for > now and we fix it later instead. Okay, I'm fine dropping this patch, and can base my respin on top of your cleanup instead. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()
On 09/12/2017 05:14 AM, Thomas Huth wrote:
> On 11.09.2017 19:20, Eric Blake wrote:
>> We have several callers that were formatting the argument strings
>> themselves; consolidate this effort by adding new convenience
>> functions directly in libqtest, and update all call-sites that
>> can benefit from it.
> [...]
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index e8c2e11817..b535d7768f 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args)
>> return global_qtest = s;
>> }
>>
>> +QTestState *qtest_vstartf(const char *fmt, va_list ap)
>> +{
>> +char *args = g_strdup_vprintf(fmt, ap);
>> +QTestState *s;
>> +
>> +s = qtest_start(args);
>> +global_qtest = NULL;
>
> Don't you need a g_free(args) here?
D'oh. Yes, I do.
>> +qtest_quit(qtest_startf("-device %s", model));
>
> Just my personal taste, but I think I'd be nicer to keep this on
> separate lines:
>
> QTestState *s;
>
> s = qtest_startf("-device %s", model);
> qtest_quit(s);
Sure. I debated about it. If we ever do more than just create/destroy,
then having the separate lines makes it easier to stick the actual test
in between the two lines, so I'll avoid my abbreviation and go with the
longer form on respin.
>> static void test_mon_partial(const void *data)
>> {
>> char *s;
>> -char *cli;
>> +const char *args = data;
>>
>> -cli = make_cli(data, "-smp 8 "
>> - "-numa node,nodeid=0,cpus=0-1 "
>> - "-numa node,nodeid=1,cpus=4-5 ");
>> -qtest_start(cli);
>> +global_qtest = qtest_startf("%s -smp 8 "
>> +"-numa node,nodeid=0,cpus=0-1 "
>> +"-numa node,nodeid=1,cpus=4-5 ", args);
>
> Does GCC emit a warning if you'd used data here directly? Otherwise I
> think you could simply do this without the local args variable...
Passing void* through varargs, with the intent of the receiver parsing
it as char*, is technically undefined in C. I don't know if gcc warns,
but I'm also worried that clang might warn. I prefer to err on the side
of defined behavior in this case, even though it annoyingly requires a
temporary variable.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
On 09/12/2017 05:45 AM, Thomas Huth wrote: > On 11.09.2017 19:20, Eric Blake wrote: >> Maintaining two layers of libqtest APIs, one that takes an explicit >> QTestState object, and the other that uses the implicit global_qtest, >> is annoying. In the interest of getting rid of global implicit >> state and having less code to maintain, merge: >> qtest_clock_set() >> qtest_clock_step() >> qtest_clock_step_next() >> with their short counterparts. All callers that previously >> used the short form now make it explicit that they are relying on >> global_qtest, and later patches can then clean things up to remove >> the global variable. >> >> @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); >> * >> * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. >> */ >> -int64_t qtest_clock_set(QTestState *s, int64_t val); >> +int64_t clock_set(QTestState *s, int64_t val); > Could we please keep the "qtest" prefix here and rather get rid of the > other ones? Even if it's more to type, I prefer to have a proper prefix > here so that it is clear at the first sight that the functions belong to > the qtest framework. I suppose we can, although it makes more lines that are likely to bump up against 80 columns, and thus slightly more churn to reformat things to keep checkpatch happy. I like the shorter name, because less typing is easier to remember. I'd prefer a second opinion on naming before doing anything about it though - Markus or Paolo, do you have any preference? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH v2 00/10] cleanup qemu-iotests
The purpose of this series is to separate the "check" sources from the tests. After these patches, common.config is reduced to simple shell initialization, and common.rc is only included by the tests. Along the way, a lot of dead code is removed too. In v2, the following patches: qemu-iotests: do not do useless search for QEMU_*_PROG qemu-iotests: do not search for binaries in the current directory qemu-iotests: include common.env and common.config early have been replaced by "qemu-iotests: cleanup and fix search for programs", which also preserves the behavior of searching for programs as symlinks in the current directory. Paolo Paolo Bonzini (10): qemu-iotests: remove dead code qemu-iotests: get rid of AWK_PROG qemu-iotests: move "check" code out of common.rc qemu-iotests: cleanup and fix search for programs qemu-iotests: limit non-_PROG-suffixed variables to common.rc qemu-iotests: do not include common.rc in "check" qemu-iotests: disintegrate more parts of common.config qemu-iotests: fix uninitialized variable qemu-iotests: get rid of $iam qemu-iotests: merge "check" and "common" tests/qemu-iotests/039.out | 10 +- tests/qemu-iotests/061.out | 4 +- tests/qemu-iotests/137.out | 2 +- tests/qemu-iotests/check | 575 ++- tests/qemu-iotests/common| 459 --- tests/qemu-iotests/common.config | 206 +- tests/qemu-iotests/common.qemu | 1 + tests/qemu-iotests/common.rc | 205 +++--- 8 files changed, 621 insertions(+), 841 deletions(-) delete mode 100644 tests/qemu-iotests/common -- 2.13.5
[Qemu-block] [PATCH 02/10] qemu-iotests: get rid of AWK_PROG
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 4 ++--
tests/qemu-iotests/common| 2 +-
tests/qemu-iotests/common.config | 3 ---
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4a6ed67b42..5c0d0c51dc 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -128,7 +128,7 @@ tmp="${TEST_DIR}"/$$
_wallclock()
{
-date "+%H %M %S" | $AWK_PROG '{ print $1*3600 + $2*60 + $3 }'
+date "+%H %M %S" | awk '{ print $1*3600 + $2*60 + $3 }'
}
_timestamp()
@@ -147,7 +147,7 @@ _wrapup()
if [ -f $TIMESTAMP_FILE -a -f $tmp.time ]
then
cat $TIMESTAMP_FILE $tmp.time \
-| $AWK_PROG '
+| awk '
{ t[$1] = $2 }
END{ if (NR > 0) {
for (i in t) print i " " t[i]
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 867918895b..130f647a4d 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -366,7 +366,7 @@ testlist options
if $xpand
then
have_test_arg=true
-$AWK_PROG
[Qemu-block] [PATCH 04/10] qemu-iotests: cleanup and fix search for programs
Instead of ./check failing when a binary is missing, we try each test
case now and each one fails with tons of test case diffs. Also, all the
variables were initialized by "check" prior to "common" being sourced,
and then (uselessly) checked for emptiness again in "check".
Centralize the search for programs in "common" (which will soon be
one with "check"), including the "realpath" invocation which can be done
just once in "check" rather than in the tests.
For qnio_server, move the detection to "common", simplifying
set_prog_path to stop handling the unused second argument, and
embedding the "realpath" pass.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 41 -
tests/qemu-iotests/common| 77 +---
tests/qemu-iotests/common.config | 61 +--
3 files changed, 73 insertions(+), 106 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 5c0d0c51dc..b7c54390d3 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -60,47 +60,6 @@ fi
build_root="$build_iotests/../.."
-if [ -x "$build_iotests/socket_scm_helper" ]
-then
-export SOCKET_SCM_HELPER="$build_iotests/socket_scm_helper"
-fi
-
-if [[ -z "$QEMU_PROG" && ! -x './qemu' ]]
-then
-arch=$(uname -m 2> /dev/null)
-
-if [[ -n $arch && -x "$build_root/$arch-softmmu/qemu-system-$arch" ]]
-then
-export QEMU_PROG="$build_root/$arch-softmmu/qemu-system-$arch"
-else
-pushd "$build_root" > /dev/null
-for binary in *-softmmu/qemu-system-*
-do
-if [ -x "$binary" ]
-then
-export QEMU_PROG="$build_root/$binary"
-break
-fi
-done
-popd > /dev/null
-fi
-fi
-
-if [[ -z $QEMU_IMG_PROG && -x "$build_root/qemu-img" && ! -x './qemu-img' ]]
-then
-export QEMU_IMG_PROG="$build_root/qemu-img"
-fi
-
-if [[ -z $QEMU_IO_PROG && -x "$build_root/qemu-io" && ! -x './qemu-io' ]]
-then
-export QEMU_IO_PROG="$build_root/qemu-io"
-fi
-
-if [[ -z $QEMU_NBD_PROG && -x "$build_root/qemu-nbd" && ! -x './qemu-nbd' ]]
-then
-export QEMU_NBD_PROG="$build_root/qemu-nbd"
-fi
-
# we need common.env
if ! . "$build_iotests/common.env"
then
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 2e98e64d5c..abacc24114 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -37,6 +37,17 @@ _full_platform_details()
echo "$os/$platform $host $kernel"
}
+# $1 = prog to look for
+set_prog_path()
+{
+p=`command -v $1 2> /dev/null`
+if [ -n "$p" -a -x "$p" ]; then
+realpath -- "$(type -p "$p")"
+else
+return 1
+fi
+}
+
diff="diff -u"
verbose=false
debug=false
@@ -450,10 +461,66 @@ fi
list=`sort $tmp.list`
rm -f $tmp.list $tmp.tmp $tmp.sed
-[ "$QEMU" = "" ] && _fatal "qemu not found"
-[ "$QEMU_IMG" = "" ] && _fatal "qemu-img not found"
-[ "$QEMU_IO" = "" ] && _fatal "qemu-io not found"
+if [ -z "$QEMU_PROG" ]
+then
+if [ -x "$build_iotests/qemu" ]; then
+export QEMU_PROG="$build_iotests/qemu"
+elif [ -x "$build_root/$arch-softmmu/qemu-system-$arch" ]; then
+export QEMU_PROG="$build_root/$arch-softmmu/qemu-system-$arch"
+else
+pushd "$build_root" > /dev/null
+for binary in *-softmmu/qemu-system-*
+do
+if [ -x "$binary" ]
+then
+export QEMU_PROG="$build_root/$binary"
+break
+fi
+done
+popd > /dev/null
+[ "$QEMU_PROG" = "" ] && _init_error "qemu not found"
+fi
+fi
+export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")
+
+if [ -z "$QEMU_IMG_PROG" ]; then
+if [ -x "$build_iotests/qemu-img" ]; then
+export QEMU_IMG_PROG="$build_iotests/qemu-img"
+elif [ -x "$build_root/qemu-img" ]; then
+export QEMU_IMG_PROG="$build_root/qemu-img"
+else
+_init_error "qemu-img not found"
+fi
+fi
+export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
+
+if [ -z "$QEMU_IO_PROG" ]; then
+if [ -x "$build_iotests/qemu-io" ]; then
+export QEMU_IO_PROG="$build_iotests/qemu-io"
+elif [ -x "$build_root/qemu-io" ]; then
+export QEMU_IO_PROG="$build_root/qemu-io"
+else
+_init_error "qemu-io not found"
+fi
+fi
+export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
-if [ "$IMGPROTO" = "nbd" ] ; then
-[ "$QEMU_NBD" = "" ] && _fatal "qemu-nbd not found"
+if [ -z $QEMU_NBD_PROG ]; then
+if [ -x "$build_iotests/qemu-nbd" ]; then
+export QEMU_NBD_PROG="$build_iotests/qemu-nbd"
+elif [ -x "$build_root/qemu-nbd" ]; then
+export QEMU_NBD_PROG="$build_root/qemu-nbd"
+else
+_init_error "qemu-nbd not found"
+fi
+fi
+export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
+
+if [ -z "$QEMU_VXHS_PROG" ]; then
+ export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
[Qemu-block] [PATCH 01/10] qemu-iotests: remove dead code
This includes shell function, shell variables and command line options
(randomize.awk does not exist).
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 28 -
tests/qemu-iotests/common| 23 --
tests/qemu-iotests/common.config | 26 ---
tests/qemu-iotests/common.rc | 68
4 files changed, 145 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d504b6e455..4a6ed67b42 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -65,7 +65,6 @@ then
export SOCKET_SCM_HELPER="$build_iotests/socket_scm_helper"
fi
-# if ./qemu exists, it should be prioritized and will be chosen by
common.config
if [[ -z "$QEMU_PROG" && ! -x './qemu' ]]
then
arch=$(uname -m 2> /dev/null)
@@ -140,12 +139,6 @@ _timestamp()
_wrapup()
{
-# for hangcheck ...
-# remove files that were used by hangcheck
-#
-[ -f "${TEST_DIR}"/check.pid ] && rm -rf "${TEST_DIR}"/check.pid
-[ -f "${TEST_DIR}"/check.sts ] && rm -rf "${TEST_DIR}"/check.sts
-
if $showme
then
:
@@ -201,24 +194,6 @@ END{ if (NR > 0) {
trap "_wrapup; exit \$status" 0 1 2 3 15
-# for hangcheck ...
-# Save pid of check in a well known place, so that hangcheck can be sure it
-# has the right pid (getting the pid from ps output is not reliable enough).
-#
-rm -rf "${TEST_DIR}"/check.pid
-echo $$ > "${TEST_DIR}"/check.pid
-
-# for hangcheck ...
-# Save the status of check in a well known place, so that hangcheck can be
-# sure to know where check is up to (getting test number from ps output is
-# not reliable enough since the trace stuff has been introduced).
-#
-rm -rf "${TEST_DIR}"/check.sts
-echo "preamble" > "${TEST_DIR}"/check.sts
-
-# don't leave old full output behind on a clean run
-rm -f check.full
-
[ -f $TIMESTAMP_FILE ] || touch $TIMESTAMP_FILE
FULL_IMGFMT_DETAILS=`_full_imgfmt_details`
@@ -276,9 +251,6 @@ do
fi
rm -f core $seq.notrun
-# for hangcheck ...
-echo "$seq" > "${TEST_DIR}"/check.sts
-
start=`_wallclock`
$timestamp && printf %s "[$(date "+%T")]"
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index d34c11c056..867918895b 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -19,17 +19,6 @@
# common procedures for QA scripts
#
-_setenvironment()
-{
-MSGVERB="text:action"
-export MSGVERB
-}
-
-rm -f "$OUTPUT_DIR/$iam.out"
-_setenvironment
-
-check=${check-true}
-
diff="diff -u"
verbose=false
debug=false
@@ -40,7 +29,6 @@ showme=false
sortme=false
expunge=true
have_test_arg=false
-randomize=false
cachemode=false
rm -f $tmp.list $tmp.tmp $tmp.sed
@@ -170,7 +158,6 @@ other options
-n show me, do not run tests
-o options -o options to pass to qemu-img create/convert
-T output timestamps
--r randomize test order
-c mode cache mode
testlist options
@@ -327,11 +314,6 @@ testlist options
cachemode=true
xpand=false
;;
--r)# randomize test order
-randomize=true
-xpand=false
-;;
-
-T)# turn on timestamp output
timestamp=true
xpand=false
@@ -445,11 +427,6 @@ fi
list=`sort $tmp.list`
rm -f $tmp.list $tmp.tmp $tmp.sed
-if $randomize
-then
-list=`echo $list | awk -f randomize.awk`
-fi
-
[ "$QEMU" = "" ] && _fatal "qemu not found"
[ "$QEMU_IMG" = "" ] && _fatal "qemu-img not found"
[ "$QEMU_IO" = "" ] && _fatal "qemu-io not found"
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index e0883a0c65..b599c72211 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -15,33 +15,14 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see
[Qemu-block] [PATCH 03/10] qemu-iotests: move "check" code out of common.rc
Some functions in common.rc are never used by the tests. Move
them out of that file and into common, which is already included
only by "check".
Code that actually *is* common to "check" and tests can be placed in
common.config.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/common| 25 -
tests/qemu-iotests/common.config | 12
tests/qemu-iotests/common.rc | 40
3 files changed, 36 insertions(+), 41 deletions(-)
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 130f647a4d..2e98e64d5c 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -19,6 +19,24 @@
# common procedures for QA scripts
#
+_full_imgfmt_details()
+{
+if [ -n "$IMGOPTS" ]; then
+echo "$IMGFMT ($IMGOPTS)"
+else
+echo "$IMGFMT"
+fi
+}
+
+_full_platform_details()
+{
+os=`uname -s`
+host=`hostname -s`
+kernel=`uname -r`
+platform=`uname -m`
+echo "$os/$platform $host $kernel"
+}
+
diff="diff -u"
verbose=false
debug=false
@@ -404,7 +422,12 @@ if [ "$IMGOPTSSYNTAX" != "true" ]; then
fi
# Set default options for qemu-img create -o if they were not specified
-_set_default_imgopts
+if [ "$IMGFMT" == "qcow2" ] && ! (echo "$IMGOPTS" | grep "compat=" >
/dev/null); then
+IMGOPTS=$(_optstr_add "$IMGOPTS" "compat=1.1")
+fi
+if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep "iter-time=" >
/dev/null); then
+IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10")
+fi
if [ -s $tmp.list ]
then
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 0f571d46eb..91da65f3dc 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -27,6 +27,9 @@ export PWD=`pwd`
export _QEMU_HANDLE=0
+# make sure we have a standard umask
+umask 022
+
# $1 = prog to look for, $2* = default pathnames if not found in $PATH
set_prog_path()
{
@@ -49,6 +52,15 @@ set_prog_path()
return 1
}
+_optstr_add()
+{
+if [ -n "$1" ]; then
+echo "$1,$2"
+else
+echo "$2"
+fi
+}
+
_fatal()
{
echo "$*"
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 5938d5145f..6f6e03366f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -50,9 +50,6 @@ then
fi
fi
-# make sure we have a standard umask
-umask 022
-
if [ "$IMGOPTSSYNTAX" = "true" ]; then
DRIVER="driver=$IMGFMT"
if [ "$IMGFMT" = "luks" ]; then
@@ -94,25 +91,6 @@ else
fi
ORIG_TEST_IMG="$TEST_IMG"
-_optstr_add()
-{
-if [ -n "$1" ]; then
-echo "$1,$2"
-else
-echo "$2"
-fi
-}
-
-_set_default_imgopts()
-{
-if [ "$IMGFMT" == "qcow2" ] && ! (echo "$IMGOPTS" | grep "compat=" >
/dev/null); then
-IMGOPTS=$(_optstr_add "$IMGOPTS" "compat=1.1")
-fi
-if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep "iter-time=" >
/dev/null); then
-IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10")
-fi
-}
-
_use_sample_img()
{
SAMPLE_IMG_FILE="${1%\.bz2}"
@@ -428,23 +406,5 @@ _require_command()
[ -x "$c" ] || _notrun "$1 utility required, skipped this test"
}
-_full_imgfmt_details()
-{
-if [ -n "$IMGOPTS" ]; then
-echo "$IMGFMT ($IMGOPTS)"
-else
-echo "$IMGFMT"
-fi
-}
-
-_full_platform_details()
-{
-os=`uname -s`
-host=`hostname -s`
-kernel=`uname -r`
-platform=`uname -m`
-echo "$os/$platform $host $kernel"
-}
-
# make sure this script returns success
true
--
2.13.5
[Qemu-block] [PATCH 07/10] qemu-iotests: disintegrate more parts of common.config
Split "check" parts from tests part.
For the directory setup, the actual computation of directories goes
in "check", while the sanity checks go in the tests.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/common| 24
tests/qemu-iotests/common.config | 49
tests/qemu-iotests/common.qemu | 1 +
tests/qemu-iotests/common.rc | 25
4 files changed, 50 insertions(+), 49 deletions(-)
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index abacc24114..ee313af92f 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -48,6 +48,14 @@ set_prog_path()
fi
}
+if [ -z "$TEST_DIR" ]; then
+TEST_DIR=`pwd`/scratch
+fi
+
+if [ ! -e "$TEST_DIR" ]; then
+mkdir "$TEST_DIR"
+fi
+
diff="diff -u"
verbose=false
debug=false
@@ -440,6 +448,13 @@ if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep
"iter-time=" > /dev/null
IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10")
fi
+if [ -z "$SAMPLE_IMG_DIR" ]; then
+SAMPLE_IMG_DIR="$source_iotests/sample_images"
+fi
+
+export TEST_DIR
+export SAMPLE_IMG_DIR
+
if [ -s $tmp.list ]
then
# found some valid test numbers ... this is good
@@ -524,3 +539,12 @@ if [ -x "$build_iotests/socket_scm_helper" ]
then
export SOCKET_SCM_HELPER="$build_iotests/socket_scm_helper"
fi
+
+default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ .*//p')
+default_alias_machine=$($QEMU_PROG -machine help | \
+ sed -n "/(alias of $default_machine)/ { s/ .*//p; q; }")
+if [[ "$default_alias_machine" ]]; then
+default_machine="$default_alias_machine"
+fi
+
+export QEMU_DEFAULT_MACHINE="$default_machine"
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index c7f0a7a7e0..cdcda54546 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -26,8 +26,6 @@ arch=`uname -m`
export PWD=`pwd`
-export _QEMU_HANDLE=0
-
# make sure we have a standard umask
umask 022
@@ -40,52 +38,5 @@ _optstr_add()
fi
}
-QEMU_IMG_EXTRA_ARGS=
-if [ "$IMGOPTSSYNTAX" = "true" ]; then
-QEMU_IMG_EXTRA_ARGS="--image-opts $QEMU_IMG_EXTRA_ARGS"
-if [ -n "$IMGKEYSECRET" ]; then
-QEMU_IMG_EXTRA_ARGS="--object secret,id=keysec0,data=$IMGKEYSECRET
$QEMU_IMG_EXTRA_ARGS"
-fi
-fi
-export QEMU_IMG_EXTRA_ARGS
-
-
-default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ .*//p')
-default_alias_machine=$($QEMU_PROG -machine help | \
- sed -n "/(alias of $default_machine)/ { s/ .*//p; q; }")
-if [[ "$default_alias_machine" ]]; then
-default_machine="$default_alias_machine"
-fi
-
-export QEMU_DEFAULT_MACHINE="$default_machine"
-
-if [ -z "$TEST_DIR" ]; then
-TEST_DIR=`pwd`/scratch
-fi
-
-QEMU_TEST_DIR="${TEST_DIR}"
-
-if [ ! -e "$TEST_DIR" ]; then
-mkdir "$TEST_DIR"
-fi
-
-if [ ! -d "$TEST_DIR" ]; then
-echo "common.config: Error: \$TEST_DIR ($TEST_DIR) is not a directory"
-exit 1
-fi
-
-export TEST_DIR
-
-if [ -z "$SAMPLE_IMG_DIR" ]; then
-SAMPLE_IMG_DIR="$source_iotests/sample_images"
-fi
-
-if [ ! -d "$SAMPLE_IMG_DIR" ]; then
-echo "common.config: Error: \$SAMPLE_IMG_DIR ($SAMPLE_IMG_DIR) is not a
directory"
-exit 1
-fi
-
-export SAMPLE_IMG_DIR
-
# make sure this script returns success
true
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 7645f1dc72..9f9aecc9df 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -31,6 +31,7 @@ QEMU_FIFO_IN="${QEMU_TEST_DIR}/qmp-in-$$"
QEMU_FIFO_OUT="${QEMU_TEST_DIR}/qmp-out-$$"
QEMU_HANDLE=0
+export _QEMU_HANDLE=0
# If bash version is >= 4.1, these will be overwritten and dynamic
# file descriptor values assigned.
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 20f6821a69..f4dc0104e6 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -114,6 +114,10 @@ export QEMU_VXHS=_qemu_vxhs_wrapper
if [ "$IMGOPTSSYNTAX" = "true" ]; then
DRIVER="driver=$IMGFMT"
+QEMU_IMG_EXTRA_ARGS="--image-opts $QEMU_IMG_EXTRA_ARGS"
+if [ -n "$IMGKEYSECRET" ]; then
+QEMU_IMG_EXTRA_ARGS="--object secret,id=keysec0,data=$IMGKEYSECRET
$QEMU_IMG_EXTRA_ARGS"
+fi
if [ "$IMGFMT" = "luks" ]; then
DRIVER="$DRIVER,key-secret=keysec0"
fi
@@ -133,6 +137,7 @@ if [ "$IMGOPTSSYNTAX" = "true" ]; then
TEST_IMG="$DRIVER,file.driver=$IMGPROTO,file.filename=$TEST_DIR/t.$IMGFMT"
fi
else
+QEMU_IMG_EXTRA_ARGS=
if [ "$IMGPROTO" = "file" ]; then
TEST_IMG=$TEST_DIR/t.$IMGFMT
elif [ "$IMGPROTO" = "nbd" ]; then
@@ -153,6 +158,26 @@ else
fi
ORIG_TEST_IMG="$TEST_IMG"
+if [ -z "$TEST_DIR" ]; then
+TEST_DIR=`pwd`/scratch
+fi
+
+QEMU_TEST_DIR="${TEST_DIR}"
+
+if [ ! -e "$TEST_DIR" ]; then
+mkdir "$TEST_DIR"
+fi
+
+if [ ! -d "$TEST_DIR" ]; then
+echo "common.config: Error: \$TEST
[Qemu-block] [PATCH 08/10] qemu-iotests: fix uninitialized variable
The variable is used in "common" but defined only after the file
is sourced.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 2 --
tests/qemu-iotests/common | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4cb5b05908..7aec176ac4 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -77,8 +77,6 @@ fi
TIMESTAMP_FILE=check.time-$IMGPROTO-$IMGFMT
-tmp="${TEST_DIR}"/$$
-
_wallclock()
{
date "+%H %M %S" | awk '{ print $1*3600 + $2*60 + $3 }'
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index ee313af92f..365d3c4349 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -67,6 +67,8 @@ sortme=false
expunge=true
have_test_arg=false
cachemode=false
+
+tmp="${TEST_DIR}"/$$
rm -f $tmp.list $tmp.tmp $tmp.sed
export IMGFMT=raw
--
2.13.5
[Qemu-block] [PATCH 06/10] qemu-iotests: do not include common.rc in "check"
It only provides functions used by the test programs. Signed-off-by: Paolo Bonzini --- tests/qemu-iotests/check | 6 -- tests/qemu-iotests/common.rc | 13 + 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index b7c54390d3..4cb5b05908 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -72,12 +72,6 @@ then _init_error "failed to source common.config" fi -# we need common.rc -if ! . "$source_iotests/common.rc" -then -_init_error "failed to source common.rc" -fi - # we need common . "$source_iotests/common" diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index e7f74b4dbd..20f6821a69 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -40,14 +40,11 @@ poke_file() printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null } -# we need common.config -if [ "$iam" != "check" ] -then -if ! . ./common.config -then -echo "$iam: failed to source common.config" -exit 1 -fi + +if ! . ./common.config +then +echo "$iam: failed to source common.config" +exit 1 fi _qemu_wrapper() -- 2.13.5
[Qemu-block] [PATCH 10/10] qemu-iotests: merge "check" and "common"
"check" is full of qemu-iotests--specific details. Separating it
from "common" does not make much sense anymore.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 533 +++-
tests/qemu-iotests/common | 552 --
2 files changed, 531 insertions(+), 554 deletions(-)
delete mode 100644 tests/qemu-iotests/common
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 5dfd4bd51d..2a2f7c604f 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -69,8 +69,537 @@ then
_init_error "failed to source common.config"
fi
-# we need common
-. "$source_iotests/common"
+_full_imgfmt_details()
+{
+if [ -n "$IMGOPTS" ]; then
+echo "$IMGFMT ($IMGOPTS)"
+else
+echo "$IMGFMT"
+fi
+}
+
+_full_platform_details()
+{
+os=`uname -s`
+host=`hostname -s`
+kernel=`uname -r`
+platform=`uname -m`
+echo "$os/$platform $host $kernel"
+}
+
+# $1 = prog to look for
+set_prog_path()
+{
+p=`command -v $1 2> /dev/null`
+if [ -n "$p" -a -x "$p" ]; then
+realpath -- "$(type -p "$p")"
+else
+return 1
+fi
+}
+
+if [ -z "$TEST_DIR" ]; then
+TEST_DIR=`pwd`/scratch
+fi
+
+if [ ! -e "$TEST_DIR" ]; then
+mkdir "$TEST_DIR"
+fi
+
+diff="diff -u"
+verbose=false
+debug=false
+group=false
+xgroup=false
+imgopts=false
+showme=false
+sortme=false
+expunge=true
+have_test_arg=false
+cachemode=false
+
+tmp="${TEST_DIR}"/$$
+rm -f $tmp.list $tmp.tmp $tmp.sed
+
+export IMGFMT=raw
+export IMGFMT_GENERIC=true
+export IMGPROTO=file
+export IMGOPTS=""
+export CACHEMODE="writeback"
+export QEMU_IO_OPTIONS=""
+export QEMU_IO_OPTIONS_NO_FMT=""
+export CACHEMODE_IS_DEFAULT=true
+export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
+export VALGRIND_QEMU=
+export IMGKEYSECRET=
+export IMGOPTSSYNTAX=false
+
+for r
+do
+
+if $group
+then
+# arg after -g
+group_list=`sed -n <"$source_iotests/group" -e 's/$/ /' -e
"/^[0-9][0-9][0-9].* $r /"'{
+s/ .*//p
+}'`
+if [ -z "$group_list" ]
+then
+echo "Group \"$r\" is empty or not defined?"
+exit 1
+fi
+[ ! -s $tmp.list ] && touch $tmp.list
+for t in $group_list
+do
+if grep -s "^$t\$" $tmp.list >/dev/null
+then
+:
+else
+echo "$t" >>$tmp.list
+fi
+done
+group=false
+continue
+
+elif $xgroup
+then
+# arg after -x
+# Populate $tmp.list with all tests
+awk '/^[0-9]{3,}/ {print $1}' "${source_iotests}/group" > $tmp.list
2>/dev/null
+group_list=`sed -n <"$source_iotests/group" -e 's/$/ /' -e
"/^[0-9][0-9][0-9].* $r /"'{
+s/ .*//p
+}'`
+if [ -z "$group_list" ]
+then
+echo "Group \"$r\" is empty or not defined?"
+exit 1
+fi
+numsed=0
+rm -f $tmp.sed
+for t in $group_list
+do
+if [ $numsed -gt 100 ]
+then
+sed -f $tmp.sed <$tmp.list >$tmp.tmp
+mv $tmp.tmp $tmp.list
+numsed=0
+rm -f $tmp.sed
+fi
+echo "/^$t\$/d" >>$tmp.sed
+numsed=`expr $numsed + 1`
+done
+sed -f $tmp.sed <$tmp.list >$tmp.tmp
+mv $tmp.tmp $tmp.list
+xgroup=false
+continue
+
+elif $imgopts
+then
+IMGOPTS="$r"
+imgopts=false
+continue
+elif $cachemode
+then
+CACHEMODE="$r"
+CACHEMODE_IS_DEFAULT=false
+cachemode=false
+continue
+fi
+
+xpand=true
+case "$r"
+in
+
+-\? | -h | --help)# usage
+echo "Usage: $0 [options] [testlist]"'
+
+common options
+-v verbose
+-d debug
+
+image format options
+-rawtest raw (default)
+-bochs test bochs
+-cloop test cloop
+-parallels test parallels
+-qcow test qcow
+-qcow2 test qcow2
+-qedtest qed
+-vditest vdi
+-vpctest vpc
+-vhdx test vhdx
+-vmdk test vmdk
+-luks test luks
+
+image protocol options
+-file test file (default)
+-rbdtest rbd
+-sheepdog test sheepdog
+-nbdtest nbd
+-sshtest ssh
+-nfstest nfs
+-vxhs test vxhs
+
+other options
+-xdiff graphical mode diff
+-nocacheuse O_DIRECT on backing file
+-misalign misalign memory allocations
+-n show me, do not run tests
+-o options -o options to pass to qemu-img create/convert
+-T output timest
[Qemu-block] [PATCH 09/10] qemu-iotests: get rid of $iam
The variable is almost unused, and one of the two uses is actually
uninitialized.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 5 +
tests/qemu-iotests/common.rc | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 7aec176ac4..5dfd4bd51d 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -30,12 +30,9 @@ interrupt=true
# by default don't output timestamps
timestamp=${TIMESTAMP:=false}
-# generic initialization
-iam=check
-
_init_error()
{
-echo "$iam: $1" >&2
+echo "check: $1" >&2
exit 1
}
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index f4dc0104e6..0e8a33c696 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -43,7 +43,7 @@ poke_file()
if ! . ./common.config
then
-echo "$iam: failed to source common.config"
+echo "$0: failed to source common.config"
exit 1
fi
--
2.13.5
[Qemu-block] [PATCH 05/10] qemu-iotests: limit non-_PROG-suffixed variables to common.rc
These are never used by "check", with one exception that does not need
$QEMU_OPTIONS. Keep them in common.rc, which will be soon included only
by the tests.
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/039.out | 10 +++---
tests/qemu-iotests/061.out | 4 +--
tests/qemu-iotests/137.out | 2 +-
tests/qemu-iotests/common.config | 69 ++--
tests/qemu-iotests/common.rc | 65 +
5 files changed, 75 insertions(+), 75 deletions(-)
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index c6e0ac2da3..724d7b2508 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -11,7 +11,7 @@ No errors were found on the image.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
@@ -50,7 +50,7 @@ read 512/512 bytes at offset 0
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
@@ -68,7 +68,7 @@ incompatible_features 0x0
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
@@ -91,7 +91,7 @@ No errors were found on the image.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
@@ -105,7 +105,7 @@ Data may be corrupted, or further writes to the image may
corrupt it.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index a431b7f305..942485de99 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -57,7 +57,7 @@ No errors were found on the image.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
wrote 131072/131072 bytes at offset 0
128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
@@ -219,7 +219,7 @@ No errors were found on the image.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
wrote 131072/131072 bytes at offset 0
128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed ( if [ "${VALGRIND_QEMU}" == "y" ];
then
+./common.rc: Killed ( if [ "${VALGRIND_QEMU}" == "y" ]; then
exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
else
exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index c0e753483b..05efd74d17 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -31,7 +31,7 @@ Cache clean interval too big
Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of
the following: none, constant, cached, all
wrote 512/512 bytes at offset 0
512 byt
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] iotests: use -ccw on s390x for 051
On Fri, 8 Sep 2017 17:55:29 +0200 Thomas Huth wrote: > On 08.09.2017 13:54, Kevin Wolf wrote: > > Am 08.09.2017 um 13:24 hat Cornelia Huck geschrieben: > >> On Fri, 8 Sep 2017 13:04:25 +0200 > >> Kevin Wolf wrote: > >> > >>> Am 05.09.2017 um 17:16 hat Cornelia Huck geschrieben: > The default cpu model on s390x does not provide zPCI, which is > not yet wired up on tcg. Moreover, virtio-ccw is the standard > on s390x, so use the -ccw instead of the -pci versions of virtio > devices on s390x. > > Provide an output file for s390x. > > Signed-off-by: Cornelia Huck > --- > tests/qemu-iotests/051 | 9 +- > tests/qemu-iotests/051.s390-ccw-virtio.out | 434 > + > 2 files changed, 442 insertions(+), 1 deletion(-) > create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out > >>> > >>> It's already a pain to have two separate output files for 051, let's try > >>> to avoid adding a third one. Even more so since I think that the split > >>> between 051.out and 051.pc.out was already made for s390, so I'm not > >>> sure if anyone would actually still make use of the plain 051.out > >>> output if s390 got it's own one. > >> > >> Are there no non-pc and non-s390 machines for which this is run? > > > > Who knows? But I'm not aware of anyone who is interested in something > > else and has contributed to the test cases until now. > > FWIW, as far as I know, Lukáš is running this test also on ppc64 in our > weekly regression run. So it would be good to keep that working, please :-) > > >> Another approach would be to drop the -pci postfix, but I don't want to > >> introduce more usage of aliases. > > > > Maybe that would indeed be the easiest way. As long as we don't intend > > to remove the alias from qemu, there's no reason not to use it in tests. > > Maybe we should even use it in a couple of places on purpose - so we get > some test coverage for them? FWIW, using the the sed post-run filtering works well for 051, but it is driving me bonkers on 067 (or maybe I should just call it a day...) We could use the sed approach for 051 and the alias approach on 067 - that way we would also test aliases :) Any preferences by the iotest maintainers? [I'd be happy if iotests worked again on s390x...]
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] iotests: use -ccw on s390x for 051
Am 12.09.2017 um 18:05 hat Cornelia Huck geschrieben: > On Fri, 8 Sep 2017 17:55:29 +0200 > Thomas Huth wrote: > > > On 08.09.2017 13:54, Kevin Wolf wrote: > > > Am 08.09.2017 um 13:24 hat Cornelia Huck geschrieben: > > >> On Fri, 8 Sep 2017 13:04:25 +0200 > > >> Kevin Wolf wrote: > > >> > > >>> Am 05.09.2017 um 17:16 hat Cornelia Huck geschrieben: > > The default cpu model on s390x does not provide zPCI, which is > > not yet wired up on tcg. Moreover, virtio-ccw is the standard > > on s390x, so use the -ccw instead of the -pci versions of virtio > > devices on s390x. > > > > Provide an output file for s390x. > > > > Signed-off-by: Cornelia Huck > > --- > > tests/qemu-iotests/051 | 9 +- > > tests/qemu-iotests/051.s390-ccw-virtio.out | 434 > > + > > 2 files changed, 442 insertions(+), 1 deletion(-) > > create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out > > >>> > > >>> It's already a pain to have two separate output files for 051, let's try > > >>> to avoid adding a third one. Even more so since I think that the split > > >>> between 051.out and 051.pc.out was already made for s390, so I'm not > > >>> sure if anyone would actually still make use of the plain 051.out > > >>> output if s390 got it's own one. > > >> > > >> Are there no non-pc and non-s390 machines for which this is run? > > > > > > Who knows? But I'm not aware of anyone who is interested in something > > > else and has contributed to the test cases until now. > > > > FWIW, as far as I know, Lukáš is running this test also on ppc64 in our > > weekly regression run. So it would be good to keep that working, please :-) > > > > >> Another approach would be to drop the -pci postfix, but I don't want to > > >> introduce more usage of aliases. > > > > > > Maybe that would indeed be the easiest way. As long as we don't intend > > > to remove the alias from qemu, there's no reason not to use it in tests. > > > > Maybe we should even use it in a couple of places on purpose - so we get > > some test coverage for them? > > FWIW, using the the sed post-run filtering works well for 051, but it > is driving me bonkers on 067 (or maybe I should just call it a day...) > > We could use the sed approach for 051 and the alias approach on 067 - > that way we would also test aliases :) > > Any preferences by the iotest maintainers? Sure, that works for me. I'm happy with anything that works and doesn't split the reference output in two versions. (Not splitting is also in your own interest; if anything bitrots, it's the non-pc version.) Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] qemu-iotests: get rid of AWK_PROG
Hi Paolo,
Hmm did you just resend your v1? Except the cover the patches don't have
"v2" and I remember reviewing this one (also Eric Blake replied with his
R-b).
On 09/12/2017 11:44 AM, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini
---
tests/qemu-iotests/check | 4 ++--
tests/qemu-iotests/common| 2 +-
tests/qemu-iotests/common.config | 3 ---
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4a6ed67b42..5c0d0c51dc 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -128,7 +128,7 @@ tmp="${TEST_DIR}"/$$
_wallclock()
{
-date "+%H %M %S" | $AWK_PROG '{ print $1*3600 + $2*60 + $3 }'
+date "+%H %M %S" | awk '{ print $1*3600 + $2*60 + $3 }'
}
_timestamp()
@@ -147,7 +147,7 @@ _wrapup()
if [ -f $TIMESTAMP_FILE -a -f $tmp.time ]
then
cat $TIMESTAMP_FILE $tmp.time \
-| $AWK_PROG '
+| awk '
{ t[$1] = $2 }
END{ if (NR > 0) {
for (i in t) print i " " t[i]
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 867918895b..130f647a4d 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -366,7 +366,7 @@ testlist options
if $xpand
then
have_test_arg=true
-$AWK_PROG
-export AWK_PROG="`set_prog_path awk`"
-[ "$AWK_PROG" = "" ] && _fatal "awk not found"
-
if [ -z "$QEMU_PROG" ]; then
export QEMU_PROG="`set_prog_path qemu`"
fi
Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] qemu-iotests: get rid of AWK_PROG
On 12/09/2017 18:30, Philippe Mathieu-Daudé wrote:
> Hi Paolo,
>
> Hmm did you just resend your v1? Except the cover the patches don't have
> "v2" and I remember reviewing this one (also Eric Blake replied with his
> R-b).
I didn't include the R-bs, sorry (most of the later patches had
non-trivial context changes, this one is the exception).
But it's definitely v2, because v1 had 12 patches. :)
Paolo
> On 09/12/2017 11:44 AM, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini
>> ---
>> tests/qemu-iotests/check | 4 ++--
>> tests/qemu-iotests/common| 2 +-
>> tests/qemu-iotests/common.config | 3 ---
>> 3 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 4a6ed67b42..5c0d0c51dc 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -128,7 +128,7 @@ tmp="${TEST_DIR}"/$$
>> _wallclock()
>> {
>> -date "+%H %M %S" | $AWK_PROG '{ print $1*3600 + $2*60 + $3 }'
>> +date "+%H %M %S" | awk '{ print $1*3600 + $2*60 + $3 }'
>> }
>> _timestamp()
>> @@ -147,7 +147,7 @@ _wrapup()
>> if [ -f $TIMESTAMP_FILE -a -f $tmp.time ]
>> then
>> cat $TIMESTAMP_FILE $tmp.time \
>> -| $AWK_PROG '
>> +| awk '
>> { t[$1] = $2 }
>> END{ if (NR > 0) {
>> for (i in t) print i " " t[i]
>> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
>> index 867918895b..130f647a4d 100644
>> --- a/tests/qemu-iotests/common
>> +++ b/tests/qemu-iotests/common
>> @@ -366,7 +366,7 @@ testlist options
>> if $xpand
>> then
>> have_test_arg=true
>> -$AWK_PROG > +awk > BEGIN{ for (t='$start'; t<='$end'; t++) printf "%03d\n",t }' \
>> | while read id
>> do
>> diff --git a/tests/qemu-iotests/common.config
>> b/tests/qemu-iotests/common.config
>> index b599c72211..0f571d46eb 100644
>> --- a/tests/qemu-iotests/common.config
>> +++ b/tests/qemu-iotests/common.config
>> @@ -56,9 +56,6 @@ _fatal()
>> exit 1
>> }
>> -export AWK_PROG="`set_prog_path awk`"
>> -[ "$AWK_PROG" = "" ] && _fatal "awk not found"
>> -
>> if [ -z "$QEMU_PROG" ]; then
>> export QEMU_PROG="`set_prog_path qemu`"
>> fi
>>
Re: [Qemu-block] [Qemu-devel] [PATCH v6 05/18] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes
On 09/08/2017 09:04 AM, Eric Blake wrote:
>>> void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>> {
>>> BdrvDirtyBitmap *bitmap;
>>> -uint64_t size = bdrv_nb_sectors(bs);
>>> +int64_t size = bdrv_getlength(bs);
>>>
>>> +assert(size >= 0);
>>
>> How can you assert that there will never be an error? Even if it's
>> correct (I don't know whether you can have dirty bitmaps on devices that
>> don't use the cached value), this needs at least a comment.
>
> The old code wasn't checking for errors; if an error occurs, we have no
> way to report it. So I indeed need to audit whether all callers have a
> cached length at this point in time (it can't fail), or else change
> bdrv_dirty_bitmap_truncate() to be able to fail (pass failure along) and
> update all callers. This may indeed be reason for a respin, depending
> on what I find.
Verdict - it can indeed fail; bdrv_truncate() was blindly calling the
dirty bitmap resize even after a failed refresh_total_sectors(), which
could then resize the dirty bitmap to -1. At least bdrv_truncate()
itself still failed, but it's cleaner to fail up front rather than get
internal state even more botched in the meantime, so fixing that will be
a separate patch in v7. Sadly, the failure is probably more
theoretical, and I did not quickly see an easy way to write an iotests
to expose it (which has been the case with a lot of our recent
bdrv_nb_sectors/bdrv_getlength failure cleanup patches).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v7 01/20] block: Make bdrv_img_create() size selection easier to read
All callers of bdrv_img_create() pass in a size, or -1 to read the
size from the backing file. We then set that size as the QemuOpt
default, which means we will reuse that default rather than the
final parameter to qemu_opt_get_size() several lines later. But
it is rather confusing to read subsequent checks of 'size == -1'
when it looks (without seeing the full context) like size defaults
to 0; it also doesn't help that a size of 0 is valid (for some
formats).
Rework the logic to make things more legible.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v6: Combine into a series rather than being a standalone patch (more for
ease of tracking than for being on topic)
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 6dd47e414e..ee6a48976e 100644
--- a/block.c
+++ b/block.c
@@ -4393,7 +4393,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
/* The size for the image must always be specified, unless we have a
backing
* file and we have not been forbidden from opening it. */
-size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
+size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, img_size);
if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
BlockDriverState *bs;
char *full_backing = g_new0(char, PATH_MAX);
--
2.13.5
[Qemu-block] [PATCH v7 02/20] hbitmap: Rename serialization_granularity to serialization_align
The only client of hbitmap_serialization_granularity() is dirty-bitmap's
bdrv_dirty_bitmap_serialization_align(). Keeping the two names consistent
is worthwhile, and the shorter name is more representative of what the
function returns (the required alignment to be used for start/count of
other serialization functions, where violating the alignment causes
assertion failures).
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: no change
v4: new patch
---
include/qemu/hbitmap.h | 8
block/dirty-bitmap.c | 2 +-
tests/test-hbitmap.c | 10 +-
util/hbitmap.c | 8
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index d3a74a21fc..81e78043d1 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -159,16 +159,16 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item);
bool hbitmap_is_serializable(const HBitmap *hb);
/**
- * hbitmap_serialization_granularity:
+ * hbitmap_serialization_align:
* @hb: HBitmap to operate on.
*
- * Granularity of serialization chunks, used by other serialization functions.
- * For every chunk:
+ * Required alignment of serialization chunks, used by other serialization
+ * functions. For every chunk:
* 1. Chunk start should be aligned to this granularity.
* 2. Chunk size should be aligned too, except for last chunk (for which
* start + count == hb->size)
*/
-uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
+uint64_t hbitmap_serialization_align(const HBitmap *hb);
/**
* hbitmap_serialization_size:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 30462d4f9a..0490ca3aff 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -617,7 +617,7 @@ uint64_t bdrv_dirty_bitmap_serialization_size(const
BdrvDirtyBitmap *bitmap,
uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
{
-return hbitmap_serialization_granularity(bitmap->bitmap);
+return hbitmap_serialization_align(bitmap->bitmap);
}
void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 1acb353889..af41642346 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -738,15 +738,15 @@ static void test_hbitmap_meta_one(TestHBitmapData *data,
const void *unused)
}
}
-static void test_hbitmap_serialize_granularity(TestHBitmapData *data,
- const void *unused)
+static void test_hbitmap_serialize_align(TestHBitmapData *data,
+ const void *unused)
{
int r;
hbitmap_test_init(data, L3 * 2, 3);
g_assert(hbitmap_is_serializable(data->hb));
-r = hbitmap_serialization_granularity(data->hb);
+r = hbitmap_serialization_align(data->hb);
g_assert_cmpint(r, ==, 64 << 3);
}
@@ -974,8 +974,8 @@ int main(int argc, char **argv)
hbitmap_test_add("/hbitmap/meta/word", test_hbitmap_meta_word);
hbitmap_test_add("/hbitmap/meta/sector", test_hbitmap_meta_sector);
-hbitmap_test_add("/hbitmap/serialize/granularity",
- test_hbitmap_serialize_granularity);
+hbitmap_test_add("/hbitmap/serialize/align",
+ test_hbitmap_serialize_align);
hbitmap_test_add("/hbitmap/serialize/basic",
test_hbitmap_serialize_basic);
hbitmap_test_add("/hbitmap/serialize/part",
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 21535cc90b..2f9d0fdbd0 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -413,14 +413,14 @@ bool hbitmap_is_serializable(const HBitmap *hb)
{
/* Every serialized chunk must be aligned to 64 bits so that endianness
* requirements can be fulfilled on both 64 bit and 32 bit hosts.
- * We have hbitmap_serialization_granularity() which converts this
+ * We have hbitmap_serialization_align() which converts this
* alignment requirement from bitmap bits to items covered (e.g. sectors).
* That value is:
*64 << hb->granularity
* Since this value must not exceed UINT64_MAX, hb->granularity must be
* less than 58 (== 64 - 6, where 6 is ld(64), i.e. 1 << 6 == 64).
*
- * In order for hbitmap_serialization_granularity() to always return a
+ * In order for hbitmap_serialization_align() to always return a
* meaningful value, bitmaps that are to be serialized must have a
* granularity of less than 58. */
@@ -437,7 +437,7 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
}
-uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
+uint64_t hbitmap_serialization_align(const HBitmap *hb)
{
assert(hbitmap_is_serializable(hb));
@@ -454,7 +454,7 @@ static void serialization_chunk(const HBitmap *hb,
unsigned long **first_el, uint64_t *el_count)
{
uint64_t last = sta
[Qemu-block] [PATCH v7 00/20] make dirty-bitmap byte-based
There are patches floating around to add NBD_CMD_BLOCK_STATUS, but NBD wants to report status on byte granularity (even if the reporting will probably be naturally aligned to sectors or even much higher levels). I've therefore started the task of converting our block status code to report at a byte granularity rather than sectors. Now that 2.11 is open, I'm rebasing/reposting the remaining patches. The overall conversion currently looks like: part 1: bdrv_is_allocated (merged in 2.10, commit 51b0a488) part 2: dirty-bitmap (this series, v6 was here [1]) part 3: bdrv_get_block_status (v3 is posted [2] and is mostly reviewed, but needs a rebase) part 4: .bdrv_co_block_status (v2 is posted [3], but needs a rebase) Available as a tag at: git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-dirty-v7 [1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06247.html [2] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg03853.html [3] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04370.html Diff from v6: - split v6 patch 5/18 into 3 parts in 5-7/20 [Kevin] - improve bdrv_dirty_iter_next() interface in 11/20 [Kevin] - fix missing trace conversion in 12/20 [Kevin] - stricter conversions in 16/20, 18/20 [Kevin] - rebase to above in 20/20 001/20:[] [--] 'block: Make bdrv_img_create() size selection easier to read' 002/20:[] [--] 'hbitmap: Rename serialization_granularity to serialization_align' 003/20:[] [--] 'qcow2: Ensure bitmap serialization is aligned' 004/20:[] [--] 'dirty-bitmap: Drop unused functions' 005/20:[down] 'dirty-bitmap: Check for size query failure during truncate' 006/20:[0028] [FC] 'dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes' 007/20:[down] 'dirty-bitmap: Track bitmap size by bytes' 008/20:[] [--] 'dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes' 009/20:[] [--] 'qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based' 010/20:[] [-C] 'dirty-bitmap: Set iterator start by offset, not sector' 011/20:[0005] [FC] 'dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset' 012/20:[0003] [FC] 'dirty-bitmap: Change bdrv_get_dirty_count() to report bytes' 013/20:[] [--] 'dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes' 014/20:[] [--] 'dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes' 015/20:[] [--] 'mirror: Switch mirror_dirty_init() to byte-based iteration' 016/20:[0002] [FC] 'qcow2: Switch qcow2_measure() to byte-based iteration' 017/20:[] [--] 'qcow2: Switch load_bitmap_data() to byte-based iteration' 018/20:[0006] [FC] 'qcow2: Switch store_bitmap_data() to byte-based iteration' 019/20:[] [--] 'dirty-bitmap: Switch bdrv_set_dirty() to bytes' 020/20:[0003] [FC] 'dirty-bitmap: Convert internal hbitmap size/granularity' Eric Blake (20): block: Make bdrv_img_create() size selection easier to read hbitmap: Rename serialization_granularity to serialization_align qcow2: Ensure bitmap serialization is aligned dirty-bitmap: Drop unused functions dirty-bitmap: Check for size query failure during truncate dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes dirty-bitmap: Track bitmap size by bytes dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based dirty-bitmap: Set iterator start by offset, not sector dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset dirty-bitmap: Change bdrv_get_dirty_count() to report bytes dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes mirror: Switch mirror_dirty_init() to byte-based iteration qcow2: Switch qcow2_measure() to byte-based iteration qcow2: Switch load_bitmap_data() to byte-based iteration qcow2: Switch store_bitmap_data() to byte-based iteration dirty-bitmap: Switch bdrv_set_dirty() to bytes dirty-bitmap: Convert internal hbitmap size/granularity include/block/block_int.h| 2 +- include/block/dirty-bitmap.h | 43 +- include/qemu/hbitmap.h | 8 +-- block/io.c | 6 +- block.c | 21 +-- block/backup.c | 7 +-- block/dirty-bitmap.c | 137 +++ block/mirror.c | 79 ++--- block/qcow2-bitmap.c | 57 +- block/qcow2.c| 22 --- migration/block.c| 12 ++-- tests/test-hbitmap.c | 10 ++-- util/hbitmap.c | 8 +-- 13 files changed, 175 insertions(+), 237 deletions(-) -- 2.13.5
[Qemu-block] [PATCH v7 03/20] qcow2: Ensure bitmap serialization is aligned
When subdividing a bitmap serialization, the code in hbitmap.c
enforces that start/count parameters are aligned (except that
count can end early at end-of-bitmap). We exposed this required
alignment through bdrv_dirty_bitmap_serialization_align(), but
forgot to actually check that we comply with it.
Fortunately, qcow2 is never dividing bitmap serialization smaller
than one cluster (which is a minimum of 512 bytes); so we are
always compliant with the serialization alignment (which insists
that we partition at least 64 bits per chunk) because we are doing
at least 4k bits per chunk.
Still, it's safer to add an assertion (for the unlikely case that
we'd ever support a cluster smaller than 512 bytes, or if the
hbitmap implementation changes what it considers to be aligned),
rather than leaving bdrv_dirty_bitmap_serialization_align()
without a caller.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: no change
v4: new patch
---
block/qcow2-bitmap.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index e8d3bdbd6e..b3ee4c794a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -274,10 +274,13 @@ static int free_bitmap_clusters(BlockDriverState *bs,
Qcow2BitmapTable *tb)
static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
const BdrvDirtyBitmap
*bitmap)
{
-uint32_t sector_granularity =
+uint64_t sector_granularity =
bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+uint64_t sbc = sector_granularity * (s->cluster_size << 3);
-return (uint64_t)sector_granularity * (s->cluster_size << 3);
+assert(QEMU_IS_ALIGNED(sbc,
+ bdrv_dirty_bitmap_serialization_align(bitmap)));
+return sbc;
}
/* load_bitmap_data
--
2.13.5
[Qemu-block] [PATCH v7 06/20] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes
We're already reporting bytes for bdrv_dirty_bitmap_granularity();
mixing bytes and sectors in our return values is a recipe for
confusion. A later cleanup will convert dirty bitmap internals
to be entirely byte-based, but in the meantime, we should report
the bitmap size in bytes.
The only external caller in qcow2-bitmap.c is temporarily more verbose
(because it is still using sector-based math), but will later be
switched to track progress by bytes instead of sectors.
Signed-off-by: Eric Blake
---
v7: split external from internal change [Kevin], drop R-b
v6: no change
v5: fix bdrv_dirty_bitmap_truncate [John], drop R-b
v4: retitle from "Track size in bytes", rebase to persistent bitmaps,
round up when converting bytes to sectors
v3: no change
v2: tweak commit message, no code change
---
block/dirty-bitmap.c | 2 +-
block/qcow2-bitmap.c | 14 --
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 52f7a399b2..56a01699e9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -175,7 +175,7 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
{
-return bitmap->size;
+return bitmap->size * BDRV_SECTOR_SIZE;
}
const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b3ee4c794a..65122e9ae1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -295,10 +295,11 @@ static int load_bitmap_data(BlockDriverState *bs,
BDRVQcow2State *s = bs->opaque;
uint64_t sector, sbc;
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
uint8_t *buf = NULL;
uint64_t i, tab_size =
size_to_clusters(s,
-bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_sectors));
if (tab_size != bitmap_table_size || tab_size > BME_MAX_TABLE_SIZE) {
return -EINVAL;
@@ -307,7 +308,7 @@ static int load_bitmap_data(BlockDriverState *bs,
buf = g_malloc(s->cluster_size);
sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
-uint64_t count = MIN(bm_size - sector, sbc);
+uint64_t count = MIN(bm_sectors - sector, sbc);
uint64_t entry = bitmap_table[i];
uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
@@ -1077,13 +1078,14 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
int64_t sector;
uint64_t sbc;
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
uint8_t *buf = NULL;
BdrvDirtyBitmapIter *dbi;
uint64_t *tb;
uint64_t tb_size =
size_to_clusters(s,
-bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_sectors));
if (tb_size > BME_MAX_TABLE_SIZE ||
tb_size * s->cluster_size > BME_MAX_PHYS_SIZE)
@@ -1101,7 +1103,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
dbi = bdrv_dirty_iter_new(bitmap, 0);
buf = g_malloc(s->cluster_size);
sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
-assert(DIV_ROUND_UP(bm_size, sbc) == tb_size);
+assert(DIV_ROUND_UP(bm_sectors, sbc) == tb_size);
while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
uint64_t cluster = sector / sbc;
@@ -1109,7 +,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
int64_t off;
sector = cluster * sbc;
-end = MIN(bm_size, sector + sbc);
+end = MIN(bm_sectors, sector + sbc);
write_size =
bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
assert(write_size <= s->cluster_size);
@@ -1141,7 +1143,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
goto fail;
}
-if (end >= bm_size) {
+if (end >= bm_sectors) {
break;
}
--
2.13.5
[Qemu-block] [PATCH v7 08/20] dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes
Right now, the dirty-bitmap code exposes the fact that we use
a scale of sector granularity in the underlying hbitmap to anything
that wants to serialize a dirty bitmap. It's nicer to uniformly
expose bytes as our dirty-bitmap interface, matching the previous
change to bitmap size. The only caller to serialization is currently
qcow2-cluster.c, which becomes a bit more verbose because it is still
tracking sectors for other reasons, but a later patch will fix that
to more uniformly use byte offsets everywhere. Likewise, within
dirty-bitmap, we have to add more assertions that we are not
truncating incorrectly, which can go away once the internal hbitmap
is byte-based rather than sector-based.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: no change
v4: new patch
---
include/block/dirty-bitmap.h | 14 +++---
block/dirty-bitmap.c | 37 -
block/qcow2-bitmap.c | 22 ++
3 files changed, 45 insertions(+), 28 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 15101b59d5..cd32149db6 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -49,19 +49,19 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap
*bitmap,
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
- uint64_t start, uint64_t count);
+ uint64_t offset, uint64_t bytes);
uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
- uint8_t *buf, uint64_t start,
- uint64_t count);
+ uint8_t *buf, uint64_t offset,
+ uint64_t bytes);
void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
-uint8_t *buf, uint64_t start,
-uint64_t count, bool finish);
+uint8_t *buf, uint64_t offset,
+uint64_t bytes, bool finish);
void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
- uint64_t start, uint64_t count,
+ uint64_t offset, uint64_t bytes,
bool finish);
void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
-uint64_t start, uint64_t count,
+uint64_t offset, uint64_t bytes,
bool finish);
void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index ec4fc0e1ad..dbf5a53044 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -573,42 +573,53 @@ void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap
*bitmap, HBitmap *in)
}
uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
- uint64_t start, uint64_t count)
+ uint64_t offset, uint64_t bytes)
{
-return hbitmap_serialization_size(bitmap->bitmap, start, count);
+assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
+return hbitmap_serialization_size(bitmap->bitmap,
+ offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS);
}
uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
{
-return hbitmap_serialization_align(bitmap->bitmap);
+return hbitmap_serialization_align(bitmap->bitmap) * BDRV_SECTOR_SIZE;
}
void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
- uint8_t *buf, uint64_t start,
- uint64_t count)
+ uint8_t *buf, uint64_t offset,
+ uint64_t bytes)
{
-hbitmap_serialize_part(bitmap->bitmap, buf, start, count);
+assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
+hbitmap_serialize_part(bitmap->bitmap, buf, offset >> BDRV_SECTOR_BITS,
+ bytes >> BDRV_SECTOR_BITS);
}
void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
-uint8_t *buf, uint64_t start,
-uint64_t count, bool finish)
+uint8_t *buf, uint64_t offset,
+uint64_t bytes, bool finish)
{
-hbitmap_deserialize_part(bitmap->bitmap, buf, start, count, finish);
+
[Qemu-block] [PATCH v7 04/20] dirty-bitmap: Drop unused functions
We had several functions that no one is currently using, and which
use sector-based interfaces. I'm trying to convert towards byte-based
interfaces, so it's easier to just drop the unused functions:
bdrv_dirty_bitmap_get_meta
bdrv_dirty_bitmap_get_meta_locked
bdrv_dirty_bitmap_reset_meta
bdrv_dirty_bitmap_meta_granularity
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: no change
v4: rebase to Vladimir's persistent bitmaps (bdrv_dirty_bitmap_size now
in use), dropped R-b
v3: rebase to upstream changes (bdrv_dirty_bitmap_get_meta_locked was
added in b64bd51e with no clients), kept R-b
v2: tweak commit message based on review, no code change
---
include/block/dirty-bitmap.h | 10 --
block/dirty-bitmap.c | 44
2 files changed, 54 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a79a58d2c3..8fd842eac9 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,7 +34,6 @@ void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
@@ -44,15 +43,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors);
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors);
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, int64_t sector,
- int nb_sectors);
-int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, int64_t sector,
- int nb_sectors);
-void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, int64_t sector,
- int nb_sectors);
BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
uint64_t first_sector);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 0490ca3aff..42a55e4a4b 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -173,45 +173,6 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap
*bitmap)
qemu_mutex_unlock(bitmap->mutex);
}
-int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, int64_t sector,
- int nb_sectors)
-{
-uint64_t i;
-int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta);
-
-/* To optimize: we can make hbitmap to internally check the range in a
- * coarse level, or at least do it word by word. */
-for (i = sector; i < sector + nb_sectors; i += sectors_per_bit) {
-if (hbitmap_get(bitmap->meta, i)) {
-return true;
-}
-}
-return false;
-}
-
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, int64_t sector,
- int nb_sectors)
-{
-bool dirty;
-
-qemu_mutex_lock(bitmap->mutex);
-dirty = bdrv_dirty_bitmap_get_meta_locked(bs, bitmap, sector, nb_sectors);
-qemu_mutex_unlock(bitmap->mutex);
-
-return dirty;
-}
-
-void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, int64_t sector,
- int nb_sectors)
-{
-qemu_mutex_lock(bitmap->mutex);
-hbitmap_reset(bitmap->meta, sector, nb_sectors);
-qemu_mutex_unlock(bitmap->mutex);
-}
-
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
{
return bitmap->size;
@@ -511,11 +472,6 @@ uint32_t bdrv_dirty_bitmap_granularity(const
BdrvDirtyBitmap *bitmap)
return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
}
-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
-{
-return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
-}
-
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
uint64_t first_sector)
{
--
2.13.5
[Qemu-block] [PATCH v7 05/20] dirty-bitmap: Check for size query failure during truncate
We've previously fixed several places where we failed to account
for possible errors from bdrv_nb_sectors(). Fix another one by
making bdrv_dirty_bitmap_truncate() report the error rather then
silently resizing bitmaps to -1. Then adjust the sole caller
bdrv_truncate() to both reduce the likelihood of failure (blindly
calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()
fails was not nice) as well as propagate any actual failures.
Signed-off-by: Eric Blake
---
v7: new patch [Kevin]
---
include/block/dirty-bitmap.h | 2 +-
block.c | 19 ++-
block/dirty-bitmap.c | 12
3 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8fd842eac9..15101b59d5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
-void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+int bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
diff --git a/block.c b/block.c
index ee6a48976e..790dcce360 100644
--- a/block.c
+++ b/block.c
@@ -3450,12 +3450,21 @@ int bdrv_truncate(BdrvChild *child, int64_t offset,
PreallocMode prealloc,
assert(!(bs->open_flags & BDRV_O_INACTIVE));
ret = drv->bdrv_truncate(bs, offset, prealloc, errp);
-if (ret == 0) {
-ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
-bdrv_dirty_bitmap_truncate(bs);
-bdrv_parent_cb_resize(bs);
-atomic_inc(&bs->write_gen);
+if (ret < 0) {
+return ret;
}
+ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not refresh total sector count");
+return ret;
+}
+ret = bdrv_dirty_bitmap_truncate(bs);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not refresh total sector count");
+return ret;
+}
+bdrv_parent_cb_resize(bs);
+atomic_inc(&bs->write_gen);
return ret;
}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 42a55e4a4b..52f7a399b2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -1,7 +1,7 @@
/*
* Block Dirty Bitmap
*
- * Copyright (c) 2016 Red Hat. Inc
+ * Copyright (c) 2016-2017 Red Hat. Inc
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -300,13 +300,16 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
/**
* Truncates _all_ bitmaps attached to a BDS.
- * Called with BQL taken.
+ * Called with BQL taken, returns -errno on failure.
*/
-void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+int bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
{
BdrvDirtyBitmap *bitmap;
-uint64_t size = bdrv_nb_sectors(bs);
+int64_t size = bdrv_nb_sectors(bs);
+if (size < 0) {
+return size;
+}
bdrv_dirty_bitmaps_lock(bs);
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
assert(!bdrv_dirty_bitmap_frozen(bitmap));
@@ -315,6 +318,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
bitmap->size = size;
}
bdrv_dirty_bitmaps_unlock(bs);
+return 0;
}
static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
--
2.13.5
[Qemu-block] [PATCH v7 12/20] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes
Thanks to recent cleanups, all callers were scaling a return value
of sectors into bytes; do the scaling internally instead.
Signed-off-by: Eric Blake
---
v7: fix one more trace caller [Kevin]
v4-v6: no change
v3: no change, add R-b
v2: no change
---
block/dirty-bitmap.c | 4 ++--
block/mirror.c | 16 ++--
migration/block.c| 2 +-
3 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 49229fd501..ee0afb5e1a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -428,7 +428,7 @@ BlockDirtyInfoList
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
+info->count = bdrv_get_dirty_count(bm);
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
@@ -657,7 +657,7 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t
offset)
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
{
-return hbitmap_count(bitmap->bitmap);
+return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
}
int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index 77bf5aa3a4..7113d47db4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -340,8 +340,7 @@ static uint64_t coroutine_fn
mirror_iteration(MirrorBlockJob *s)
if (offset < 0) {
bdrv_set_dirty_iter(s->dbi, 0);
offset = bdrv_dirty_iter_next(s->dbi);
-trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
- BDRV_SECTOR_SIZE);
+trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
assert(offset >= 0);
}
bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
@@ -811,11 +810,10 @@ static void coroutine_fn mirror_run(void *opaque)
cnt = bdrv_get_dirty_count(s->dirty_bitmap);
/* s->common.offset contains the number of bytes already processed so
- * far, cnt is the number of dirty sectors remaining and
+ * far, cnt is the number of dirty bytes remaining and
* s->bytes_in_flight is the number of bytes currently being
* processed; together those are the current total operation length */
-s->common.len = s->common.offset + s->bytes_in_flight +
-cnt * BDRV_SECTOR_SIZE;
+s->common.len = s->common.offset + s->bytes_in_flight + cnt;
/* Note that even when no rate limit is applied we need to yield
* periodically with no pending I/O so that bdrv_drain_all() returns.
@@ -827,8 +825,7 @@ static void coroutine_fn mirror_run(void *opaque)
s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
(cnt == 0 && s->in_flight > 0)) {
-trace_mirror_yield(s, cnt * BDRV_SECTOR_SIZE,
- s->buf_free_count, s->in_flight);
+trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
mirror_wait_for_io(s);
continue;
} else if (cnt != 0) {
@@ -869,7 +866,7 @@ static void coroutine_fn mirror_run(void *opaque)
* whether to switch to target check one last time if I/O has
* come in the meanwhile, and if not flush the data to disk.
*/
-trace_mirror_before_drain(s, cnt * BDRV_SECTOR_SIZE);
+trace_mirror_before_drain(s, cnt);
bdrv_drained_begin(bs);
cnt = bdrv_get_dirty_count(s->dirty_bitmap);
@@ -888,8 +885,7 @@ static void coroutine_fn mirror_run(void *opaque)
}
ret = 0;
-trace_mirror_before_sleep(s, cnt * BDRV_SECTOR_SIZE,
- s->synced, delay_ns);
+trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
if (!s->synced) {
block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
if (block_job_is_cancelled(&s->common)) {
diff --git a/migration/block.c b/migration/block.c
index 9171f60028..a3512945da 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -667,7 +667,7 @@ static int64_t get_remaining_dirty(void)
aio_context_release(blk_get_aio_context(bmds->blk));
}
-return dirty << BDRV_SECTOR_BITS;
+return dirty;
}
--
2.13.5
[Qemu-block] [PATCH v7 09/20] qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based. Change the qcow2 bitmap
helper function sectors_covered_by_bitmap_cluster(), renaming it
to bytes_covered_by_bitmap_cluster() in the process.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: no change
v4: new patch
---
block/qcow2-bitmap.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 92098bfa49..4475273d8c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -269,18 +269,16 @@ static int free_bitmap_clusters(BlockDriverState *bs,
Qcow2BitmapTable *tb)
return 0;
}
-/* This function returns the number of disk sectors covered by a single qcow2
- * cluster of bitmap data. */
-static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
- const BdrvDirtyBitmap
*bitmap)
+/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
+static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
+const BdrvDirtyBitmap *bitmap)
{
-uint64_t sector_granularity =
-bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
-uint64_t sbc = sector_granularity * (s->cluster_size << 3);
+uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+uint64_t limit = granularity * (s->cluster_size << 3);
-assert(QEMU_IS_ALIGNED(sbc << BDRV_SECTOR_BITS,
+assert(QEMU_IS_ALIGNED(limit,
bdrv_dirty_bitmap_serialization_align(bitmap)));
-return sbc;
+return limit;
}
/* load_bitmap_data
@@ -293,7 +291,7 @@ static int load_bitmap_data(BlockDriverState *bs,
{
int ret = 0;
BDRVQcow2State *s = bs->opaque;
-uint64_t sector, sbc;
+uint64_t sector, limit, sbc;
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
uint8_t *buf = NULL;
@@ -306,7 +304,8 @@ static int load_bitmap_data(BlockDriverState *bs,
}
buf = g_malloc(s->cluster_size);
-sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
+limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+sbc = limit >> BDRV_SECTOR_BITS;
for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
uint64_t count = MIN(bm_sectors - sector, sbc);
uint64_t entry = bitmap_table[i];
@@ -1080,7 +1079,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
int ret;
BDRVQcow2State *s = bs->opaque;
int64_t sector;
-uint64_t sbc;
+uint64_t limit, sbc;
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
@@ -1106,8 +1105,9 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
dbi = bdrv_dirty_iter_new(bitmap, 0);
buf = g_malloc(s->cluster_size);
-sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
-assert(DIV_ROUND_UP(bm_sectors, sbc) == tb_size);
+limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+sbc = limit >> BDRV_SECTOR_BITS;
+assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
uint64_t cluster = sector / sbc;
--
2.13.5
[Qemu-block] [PATCH v7 07/20] dirty-bitmap: Track bitmap size by bytes
We are still using an internal hbitmap that tracks a size in sectors,
with the granularity scaled down accordingly, because it lets us
use a shortcut for our iterators which are currently sector-based.
But there's no reason we can't track the dirty bitmap size in bytes,
since it is (mostly) an internal-only variable (remember, the size
is how many bytes are covered by the bitmap, not how many bytes the
bitmap occupies). A later cleanup will convert dirty bitmap
internals to be entirely byte-based, eliminating the intermediate
sector rounding added here; and technically, since bdrv_getlength()
already rounds up to sectors, our use of DIV_ROUND_UP is more for
theoretical completeness than for any actual rounding.
Use is_power_of_2() while at it, instead of open-coding that.
Signed-off-by: Eric Blake
---
v7: split external from internal [Kevin], drop R-b
v6: no change
v5: fix bdrv_dirty_bitmap_truncate [John], drop R-b
v4: retitle from "Track size in bytes", rebase to persistent bitmaps,
round up when converting bytes to sectors
v3: no change
v2: tweak commit message, no code change
---
block/dirty-bitmap.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 56a01699e9..ec4fc0e1ad 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -42,7 +42,7 @@ struct BdrvDirtyBitmap {
HBitmap *meta; /* Meta dirty bitmap */
BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
char *name; /* Optional non-empty unique ID */
-int64_t size; /* Size of the bitmap (Number of sectors) */
+int64_t size; /* Size of the bitmap, in bytes */
bool disabled; /* Bitmap is disabled. It ignores all writes to
the device */
int active_iterators; /* How many iterators are active */
@@ -115,17 +115,14 @@ BdrvDirtyBitmap
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
{
int64_t bitmap_size;
BdrvDirtyBitmap *bitmap;
-uint32_t sector_granularity;
-assert((granularity & (granularity - 1)) == 0);
+assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);
if (name && bdrv_find_dirty_bitmap(bs, name)) {
error_setg(errp, "Bitmap already exists: %s", name);
return NULL;
}
-sector_granularity = granularity >> BDRV_SECTOR_BITS;
-assert(sector_granularity);
-bitmap_size = bdrv_nb_sectors(bs);
+bitmap_size = bdrv_getlength(bs);
if (bitmap_size < 0) {
error_setg_errno(errp, -bitmap_size, "could not get length of device");
errno = -bitmap_size;
@@ -133,7 +130,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState
*bs,
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->mutex = &bs->dirty_bitmap_mutex;
-bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
+/*
+ * TODO - let hbitmap track full granularity. For now, it is tracking
+ * only sector granularity, as a shortcut for our iterators.
+ */
+bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap_size, BDRV_SECTOR_SIZE),
+ ctz32(granularity) - BDRV_SECTOR_BITS);
bitmap->size = bitmap_size;
bitmap->name = g_strdup(name);
bitmap->disabled = false;
@@ -175,7 +177,7 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
{
-return bitmap->size * BDRV_SECTOR_SIZE;
+return bitmap->size;
}
const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
@@ -305,7 +307,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState
*bs,
int bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
{
BdrvDirtyBitmap *bitmap;
-int64_t size = bdrv_nb_sectors(bs);
+int64_t size = bdrv_getlength(bs);
if (size < 0) {
return size;
@@ -314,7 +316,7 @@ int bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
assert(!bdrv_dirty_bitmap_frozen(bitmap));
assert(!bitmap->active_iterators);
-hbitmap_truncate(bitmap->bitmap, size);
+hbitmap_truncate(bitmap->bitmap, DIV_ROUND_UP(size, BDRV_SECTOR_SIZE));
bitmap->size = size;
}
bdrv_dirty_bitmaps_unlock(bs);
@@ -553,7 +555,8 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
HBitmap **out)
hbitmap_reset_all(bitmap->bitmap);
} else {
HBitmap *backup = bitmap->bitmap;
-bitmap->bitmap = hbitmap_alloc(bitmap->size,
+bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap->size,
+BDRV_SECTOR_SIZE),
hbitmap_granularity(backup));
*out = backup;
}
--
2.13.5
[Qemu-block] [PATCH v7 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset
Thanks to recent cleanups, most callers were scaling a return value
of sectors into bytes (the exception, in qcow2-bitmap, will be
converted to byte-based iteration later). Update the interface to
do the scaling internally instead.
In qcow2-bitmap, the code was specifically checking for an error
to be -1; it is more robust to treat all negative values as an
error, but at the same time it is also easy enough to ensure we
return -1 (and not -512) on error.
Signed-off-by: Eric Blake
---
v7: return -1, not -512; and fix qcow2-bitmap to check all negatives [Kevin]
v5-v6: no change
v4: rebase to persistent bitmap
v3: no change
v2: no change
---
block/backup.c | 2 +-
block/dirty-bitmap.c | 3 ++-
block/mirror.c | 8
block/qcow2-bitmap.c | 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index ac9c018717..06ddbfd03d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -375,7 +375,7 @@ static int coroutine_fn
backup_run_incremental(BackupBlockJob *job)
dbi = bdrv_dirty_iter_new(job->sync_bitmap);
/* Find the next dirty sector(s) */
-while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
+while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
cluster = offset / job->cluster_size;
/* Fake progress updates for any clusters we skipped */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d50c46621d..49229fd501 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -508,7 +508,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
{
-return hbitmap_iter_next(&iter->hbi);
+int64_t ret = hbitmap_iter_next(&iter->hbi);
+return ret < 0 ? -1 : ret * BDRV_SECTOR_SIZE;
}
/* Called within bdrv_dirty_bitmap_lock..unlock */
diff --git a/block/mirror.c b/block/mirror.c
index 0b063b3c20..77bf5aa3a4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -336,10 +336,10 @@ static uint64_t coroutine_fn
mirror_iteration(MirrorBlockJob *s)
int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
bdrv_dirty_bitmap_lock(s->dirty_bitmap);
-offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+offset = bdrv_dirty_iter_next(s->dbi);
if (offset < 0) {
bdrv_set_dirty_iter(s->dbi, 0);
-offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+offset = bdrv_dirty_iter_next(s->dbi);
trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
BDRV_SECTOR_SIZE);
assert(offset >= 0);
@@ -370,11 +370,11 @@ static uint64_t coroutine_fn
mirror_iteration(MirrorBlockJob *s)
break;
}
-next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+next_dirty = bdrv_dirty_iter_next(s->dbi);
if (next_dirty > next_offset || next_dirty < 0) {
/* The bitmap iterator's cache is stale, refresh it */
bdrv_set_dirty_iter(s->dbi, next_offset);
-next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+next_dirty = bdrv_dirty_iter_next(s->dbi);
}
assert(next_dirty == next_offset);
nb_chunks++;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 44329fc74f..b09010b1d3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1109,7 +1109,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
sbc = limit >> BDRV_SECTOR_BITS;
assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
-while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) >= 0) {
uint64_t cluster = sector / sbc;
uint64_t end, write_size;
int64_t off;
--
2.13.5
[Qemu-block] [PATCH v7 10/20] dirty-bitmap: Set iterator start by offset, not sector
All callers to bdrv_dirty_iter_new() passed 0 for their initial
starting point, drop that parameter.
Most callers to bdrv_set_dirty_iter() were scaling a byte offset to
a sector number; the exception qcow2-bitmap will be converted later
to use byte rather than sector iteration. Move the scaling to occur
internally to dirty bitmap code instead, so that callers now pass
in bytes.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: no change
v4: rebase to persistent bitmaps
v3: no change
v2: no change
---
include/block/dirty-bitmap.h | 5 ++---
block/backup.c | 5 ++---
block/dirty-bitmap.c | 9 -
block/mirror.c | 4 ++--
block/qcow2-bitmap.c | 4 ++--
5 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index cd32149db6..842e57416c 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -44,8 +44,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors);
BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
-BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
- uint64_t first_sector);
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
@@ -80,7 +79,7 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors);
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
int bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
diff --git a/block/backup.c b/block/backup.c
index 517c300a49..ac9c018717 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -372,7 +372,7 @@ static int coroutine_fn
backup_run_incremental(BackupBlockJob *job)
granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
clusters_per_iter = MAX((granularity / job->cluster_size), 1);
-dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
+dbi = bdrv_dirty_iter_new(job->sync_bitmap);
/* Find the next dirty sector(s) */
while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
@@ -403,8 +403,7 @@ static int coroutine_fn
backup_run_incremental(BackupBlockJob *job)
/* If the bitmap granularity is smaller than the backup granularity,
* we need to advance the iterator pointer to the next cluster. */
if (granularity < job->cluster_size) {
-bdrv_set_dirty_iter(dbi,
-cluster * job->cluster_size /
BDRV_SECTOR_SIZE);
+bdrv_set_dirty_iter(dbi, cluster * job->cluster_size);
}
last_cluster = cluster - 1;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index dbf5a53044..d50c46621d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -478,11 +478,10 @@ uint32_t bdrv_dirty_bitmap_granularity(const
BdrvDirtyBitmap *bitmap)
return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
}
-BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
- uint64_t first_sector)
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
{
BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
+hbitmap_iter_init(&iter->hbi, bitmap->bitmap, 0);
iter->bitmap = bitmap;
bitmap->active_iterators++;
return iter;
@@ -650,9 +649,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
cur_sector,
/**
* Advance a BdrvDirtyBitmapIter to an arbitrary offset.
*/
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
{
-hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num);
+hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
}
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index 6531652d73..0b063b3c20 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -373,7 +373,7 @@ static uint64_t coroutine_fn
mirror_iteration(MirrorBlockJob *s)
next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
if (next_dirty > next_offset || next_dirty < 0) {
/* The bitmap iterator's cache is stale, refresh it */
-bdrv_set_dirty_iter(s->dbi, next_offset >> BDRV_SECTOR_BITS)
[Qemu-block] [PATCH v7 13/20] dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes
Half the callers were already scaling bytes to sectors; the other
half can eventually be simplified to use byte iteration. Both
callers were already using the result as a bool, so make that
explicit. Making the change also makes it easier for a future
dirty-bitmap patch to offload scaling over to the internal hbitmap.
Remember, asking whether a byte is dirty is effectively asking
whether the entire granularity containing the byte is dirty, since
we only track dirtiness by granularity.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
Reviewed-by: Juan Quintela
---
v4: only context change
v3: rebase to _locked rename was straightforward enough that R-b kept
v2: tweak commit message, no code change
---
include/block/dirty-bitmap.h | 4 ++--
block/dirty-bitmap.c | 8
block/mirror.c | 3 +--
migration/block.c| 3 ++-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 842e57416c..94a8d76f26 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -72,8 +72,8 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap
*bitmap,
/* Functions that require manual locking. */
void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap);
-int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
- int64_t sector);
+bool bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t offset);
void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors);
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index ee0afb5e1a..96b15d232a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -443,13 +443,13 @@ BlockDirtyInfoList
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
}
/* Called within bdrv_dirty_bitmap_lock..unlock */
-int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
- int64_t sector)
+bool bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t offset)
{
if (bitmap) {
-return hbitmap_get(bitmap->bitmap, sector);
+return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
} else {
-return 0;
+return false;
}
}
diff --git a/block/mirror.c b/block/mirror.c
index 7113d47db4..e36fc81df3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -361,8 +361,7 @@ static uint64_t coroutine_fn
mirror_iteration(MirrorBlockJob *s)
int64_t next_offset = offset + nb_chunks * s->granularity;
int64_t next_chunk = next_offset / s->granularity;
if (next_offset >= s->bdev_length ||
-!bdrv_get_dirty_locked(source, s->dirty_bitmap,
- next_offset >> BDRV_SECTOR_BITS)) {
+!bdrv_get_dirty_locked(source, s->dirty_bitmap, next_offset)) {
break;
}
if (test_bit(next_chunk, s->in_flight_bitmap)) {
diff --git a/migration/block.c b/migration/block.c
index a3512945da..b618869661 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -530,7 +530,8 @@ static int mig_save_device_dirty(QEMUFile *f,
BlkMigDevState *bmds,
blk_mig_unlock();
}
bdrv_dirty_bitmap_lock(bmds->dirty_bitmap);
-if (bdrv_get_dirty_locked(bs, bmds->dirty_bitmap, sector)) {
+if (bdrv_get_dirty_locked(bs, bmds->dirty_bitmap,
+ sector * BDRV_SECTOR_SIZE)) {
if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
nr_sectors = total_sectors - sector;
} else {
--
2.13.5
[Qemu-block] [PATCH v7 20/20] dirty-bitmap: Convert internal hbitmap size/granularity
Now that all callers are using byte-based interfaces, there's no
reason for our internal hbitmap to remain with sector-based
granularity. It also simplifies our internal scaling, since we
already know that hbitmap widens requests out to granularity
boundaries.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v7: rebase to dirty_iter_next cleanup (no semantic change, R-b kept)
v6: no change
v5: fix bdrv_dirty_bitmap_truncate [John]
v4: rebase to earlier changes, include serialization, R-b dropped
v3: no change
v2: no change
---
block/dirty-bitmap.c | 62 +++-
1 file changed, 18 insertions(+), 44 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index dd91f56b94..c2322c2619 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -38,7 +38,7 @@
*/
struct BdrvDirtyBitmap {
QemuMutex *mutex;
-HBitmap *bitmap;/* Dirty sector bitmap implementation */
+HBitmap *bitmap;/* Dirty bitmap implementation */
HBitmap *meta; /* Meta dirty bitmap */
BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
char *name; /* Optional non-empty unique ID */
@@ -130,12 +130,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState
*bs,
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->mutex = &bs->dirty_bitmap_mutex;
-/*
- * TODO - let hbitmap track full granularity. For now, it is tracking
- * only sector granularity, as a shortcut for our iterators.
- */
-bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap_size, BDRV_SECTOR_SIZE),
- ctz32(granularity) - BDRV_SECTOR_BITS);
+bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
bitmap->size = bitmap_size;
bitmap->name = g_strdup(name);
bitmap->disabled = false;
@@ -316,7 +311,7 @@ int bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
assert(!bdrv_dirty_bitmap_frozen(bitmap));
assert(!bitmap->active_iterators);
-hbitmap_truncate(bitmap->bitmap, DIV_ROUND_UP(size, BDRV_SECTOR_SIZE));
+hbitmap_truncate(bitmap->bitmap, size);
bitmap->size = size;
}
bdrv_dirty_bitmaps_unlock(bs);
@@ -447,7 +442,7 @@ bool bdrv_get_dirty_locked(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
int64_t offset)
{
if (bitmap) {
-return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
+return hbitmap_get(bitmap->bitmap, offset);
} else {
return false;
}
@@ -475,7 +470,7 @@ uint32_t
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
{
-return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+return 1U << hbitmap_granularity(bitmap->bitmap);
}
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
@@ -508,20 +503,16 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
{
-int64_t ret = hbitmap_iter_next(&iter->hbi);
-return ret < 0 ? -1 : ret * BDRV_SECTOR_SIZE;
+return hbitmap_iter_next(&iter->hbi);
}
/* Called within bdrv_dirty_bitmap_lock..unlock */
void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
{
-int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
-
assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
-end_sector - (offset >> BDRV_SECTOR_BITS));
+hbitmap_set(bitmap->bitmap, offset, bytes);
}
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
@@ -536,12 +527,9 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
{
-int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
-
assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
- end_sector - (offset >> BDRV_SECTOR_BITS));
+hbitmap_reset(bitmap->bitmap, offset, bytes);
}
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
@@ -561,8 +549,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
HBitmap **out)
hbitmap_reset_all(bitmap->bitmap);
} else {
HBitmap *backup = bitmap->bitmap;
-bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap->size,
-BDRV_SECTOR_SIZE),
+bitmap->bitmap = hbitmap_alloc(bitmap->size,
hbitmap_granularity(backup));
*out = backup;
}
@@ -581,51 +
[Qemu-block] [PATCH v7 14/20] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes
Some of the callers were already scaling bytes to sectors; others
can be easily converted to pass byte offsets, all in our shift
towards a consistent byte interface everywhere. Making the change
will also make it easier to write the hold-out callers to use byte
rather than sectors for their iterations; it also makes it easier
for a future dirty-bitmap patch to offload scaling over to the
internal hbitmap. Although all callers happen to pass
sector-aligned values, make the internal scaling robust to any
sub-sector requests.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v5: only context change
v4: only context change, due to rebasing to persistent bitmaps
v3: rebase to addition of _locked interfaces; complex enough that I
dropped R-b
v2: no change
---
include/block/dirty-bitmap.h | 8
block/dirty-bitmap.c | 22 ++
block/mirror.c | 16
migration/block.c| 7 +--
4 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 94a8d76f26..252bb25c8e 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -40,9 +40,9 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap
*bitmap);
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors);
+ int64_t offset, int64_t bytes);
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors);
+ int64_t offset, int64_t bytes);
BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
@@ -75,9 +75,9 @@ void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap);
bool bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t offset);
void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors);
+ int64_t offset, int64_t bytes);
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
-int64_t cur_sector, int64_t nr_sectors);
+int64_t offset, int64_t bytes);
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 96b15d232a..b1bbb98653 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -514,35 +514,41 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
/* Called within bdrv_dirty_bitmap_lock..unlock */
void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors)
+ int64_t offset, int64_t bytes)
{
+int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+end_sector - (offset >> BDRV_SECTOR_BITS));
}
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors)
+ int64_t offset, int64_t bytes)
{
bdrv_dirty_bitmap_lock(bitmap);
-bdrv_set_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes);
bdrv_dirty_bitmap_unlock(bitmap);
}
/* Called within bdrv_dirty_bitmap_lock..unlock */
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
-int64_t cur_sector, int64_t nr_sectors)
+int64_t offset, int64_t bytes)
{
+int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+ end_sector - (offset >> BDRV_SECTOR_BITS));
}
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors)
+ int64_t offset, int64_t bytes)
{
bdrv_dirty_bitmap_lock(bitmap);
-bdrv_reset_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+bdrv_reset_dirty_bitmap_locked(bitmap, offset, bytes);
bdrv_dir
[Qemu-block] [PATCH v7 16/20] qcow2: Switch qcow2_measure() to byte-based iteration
This is new code, but it is easier to read if it makes passes over
the image using bytes rather than sectors (and will get easier in
the future when bdrv_get_block_status is converted to byte-based).
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v7: tweak constant given to MIN (no semantic change, R-b kept) [Kevin]
v6: separate bug fix to earlier patch
v5: new patch
---
block/qcow2.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index bae5893327..64dcd98a91 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3647,20 +3647,19 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts,
BlockDriverState *in_bs,
*/
required = virtual_size;
} else {
-int cluster_sectors = cluster_size / BDRV_SECTOR_SIZE;
-int64_t sector_num;
+int64_t offset;
int pnum = 0;
-for (sector_num = 0;
- sector_num < ssize / BDRV_SECTOR_SIZE;
- sector_num += pnum) {
-int nb_sectors = MIN(ssize / BDRV_SECTOR_SIZE - sector_num,
- BDRV_REQUEST_MAX_SECTORS);
+for (offset = 0; offset < ssize;
+ offset += pnum * BDRV_SECTOR_SIZE) {
+int nb_sectors = MIN(ssize - offset,
+ BDRV_REQUEST_MAX_BYTES) /
BDRV_SECTOR_SIZE;
BlockDriverState *file;
int64_t ret;
ret = bdrv_get_block_status_above(in_bs, NULL,
- sector_num, nb_sectors,
+ offset >> BDRV_SECTOR_BITS,
+ nb_sectors,
&pnum, &file);
if (ret < 0) {
error_setg_errno(&local_err, -ret,
@@ -3673,12 +3672,11 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts,
BlockDriverState *in_bs,
} else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
(BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
/* Extend pnum to end of cluster for next iteration */
-pnum = ROUND_UP(sector_num + pnum, cluster_sectors) -
- sector_num;
+pnum = (ROUND_UP(offset + pnum * BDRV_SECTOR_SIZE,
+ cluster_size) - offset) >> BDRV_SECTOR_BITS;
/* Count clusters we've seen */
-required += (sector_num % cluster_sectors + pnum) *
-BDRV_SECTOR_SIZE;
+required += offset % cluster_size + pnum *
BDRV_SECTOR_SIZE;
}
}
}
--
2.13.5
[Qemu-block] [PATCH v7 15/20] mirror: Switch mirror_dirty_init() to byte-based iteration
Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v6: no change
v5: rebase to earlier changes
v2-v4: no change
---
block/mirror.c | 38 ++
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index c2f73c91c5..5cdaaed7be 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -612,15 +612,13 @@ static void mirror_throttle(MirrorBlockJob *s)
static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
{
-int64_t sector_num, end;
+int64_t offset;
BlockDriverState *base = s->base;
BlockDriverState *bs = s->source;
BlockDriverState *target_bs = blk_bs(s->target);
-int ret, n;
+int ret;
int64_t count;
-end = s->bdev_length / BDRV_SECTOR_SIZE;
-
if (base == NULL && !bdrv_has_zero_init(target_bs)) {
if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
@@ -628,9 +626,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
}
s->initial_zeroing_ongoing = true;
-for (sector_num = 0; sector_num < end; ) {
-int nb_sectors = MIN(end - sector_num,
-QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS);
+for (offset = 0; offset < s->bdev_length; ) {
+int bytes = MIN(s->bdev_length - offset,
+QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
mirror_throttle(s);
@@ -646,9 +644,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
continue;
}
-mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE, false);
-sector_num += nb_sectors;
+mirror_do_zero_or_discard(s, offset, bytes, false);
+offset += bytes;
}
mirror_wait_for_all_io(s);
@@ -656,10 +653,10 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
*s)
}
/* First part, loop on the sectors and initialize the dirty bitmap. */
-for (sector_num = 0; sector_num < end; ) {
+for (offset = 0; offset < s->bdev_length; ) {
/* Just to make sure we are not exceeding int limit. */
-int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
- end - sector_num);
+int bytes = MIN(s->bdev_length - offset,
+QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
mirror_throttle(s);
@@ -667,23 +664,16 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
*s)
return 0;
}
-ret = bdrv_is_allocated_above(bs, base, sector_num * BDRV_SECTOR_SIZE,
- nb_sectors * BDRV_SECTOR_SIZE, &count);
+ret = bdrv_is_allocated_above(bs, base, offset, bytes, &count);
if (ret < 0) {
return ret;
}
-/* TODO: Relax this once bdrv_is_allocated_above and dirty
- * bitmaps no longer require sector alignment. */
-assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
-n = count >> BDRV_SECTOR_BITS;
-assert(n > 0);
+assert(count);
if (ret == 1) {
-bdrv_set_dirty_bitmap(s->dirty_bitmap,
- sector_num * BDRV_SECTOR_SIZE,
- n * BDRV_SECTOR_SIZE);
+bdrv_set_dirty_bitmap(s->dirty_bitmap, offset, count);
}
-sector_num += n;
+offset += count;
}
return 0;
}
--
2.13.5
[Qemu-block] [PATCH v7 17/20] qcow2: Switch load_bitmap_data() to byte-based iteration
Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v5: no change
v4: new patch
---
block/qcow2-bitmap.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b09010b1d3..692ce0de88 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -291,9 +291,8 @@ static int load_bitmap_data(BlockDriverState *bs,
{
int ret = 0;
BDRVQcow2State *s = bs->opaque;
-uint64_t sector, limit, sbc;
+uint64_t offset, limit;
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
-uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
uint8_t *buf = NULL;
uint64_t i, tab_size =
size_to_clusters(s,
@@ -305,32 +304,27 @@ static int load_bitmap_data(BlockDriverState *bs,
buf = g_malloc(s->cluster_size);
limit = bytes_covered_by_bitmap_cluster(s, bitmap);
-sbc = limit >> BDRV_SECTOR_BITS;
-for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
-uint64_t count = MIN(bm_sectors - sector, sbc);
+for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
+uint64_t count = MIN(bm_size - offset, limit);
uint64_t entry = bitmap_table[i];
-uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+uint64_t data_offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
assert(check_table_entry(entry, s->cluster_size) == 0);
-if (offset == 0) {
+if (data_offset == 0) {
if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
-bdrv_dirty_bitmap_deserialize_ones(bitmap,
- sector * BDRV_SECTOR_SIZE,
- count * BDRV_SECTOR_SIZE,
+bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count,
false);
} else {
/* No need to deserialize zeros because the dirty bitmap is
* already cleared */
}
} else {
-ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
+ret = bdrv_pread(bs->file, data_offset, buf, s->cluster_size);
if (ret < 0) {
goto finish;
}
-bdrv_dirty_bitmap_deserialize_part(bitmap, buf,
- sector * BDRV_SECTOR_SIZE,
- count * BDRV_SECTOR_SIZE,
+bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count,
false);
}
}
--
2.13.5
Re: [Qemu-block] [Qemu-devel] [PATCH 01/10] qemu-iotests: remove dead code
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > This includes shell function, shell variables and command line options > (randomize.awk does not exist). > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/check | 28 - > tests/qemu-iotests/common| 23 -- > tests/qemu-iotests/common.config | 26 --- > tests/qemu-iotests/common.rc | 68 > > 4 files changed, 145 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH v7 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration
Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v7: rebase to earlier change, make rounding of offset obvious (no semantic
change, so R-b kept) [Kevin]
v5-v6: no change
v4: new patch
---
block/qcow2-bitmap.c | 26 +++---
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 692ce0de88..302fffd6e1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1072,10 +1072,9 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
{
int ret;
BDRVQcow2State *s = bs->opaque;
-int64_t sector;
-uint64_t limit, sbc;
+int64_t offset;
+uint64_t limit;
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
-uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
uint8_t *buf = NULL;
BdrvDirtyBitmapIter *dbi;
@@ -1100,18 +1099,17 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
dbi = bdrv_dirty_iter_new(bitmap);
buf = g_malloc(s->cluster_size);
limit = bytes_covered_by_bitmap_cluster(s, bitmap);
-sbc = limit >> BDRV_SECTOR_BITS;
assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
-while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) >= 0) {
-uint64_t cluster = sector / sbc;
+while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
+uint64_t cluster = offset / limit;
uint64_t end, write_size;
int64_t off;
-sector = cluster * sbc;
-end = MIN(bm_sectors, sector + sbc);
-write_size = bdrv_dirty_bitmap_serialization_size(bitmap,
-sector * BDRV_SECTOR_SIZE, (end - sector) * BDRV_SECTOR_SIZE);
+offset = QEMU_ALIGN_DOWN(offset, limit);
+end = MIN(bm_size, offset + limit);
+write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+ end - offset);
assert(write_size <= s->cluster_size);
off = qcow2_alloc_clusters(bs, s->cluster_size);
@@ -1123,9 +1121,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
}
tb[cluster] = off;
-bdrv_dirty_bitmap_serialize_part(bitmap, buf,
- sector * BDRV_SECTOR_SIZE,
- (end - sector) * BDRV_SECTOR_SIZE);
+bdrv_dirty_bitmap_serialize_part(bitmap, buf, offset, end - offset);
if (write_size < s->cluster_size) {
memset(buf + write_size, 0, s->cluster_size - write_size);
}
@@ -1143,11 +1139,11 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
goto fail;
}
-if (end >= bm_sectors) {
+if (end >= bm_size) {
break;
}
-bdrv_set_dirty_iter(dbi, end * BDRV_SECTOR_SIZE);
+bdrv_set_dirty_iter(dbi, end);
}
*bitmap_table_size = tb_size;
--
2.13.5
[Qemu-block] [PATCH v7 19/20] dirty-bitmap: Switch bdrv_set_dirty() to bytes
Both callers already had bytes available, but were scaling to
sectors. Move the scaling to internal code. In the case of
bdrv_aligned_pwritev(), we are now passing the exact offset
rather than a rounded sector-aligned value, but that's okay
as long as dirty bitmap widens start/bytes to granularity
boundaries.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v4: only context changes
v3: rebase to lock context changes, R-b kept
v2: no change
---
include/block/block_int.h | 2 +-
block/io.c| 6 ++
block/dirty-bitmap.c | 7 ---
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..55c5d573d4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1021,7 +1021,7 @@ void blk_dev_eject_request(BlockBackend *blk, bool force);
bool blk_dev_is_tray_open(BlockBackend *blk);
bool blk_dev_is_medium_locked(BlockBackend *blk);
-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int64_t nr_sect);
+void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
bool bdrv_requests_pending(BlockDriverState *bs);
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..8a0cd8835a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1334,7 +1334,6 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild
*child,
bool waited;
int ret;
-int64_t start_sector = offset >> BDRV_SECTOR_BITS;
int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
uint64_t bytes_remaining = bytes;
int max_transfer;
@@ -1409,7 +1408,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild
*child,
bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);
atomic_inc(&bs->write_gen);
-bdrv_set_dirty(bs, start_sector, end_sector - start_sector);
+bdrv_set_dirty(bs, offset, bytes);
stat64_max(&bs->wr_highest_offset, offset + bytes);
@@ -2438,8 +2437,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs,
int64_t offset,
ret = 0;
out:
atomic_inc(&bs->write_gen);
-bdrv_set_dirty(bs, req.offset >> BDRV_SECTOR_BITS,
- req.bytes >> BDRV_SECTOR_BITS);
+bdrv_set_dirty(bs, req.offset, req.bytes);
tracked_request_end(&req);
bdrv_dec_in_flight(bs);
return ret;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index b1bbb98653..dd91f56b94 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -633,10 +633,10 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap
*bitmap)
hbitmap_deserialize_finish(bitmap->bitmap);
}
-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
-int64_t nr_sectors)
+void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
BdrvDirtyBitmap *bitmap;
+int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
if (QLIST_EMPTY(&bs->dirty_bitmaps)) {
return;
@@ -648,7 +648,8 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
cur_sector,
continue;
}
assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+end_sector - (offset >> BDRV_SECTOR_BITS));
}
bdrv_dirty_bitmaps_unlock(bs);
}
--
2.13.5
Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] qemu-iotests: get rid of AWK_PROG
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/check | 4 ++-- > tests/qemu-iotests/common| 2 +- > tests/qemu-iotests/common.config | 3 --- > 3 files changed, 3 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] qemu-iotests: move "check" code out of common.rc
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > Some functions in common.rc are never used by the tests. Move > them out of that file and into common, which is already included > only by "check". > > Code that actually *is* common to "check" and tests can be placed in > common.config. > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/common| 25 - > tests/qemu-iotests/common.config | 12 > tests/qemu-iotests/common.rc | 40 > > 3 files changed, 36 insertions(+), 41 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] qemu-iotests: cleanup and fix search for programs
On 09/12/2017 09:44 AM, Paolo Bonzini wrote:
> Instead of ./check failing when a binary is missing, we try each test
> case now and each one fails with tons of test case diffs. Also, all the
> variables were initialized by "check" prior to "common" being sourced,
> and then (uselessly) checked for emptiness again in "check".
>
> Centralize the search for programs in "common" (which will soon be
> one with "check"), including the "realpath" invocation which can be done
> just once in "check" rather than in the tests.
>
> For qnio_server, move the detection to "common", simplifying
> set_prog_path to stop handling the unused second argument, and
> embedding the "realpath" pass.
>
> Signed-off-by: Paolo Bonzini
> ---
> tests/qemu-iotests/check | 41 -
> tests/qemu-iotests/common| 77
> +---
> tests/qemu-iotests/common.config | 61 +--
> 3 files changed, 73 insertions(+), 106 deletions(-)
>
> +++ b/tests/qemu-iotests/common
> @@ -37,6 +37,17 @@ _full_platform_details()
> echo "$os/$platform $host $kernel"
> }
>
> +# $1 = prog to look for
> +set_prog_path()
> +{
> +p=`command -v $1 2> /dev/null`
Are we sure that $1 will always respond to -v sanely?
> +if [ -n "$p" -a -x "$p" ]; then
[ ... -a ... ] should never be used; there are cases where it is
inherently ambiguous (even if it happens to work here, because we know
our shell is bash and because $p is not likely to contain text that
throws off the parser). Please spell that either:
if [ -n "$p" ] && [ -x "$p" ]; then
if [[ -n $p && -x $p ]]; then
(that is, I don't mind if you rely on bash's [[]] in which case you can
use less typing, but if you stick to [], then you should use code that
can be pasted to other shells)
> +then
> +if [ -x "$build_iotests/qemu" ]; then
> +export QEMU_PROG="$build_iotests/qemu"
> +elif [ -x "$build_root/$arch-softmmu/qemu-system-$arch" ]; then
> +export QEMU_PROG="$build_root/$arch-softmmu/qemu-system-$arch"
> +else
> +pushd "$build_root" > /dev/null
Shouldn't you check for failure to change directories?
> +++ b/tests/qemu-iotests/common.config
> @@ -22,6 +22,7 @@ export LANG=C
> PATH=".:$PATH"
>
> HOSTOS=`uname -s`
> +arch=`uname -m`
As long as we're touching this, should we prefer $() instead of ``?
>
> export PWD=`pwd`
>
> @@ -30,28 +31,6 @@ export _QEMU_HANDLE=0
> # make sure we have a standard umask
> umask 022
>
> -# $1 = prog to look for, $2* = default pathnames if not found in $PATH
> -set_prog_path()
> -{
> -p=`command -v $1 2> /dev/null`
> -if [ -n "$p" -a -x "$p" ]; then
Urrgh - your use of [ -a ] was pre-existing, and you just moved it.
Still worth fixing, though (either here or in a separate cleanup patch).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 05/10] qemu-iotests: limit non-_PROG-suffixed variables to common.rc
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > These are never used by "check", with one exception that does not need > $QEMU_OPTIONS. Keep them in common.rc, which will be soon included only > by the tests. > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/039.out | 10 +++--- > tests/qemu-iotests/061.out | 4 +-- > tests/qemu-iotests/137.out | 2 +- > tests/qemu-iotests/common.config | 69 > ++-- > tests/qemu-iotests/common.rc | 65 + > 5 files changed, 75 insertions(+), 75 deletions(-) It may be worth tweaking scripts/git.orderfile to prioritize tests/qemu-iotests/common* above the rest of tests/qemu-iotests (I have that tweak locally, at any rate). But that's a separate issue. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 06/10] qemu-iotests: do not include common.rc in "check"
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > It only provides functions used by the test programs. > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/check | 6 -- > tests/qemu-iotests/common.rc | 13 + > 2 files changed, 5 insertions(+), 14 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 07/10] qemu-iotests: disintegrate more parts of common.config
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > Split "check" parts from tests part. > > For the directory setup, the actual computation of directories goes > in "check", while the sanity checks go in the tests. > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/common| 24 > tests/qemu-iotests/common.config | 49 > > tests/qemu-iotests/common.qemu | 1 + > tests/qemu-iotests/common.rc | 25 > 4 files changed, 50 insertions(+), 49 deletions(-) > > diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common > index abacc24114..ee313af92f 100644 > --- a/tests/qemu-iotests/common > +++ b/tests/qemu-iotests/common > @@ -48,6 +48,14 @@ set_prog_path() > fi > } > > +if [ -z "$TEST_DIR" ]; then > +TEST_DIR=`pwd`/scratch 8-space indent looks odd compared to the rest of the file. > @@ -153,6 +158,26 @@ else > fi > ORIG_TEST_IMG="$TEST_IMG" > > +if [ -z "$TEST_DIR" ]; then > +TEST_DIR=`pwd`/scratch We probably ought to clean things up to use $PWD instead of `pwd` everywhere as a minor optimization (why fork, when bash already has the right information for you?) - but that can be separate. Split looks okay to me, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] qemu-iotests: cleanup and fix search for programs
> > +then
> > +if [ -x "$build_iotests/qemu" ]; then
> > +export QEMU_PROG="$build_iotests/qemu"
> > +elif [ -x "$build_root/$arch-softmmu/qemu-system-$arch" ]; then
> > +export QEMU_PROG="$build_root/$arch-softmmu/qemu-system-$arch"
> > +else
> > +pushd "$build_root" > /dev/null
>
> Shouldn't you check for failure to change directories?
>
> > export PWD=`pwd`
> >
> > @@ -30,28 +31,6 @@ export _QEMU_HANDLE=0
> > # make sure we have a standard umask
> > umask 022
> >
> > -# $1 = prog to look for, $2* = default pathnames if not found in $PATH
> > -set_prog_path()
> > -{
> > -p=`command -v $1 2> /dev/null`
> > -if [ -n "$p" -a -x "$p" ]; then
>
> Urrgh - your use of [ -a ] was pre-existing, and you just moved it.
And same for pushd...
Paolo
Re: [Qemu-block] [Qemu-devel] [PATCH 08/10] qemu-iotests: fix uninitialized variable
On 09/12/2017 09:44 AM, Paolo Bonzini wrote:
> The variable is used in "common" but defined only after the file
> is sourced.
>
> Signed-off-by: Paolo Bonzini
> ---
> tests/qemu-iotests/check | 2 --
> tests/qemu-iotests/common | 2 ++
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
Reviewed-by: Eric Blake
> +tmp="${TEST_DIR}"/$$
Pre-existing to your code motion, but would we be any safer if $tmp also
included a use of $RANDOM? (The $$ already protects us from collisions
with a parallel run, but it is easy to guess, so if $tmp is used to
create any file that an attacker can access, running the testsuite may
expose a machine to a symlink or other attack - but we probably have
lots of those things to audit for before we can recommend running the
testsuite in an untrusted environment).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 09/10] qemu-iotests: get rid of $iam
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > The variable is almost unused, and one of the two uses is actually > uninitialized. > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/check | 5 + > tests/qemu-iotests/common.rc | 2 +- > 2 files changed, 2 insertions(+), 5 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 10/10] qemu-iotests: merge "check" and "common"
On 09/12/2017 09:44 AM, Paolo Bonzini wrote: > "check" is full of qemu-iotests--specific details. Separating it > from "common" does not make much sense anymore. > > Signed-off-by: Paolo Bonzini > --- > tests/qemu-iotests/check | 533 +++- > tests/qemu-iotests/common | 552 > -- > 2 files changed, 531 insertions(+), 554 deletions(-) > delete mode 100644 tests/qemu-iotests/common > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] qemu-iotests: cleanup and fix search for programs
On 09/12/2017 04:26 PM, Paolo Bonzini wrote:
>
>
>>> +then
>>> +if [ -x "$build_iotests/qemu" ]; then
>>> +export QEMU_PROG="$build_iotests/qemu"
>>> +elif [ -x "$build_root/$arch-softmmu/qemu-system-$arch" ]; then
>>> +export QEMU_PROG="$build_root/$arch-softmmu/qemu-system-$arch"
>>> +else
>>> +pushd "$build_root" > /dev/null
>>
>> Shouldn't you check for failure to change directories?
>>
>>> export PWD=`pwd`
>>>
>>> @@ -30,28 +31,6 @@ export _QEMU_HANDLE=0
>>> # make sure we have a standard umask
>>> umask 022
>>>
>>> -# $1 = prog to look for, $2* = default pathnames if not found in $PATH
>>> -set_prog_path()
>>> -{
>>> -p=`command -v $1 2> /dev/null`
>>> -if [ -n "$p" -a -x "$p" ]; then
>>
>> Urrgh - your use of [ -a ] was pre-existing, and you just moved it.
>
> And same for pushd...
True - since this patch is just code motion, we can do the cleanups in a
different patch, so you can have:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/10] cleanup qemu-iotests
On 12.09.2017 16:44, Paolo Bonzini wrote: > The purpose of this series is to separate the "check" sources from > the tests. After these patches, common.config is reduced to simple > shell initialization, and common.rc is only included by the tests. > > Along the way, a lot of dead code is removed too. > > In v2, the following patches: > > qemu-iotests: do not do useless search for QEMU_*_PROG > qemu-iotests: do not search for binaries in the current directory > qemu-iotests: include common.env and common.config early > > have been replaced by "qemu-iotests: cleanup and fix search for programs", > which also preserves the behavior of searching for programs as symlinks > in the current directory. > > Paolo > > Paolo Bonzini (10): > qemu-iotests: remove dead code > qemu-iotests: get rid of AWK_PROG > qemu-iotests: move "check" code out of common.rc > qemu-iotests: cleanup and fix search for programs > qemu-iotests: limit non-_PROG-suffixed variables to common.rc > qemu-iotests: do not include common.rc in "check" > qemu-iotests: disintegrate more parts of common.config > qemu-iotests: fix uninitialized variable > qemu-iotests: get rid of $iam > qemu-iotests: merge "check" and "common" > > tests/qemu-iotests/039.out | 10 +- > tests/qemu-iotests/061.out | 4 +- > tests/qemu-iotests/137.out | 2 +- > tests/qemu-iotests/check | 575 > ++- > tests/qemu-iotests/common| 459 --- > tests/qemu-iotests/common.config | 206 +- > tests/qemu-iotests/common.qemu | 1 + > tests/qemu-iotests/common.rc | 205 +++--- Meta comment: Could we maybe also rename "tests/qemu-iotests" to "tests/iotests" ? The "qemu" prefix sounds always very superfluous to me here... Thomas
