[Qemu-devel] [PATCH 0/3] iotests: Skip 162 if there is no SSH support

[Qemu-devel] [PATCH 0/3] iotests: Skip 162 if there is no SSH support

[Max Reitz]

As reported by Hao QingFeng, iotest 162 is currently executed even if
qemu does not have any SSH support (which makes it fail, naturally).

Fixing that is not so trivial, because qemu-img currently does not
report modules, and SSH can be compiled as a module, so that needs to be
fixed first. While doing so, I noticed that bdrv_iterate_format() tries
to sort the list of formats, which is a bit contrary to my experience.
Turns out that needs to be fixed, too.


This series can either be applied on top of my series "iotests: Fix test
162" or just directly on master, both works (i.e. the patches in this
series do not interfere with those from that one). I still thought I'd
mention that series, if nothing else then only to get you to review that
other one. ;-)


Max Reitz (3):
  block: Fix bdrv_iterate_format() sorting
  block: Emit modules in bdrv_iterate_format()
  iotests: Skip test 162 if there is no SSH support

 block.c                | 20 +++++++++++++++++++-
 tests/qemu-iotests/162 |  3 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.10.0

[Qemu-devel] [PATCH 1/3] block: Fix bdrv_iterate_format() sorting

[Max Reitz]

bdrv_iterate_format() did not actually sort the formats by name but by
"pointer interpreted as string". That is probably not what we intended
to do, so fix it (by changing qsort_strcmp() so it matches the example
from qsort()'s manual page).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index bb1f1ec..e46e4b2 100644
--- a/block.c
+++ b/block.c
@@ -2789,7 +2789,7 @@ const char *bdrv_get_format_name(BlockDriverState *bs)
 
 static int qsort_strcmp(const void *a, const void *b)
 {
-    return strcmp(a, b);
+    return strcmp(*(char *const *)a, *(char *const *)b);
 }
 
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
-- 
2.10.0

Re: [Qemu-devel] [PATCH 1/3] block: Fix bdrv_iterate_format() sorting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On 10/12/2016 03:49 PM, Max Reitz wrote:
> bdrv_iterate_format() did not actually sort the formats by name but by
> "pointer interpreted as string". That is probably not what we intended
> to do, so fix it (by changing qsort_strcmp() so it matches the example
> from qsort()'s manual page).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'm a bit surprised that code sanitizers like Coverity or ASAN aren't
(yet?) able to flag this.

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

> 
> diff --git a/block.c b/block.c
> index bb1f1ec..e46e4b2 100644
> --- a/block.c
> +++ b/block.c
> @@ -2789,7 +2789,7 @@ const char *bdrv_get_format_name(BlockDriverState *bs)
>  
>  static int qsort_strcmp(const void *a, const void *b)
>  {
> -    return strcmp(a, b);
> +    return strcmp(*(char *const *)a, *(char *const *)b);
>  }
>  
>  void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Max Reitz]

Some block drivers may not be loaded yet, but qemu supports them
nonetheless. bdrv_iterate_format() should report them, too.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block.c b/block.c
index e46e4b2..88a1ea5 100644
--- a/block.c
+++ b/block.c
@@ -2815,6 +2815,24 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
         }
     }
 
+    for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); i++) {
+        const char *format_name = block_driver_modules[i].format_name;
+
+        if (format_name) {
+            bool found = false;
+            int j = count;
+
+            while (formats && j && !found) {
+                found = !strcmp(formats[--j], format_name);
+            }
+
+            if (!found) {
+                formats = g_renew(const char *, formats, count + 1);
+                formats[count++] = format_name;
+            }
+        }
+    }
+
     qsort(formats, count, sizeof(formats[0]), qsort_strcmp);
 
     for (i = 0; i < count; i++) {
-- 
2.10.0

Re: [Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On 10/12/2016 03:49 PM, Max Reitz wrote:
> Some block drivers may not be loaded yet, but qemu supports them
> nonetheless. bdrv_iterate_format() should report them, too.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Hao QingFeng]

在 2016-10-13 4:49, Max Reitz 写道:
> Some block drivers may not be loaded yet, but qemu supports them
> nonetheless. bdrv_iterate_format() should report them, too.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/block.c b/block.c
> index e46e4b2..88a1ea5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2815,6 +2815,24 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>           }
>       }
>
> +    for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); i++) {
> +        const char *format_name = block_driver_modules[i].format_name;
> +
> +        if (format_name) {
> +            bool found = false;
> +            int j = count;
> +
> +            while (formats && j && !found) {
> +                found = !strcmp(formats[--j], format_name);
> +            }
> +
> +            if (!found) {
> +                formats = g_renew(const char *, formats, count + 1);
> +                formats[count++] = format_name;
> +            }
> +        }
> +    }
> +
Sorry for a bit late response. The function looks fine but just some 
doubt on g_renew
in this piece of code(and the legacy), does g_renew(realloc) has much 
performance
impact if it's call many times since alloc and memory copy are both also 
run many times?
So how about increase the memory for more instead of 1 each time?

formats = g_renew(const char *, formats, count + 10);

A new counter also needs to be introduced to record the space size.
Thanks

>       qsort(formats, count, sizeof(formats[0]), qsort_strcmp);
>
>       for (i = 0; i < count; i++) {

-- 
QingFeng Hao(Robin)

Re: [Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Kevin Wolf]

Am 02.11.2016 um 11:52 hat Hao QingFeng geschrieben:
> Sorry for a bit late response. The function looks fine but just some
> doubt on g_renew in this piece of code(and the legacy), does
> g_renew(realloc) has much performance impact if it's call many times
> since alloc and memory copy are both also run many times?  So how
> about increase the memory for more instead of 1 each time?
> 
> formats = g_renew(const char *, formats, count + 10);
> 
> A new counter also needs to be introduced to record the space size.

This code is not on a hot path, so keeping the code simple and easy to
maintain is preferrable to complicating it just so --help can save a few
CPU cycles.

Kevin

Re: [Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Hao QingFeng]

在 2016-11-02 19:15, Kevin Wolf 写道:
> Am 02.11.2016 um 11:52 hat Hao QingFeng geschrieben:
>> Sorry for a bit late response. The function looks fine but just some
>> doubt on g_renew in this piece of code(and the legacy), does
>> g_renew(realloc) has much performance impact if it's call many times
>> since alloc and memory copy are both also run many times?  So how
>> about increase the memory for more instead of 1 each time?
>>
>> formats = g_renew(const char *, formats, count + 10);
>>
>> A new counter also needs to be introduced to record the space size.
> This code is not on a hot path, so keeping the code simple and easy to
> maintain is preferrable to complicating it just so --help can save a few
> CPU cycles.
Got it. thanks!
> Kevin
>

-- 
QingFeng Hao(Robin)

Re: [Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]

On 11/02/2016 06:15 AM, Kevin Wolf wrote:
> Am 02.11.2016 um 11:52 hat Hao QingFeng geschrieben:
>> Sorry for a bit late response. The function looks fine but just some
>> doubt on g_renew in this piece of code(and the legacy), does
>> g_renew(realloc) has much performance impact if it's call many times
>> since alloc and memory copy are both also run many times?  So how
>> about increase the memory for more instead of 1 each time?
>>
>> formats = g_renew(const char *, formats, count + 10);
>>
>> A new counter also needs to be introduced to record the space size.

You are correct that the naive code has O(n^2) complexity, but so does
your replacement.  The only way to get to O(n) amortized complexity is
to change count by a geometric scale (the simplest is doubling each time
you have to realloc, but even something like increasing by 50% any time
an increase is needed would also work).  So if it is ever worth tracking
allocation size to reduce reallocation costs, it is worth doing it right
by using geometric rather than linear growth.

> 
> This code is not on a hot path, so keeping the code simple and easy to
> maintain is preferrable to complicating it just so --help can save a few
> CPU cycles.

But Kevin makes a good point - for small values of n, the difference
between running a complex O(n) algorithm or a simpler naive O(n^2)
algorithm can actually be faster for the naive algorithm; complexity
alone is not necessarily a good measure of performance until you have
large enough n that it becomes the dominating factor.  And this
certainly counts as code that is not executed frequently, nor where we
expect to have so many distinct formats that n will ever grow large
enough that the O(n^2) nature of the algorithm is noticeable to the
humans reading the help output.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/3] iotests: Skip test 162 if there is no SSH support

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/162 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
index f8eecb3..cad2bd7 100755
--- a/tests/qemu-iotests/162
+++ b/tests/qemu-iotests/162
@@ -35,6 +35,9 @@ status=1	# failure is the default!
 _supported_fmt generic
 _supported_os Linux
 
+test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
+[ "$test_ssh" = "" ] && _notrun "ssh support required"
+
 echo
 echo '=== NBD ==='
 # NBD expects all of its arguments to be strings
-- 
2.10.0

Re: [Qemu-devel] [PATCH 3/3] iotests: Skip test 162 if there is no SSH support

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

On 10/12/2016 03:49 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/162 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
> index f8eecb3..cad2bd7 100755
> --- a/tests/qemu-iotests/162
> +++ b/tests/qemu-iotests/162
> @@ -35,6 +35,9 @@ status=1	# failure is the default!
>  _supported_fmt generic
>  _supported_os Linux
>  
> +test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
> +[ "$test_ssh" = "" ] && _notrun "ssh support required"
> +

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

>  echo
>  echo '=== NBD ==='
>  # NBD expects all of its arguments to be strings
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/3] iotests: Skip 162 if there is no SSH support

[no-reply]

Hi,

Your series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Message-id: 20161012204907.25941-1-mreitz@redhat.com
Subject: [Qemu-devel] [PATCH 0/3] iotests: Skip 162 if there is no SSH support
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=16
make docker-test-quick@centos6
make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20161012204907.25941-1-mreitz@redhat.com -> patchew/20161012204907.25941-1-mreitz@redhat.com
Switched to a new branch 'test'
edffc08 iotests: Skip test 162 if there is no SSH support
de2a49f block: Emit modules in bdrv_iterate_format()
63e6b44 block: Fix bdrv_iterate_format() sorting

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
  BUILD   centos6
=== OUTPUT END ===

Abort: command timeout (>3600 seconds)


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/3] iotests: Skip 162 if there is no SSH support

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

On 12.10.2016 22:49, Max Reitz wrote:
> As reported by Hao QingFeng, iotest 162 is currently executed even if
> qemu does not have any SSH support (which makes it fail, naturally).
> 
> Fixing that is not so trivial, because qemu-img currently does not
> report modules, and SSH can be compiled as a module, so that needs to be
> fixed first. While doing so, I noticed that bdrv_iterate_format() tries
> to sort the list of formats, which is a bit contrary to my experience.
> Turns out that needs to be fixed, too.
> 
> 
> This series can either be applied on top of my series "iotests: Fix test
> 162" or just directly on master, both works (i.e. the patches in this
> series do not interfere with those from that one). I still thought I'd
> mention that series, if nothing else then only to get you to review that
> other one. ;-)
> 
> 
> Max Reitz (3):
>   block: Fix bdrv_iterate_format() sorting
>   block: Emit modules in bdrv_iterate_format()
>   iotests: Skip test 162 if there is no SSH support
> 
>  block.c                | 20 +++++++++++++++++++-
>  tests/qemu-iotests/162 |  3 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)

Applied to my block tree.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]