diff mbox series

[RFC,2/2] qemu-img: consider a zero number of I/O requests an invalid count

Message ID 20181109221213.7310-3-crosa@redhat.com
State New
Headers show
Series Acceptance tests for qemu-img | expand

Commit Message

Cleber Rosa Nov. 9, 2018, 10:12 p.m. UTC
It's debatable whether it makes sense to consider the bench command
successful when no I/O requests will be performed.  This changes the
behavior to consider a zero count of I/O requests an invalid value.
While at it, avoid using signed types for number of I/O requests.

The image file used, is a simple raw image with 1K size.  There's a
obvious trade off between creating and reusing those images.  This is
an experiment in sharing those among tests.  It was created using:

 mkdir -p tests/data/images/empty
 cd tests/data/images/empty
 qemu-img create raw 1K

This relies on the Test's "get_data()", which by default looks for
data files on sources that map to various levels of specificity of a
test: file, test and additionally with variant and a symlink.

One other possibility with regards to in-tree images, is to extend the
Test's "get_data()" API (which is extensible by design) and make the
common images directory a "source".  The resulting API usage would be
similar to:

   self.get_data("empty/raw", source="common_images")

or simply:

   self.get_data("empty/raw")

To look for "empty/raw" in any of the available sources.  That would
make the symlink unnecessary.

Reference: https://avocado-framework.readthedocs.io/en/65.0/api/core/avocado.core.html#avocado.core.test.TestData
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 qemu-img.c                                  |   8 ++---
 tests/acceptance/qemu_img_bench.py          |  34 ++++++++++++++++++++
 tests/acceptance/qemu_img_bench.py.data/img |   1 +
 tests/data/images/empty/raw                 | Bin 0 -> 1024 bytes
 4 files changed, 39 insertions(+), 4 deletions(-)
 create mode 100644 tests/acceptance/qemu_img_bench.py
 create mode 120000 tests/acceptance/qemu_img_bench.py.data/img
 create mode 100644 tests/data/images/empty/raw

diff --git a/tests/data/images/empty/raw b/tests/data/images/empty/raw
new file mode 100644
index 0000000000000000000000000000000000000000..06d7405020018ddf3cacee90fd4af10487da3d20
GIT binary patch
literal 1024
ScmZQz7zLvtFd70QH3R?z00031

literal 0
HcmV?d00001

Comments

Philippe Mathieu-Daudé Nov. 12, 2018, 2:38 p.m. UTC | #1
On 9/11/18 23:12, Cleber Rosa wrote:
> It's debatable whether it makes sense to consider the bench command
> successful when no I/O requests will be performed.  This changes the
> behavior to consider a zero count of I/O requests an invalid value.
> While at it, avoid using signed types for number of I/O requests.
> 
> The image file used, is a simple raw image with 1K size.  There's a
> obvious trade off between creating and reusing those images.  This is
> an experiment in sharing those among tests.  It was created using:
> 
>   mkdir -p tests/data/images/empty
>   cd tests/data/images/empty
>   qemu-img create raw 1K
> 
> This relies on the Test's "get_data()", which by default looks for
> data files on sources that map to various levels of specificity of a
> test: file, test and additionally with variant and a symlink.
> 
> One other possibility with regards to in-tree images, is to extend the
> Test's "get_data()" API (which is extensible by design) and make the
> common images directory a "source".  The resulting API usage would be
> similar to:
> 
>     self.get_data("empty/raw", source="common_images")
> 
> or simply:
> 
>     self.get_data("empty/raw")
> 
> To look for "empty/raw" in any of the available sources.  That would
> make the symlink unnecessary.
> 
> Reference: https://avocado-framework.readthedocs.io/en/65.0/api/core/avocado.core.html#avocado.core.test.TestData
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   qemu-img.c                                  |   8 ++---
>   tests/acceptance/qemu_img_bench.py          |  34 ++++++++++++++++++++
>   tests/acceptance/qemu_img_bench.py.data/img |   1 +
>   tests/data/images/empty/raw                 | Bin 0 -> 1024 bytes
>   4 files changed, 39 insertions(+), 4 deletions(-)
>   create mode 100644 tests/acceptance/qemu_img_bench.py
>   create mode 120000 tests/acceptance/qemu_img_bench.py.data/img
>   create mode 100644 tests/data/images/empty/raw
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4c96db7ba4..7ffcdd1589 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3960,7 +3960,7 @@ typedef struct BenchData {
>       int bufsize;
>       int step;
>       int nrreq;
> -    int n;
> +    unsigned int n;
>       int flush_interval;
>       bool drain_on_flush;
>       uint8_t *buf;
> @@ -4051,7 +4051,7 @@ static int img_bench(int argc, char **argv)
>       bool quiet = false;
>       bool image_opts = false;
>       bool is_write = false;
> -    int count = 75000;
> +    unsigned int count = 75000;
>       int depth = 64;
>       int64_t offset = 0;
>       size_t bufsize = 4096;
> @@ -4098,7 +4098,7 @@ static int img_bench(int argc, char **argv)
>           {
>               unsigned long res;
>   
> -            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
> +            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > UINT_MAX || res <= 0) {

res is unsigned so can't be < 0.

>                   error_report("Invalid request count specified");
>                   return 1;
>               }
> @@ -4248,7 +4248,7 @@ static int img_bench(int argc, char **argv)
>           .flush_interval = flush_interval,
>           .drain_on_flush = drain_on_flush,
>       };
> -    printf("Sending %d %s requests, %d bytes each, %d in parallel "
> +    printf("Sending %u %s requests, %d bytes each, %d in parallel "
>              "(starting at offset %" PRId64 ", step size %d)\n",
>              data.n, data.write ? "write" : "read", data.bufsize, data.nrreq,
>              data.offset, data.step);
> diff --git a/tests/acceptance/qemu_img_bench.py b/tests/acceptance/qemu_img_bench.py
> new file mode 100644
> index 0000000000..327524ad8f
> --- /dev/null
> +++ b/tests/acceptance/qemu_img_bench.py
> @@ -0,0 +1,34 @@
> +# Test for the basic qemu-img bench command
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +#  Cleber Rosa <crosa@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import os
> +
> +from avocado_qemu import QemuImgTest
> +from avocado.utils import process
> +
> +
> +class Bench(QemuImgTest):
> +    """
> +    Runs the qemu-img tool with the bench command and different
> +    options and verifies the expected outcome.
> +
> +    :avocado: enable
> +    """
> +    def check_invalid_count(self, count):
> +        cmd = "%s bench -c %d %s" % (self.qemu_img_bin, count, self.get_data("img"))
> +        result = process.run(cmd, ignore_status=True)
> +        self.assertEqual(1, result.exit_status)
> +        self.assertIn(b"Invalid request count", result.stderr)
> +
> +    def test_zero_count(self):
> +        self.check_invalid_count(0)
> +
> +    def test_negative_count(self):
> +        self.check_invalid_count(-1)
> diff --git a/tests/acceptance/qemu_img_bench.py.data/img b/tests/acceptance/qemu_img_bench.py.data/img
> new file mode 120000
> index 0000000000..6d01ef2e85
> --- /dev/null
> +++ b/tests/acceptance/qemu_img_bench.py.data/img
> @@ -0,0 +1 @@
> +../../data/images/empty/raw
> \ No newline at end of file
> diff --git a/tests/data/images/empty/raw b/tests/data/images/empty/raw
> new file mode 100644
> index 0000000000000000000000000000000000000000..06d7405020018ddf3cacee90fd4af10487da3d20
> GIT binary patch
> literal 1024
> ScmZQz7zLvtFd70QH3R?z00031
> 
> literal 0
> HcmV?d00001
>
Cleber Rosa Nov. 12, 2018, 3:04 p.m. UTC | #2
On 11/12/18 9:38 AM, Philippe Mathieu-Daudé wrote:
> On 9/11/18 23:12, Cleber Rosa wrote:
>> It's debatable whether it makes sense to consider the bench command
>> successful when no I/O requests will be performed.  This changes the
>> behavior to consider a zero count of I/O requests an invalid value.
>> While at it, avoid using signed types for number of I/O requests.
>>
>> The image file used, is a simple raw image with 1K size.  There's a
>> obvious trade off between creating and reusing those images.  This is
>> an experiment in sharing those among tests.  It was created using:
>>
>>   mkdir -p tests/data/images/empty
>>   cd tests/data/images/empty
>>   qemu-img create raw 1K
>>
>> This relies on the Test's "get_data()", which by default looks for
>> data files on sources that map to various levels of specificity of a
>> test: file, test and additionally with variant and a symlink.
>>
>> One other possibility with regards to in-tree images, is to extend the
>> Test's "get_data()" API (which is extensible by design) and make the
>> common images directory a "source".  The resulting API usage would be
>> similar to:
>>
>>     self.get_data("empty/raw", source="common_images")
>>
>> or simply:
>>
>>     self.get_data("empty/raw")
>>
>> To look for "empty/raw" in any of the available sources.  That would
>> make the symlink unnecessary.
>>
>> Reference:
>> https://avocado-framework.readthedocs.io/en/65.0/api/core/avocado.core.html#avocado.core.test.TestData
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   qemu-img.c                                  |   8 ++---
>>   tests/acceptance/qemu_img_bench.py          |  34 ++++++++++++++++++++
>>   tests/acceptance/qemu_img_bench.py.data/img |   1 +
>>   tests/data/images/empty/raw                 | Bin 0 -> 1024 bytes
>>   4 files changed, 39 insertions(+), 4 deletions(-)
>>   create mode 100644 tests/acceptance/qemu_img_bench.py
>>   create mode 120000 tests/acceptance/qemu_img_bench.py.data/img
>>   create mode 100644 tests/data/images/empty/raw
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 4c96db7ba4..7ffcdd1589 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -3960,7 +3960,7 @@ typedef struct BenchData {
>>       int bufsize;
>>       int step;
>>       int nrreq;
>> -    int n;
>> +    unsigned int n;
>>       int flush_interval;
>>       bool drain_on_flush;
>>       uint8_t *buf;
>> @@ -4051,7 +4051,7 @@ static int img_bench(int argc, char **argv)
>>       bool quiet = false;
>>       bool image_opts = false;
>>       bool is_write = false;
>> -    int count = 75000;
>> +    unsigned int count = 75000;
>>       int depth = 64;
>>       int64_t offset = 0;
>>       size_t bufsize = 4096;
>> @@ -4098,7 +4098,7 @@ static int img_bench(int argc, char **argv)
>>           {
>>               unsigned long res;
>>   -            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res >
>> INT_MAX) {
>> +            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res >
>> UINT_MAX || res <= 0) {
> 
> res is unsigned so can't be < 0.
> 

Right, that should have been obvious to me.

Thanks!
- Cleber.

>>                   error_report("Invalid request count specified");
>>                   return 1;
>>               }
>> @@ -4248,7 +4248,7 @@ static int img_bench(int argc, char **argv)
>>           .flush_interval = flush_interval,
>>           .drain_on_flush = drain_on_flush,
>>       };
>> -    printf("Sending %d %s requests, %d bytes each, %d in parallel "
>> +    printf("Sending %u %s requests, %d bytes each, %d in parallel "
>>              "(starting at offset %" PRId64 ", step size %d)\n",
>>              data.n, data.write ? "write" : "read", data.bufsize,
>> data.nrreq,
>>              data.offset, data.step);
>> diff --git a/tests/acceptance/qemu_img_bench.py
>> b/tests/acceptance/qemu_img_bench.py
>> new file mode 100644
>> index 0000000000..327524ad8f
>> --- /dev/null
>> +++ b/tests/acceptance/qemu_img_bench.py
>> @@ -0,0 +1,34 @@
>> +# Test for the basic qemu-img bench command
>> +#
>> +# Copyright (c) 2018 Red Hat, Inc.
>> +#
>> +# Author:
>> +#  Cleber Rosa <crosa@redhat.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +import os
>> +
>> +from avocado_qemu import QemuImgTest
>> +from avocado.utils import process
>> +
>> +
>> +class Bench(QemuImgTest):
>> +    """
>> +    Runs the qemu-img tool with the bench command and different
>> +    options and verifies the expected outcome.
>> +
>> +    :avocado: enable
>> +    """
>> +    def check_invalid_count(self, count):
>> +        cmd = "%s bench -c %d %s" % (self.qemu_img_bin, count,
>> self.get_data("img"))
>> +        result = process.run(cmd, ignore_status=True)
>> +        self.assertEqual(1, result.exit_status)
>> +        self.assertIn(b"Invalid request count", result.stderr)
>> +
>> +    def test_zero_count(self):
>> +        self.check_invalid_count(0)
>> +
>> +    def test_negative_count(self):
>> +        self.check_invalid_count(-1)
>> diff --git a/tests/acceptance/qemu_img_bench.py.data/img
>> b/tests/acceptance/qemu_img_bench.py.data/img
>> new file mode 120000
>> index 0000000000..6d01ef2e85
>> --- /dev/null
>> +++ b/tests/acceptance/qemu_img_bench.py.data/img
>> @@ -0,0 +1 @@
>> +../../data/images/empty/raw
>> \ No newline at end of file
>> diff --git a/tests/data/images/empty/raw b/tests/data/images/empty/raw
>> new file mode 100644
>> index
>> 0000000000000000000000000000000000000000..06d7405020018ddf3cacee90fd4af10487da3d20
>>
>> GIT binary patch
>> literal 1024
>> ScmZQz7zLvtFd70QH3R?z00031
>>
>> literal 0
>> HcmV?d00001
>>
diff mbox series

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 4c96db7ba4..7ffcdd1589 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3960,7 +3960,7 @@  typedef struct BenchData {
     int bufsize;
     int step;
     int nrreq;
-    int n;
+    unsigned int n;
     int flush_interval;
     bool drain_on_flush;
     uint8_t *buf;
@@ -4051,7 +4051,7 @@  static int img_bench(int argc, char **argv)
     bool quiet = false;
     bool image_opts = false;
     bool is_write = false;
-    int count = 75000;
+    unsigned int count = 75000;
     int depth = 64;
     int64_t offset = 0;
     size_t bufsize = 4096;
@@ -4098,7 +4098,7 @@  static int img_bench(int argc, char **argv)
         {
             unsigned long res;
 
-            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
+            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > UINT_MAX || res <= 0) {
                 error_report("Invalid request count specified");
                 return 1;
             }
@@ -4248,7 +4248,7 @@  static int img_bench(int argc, char **argv)
         .flush_interval = flush_interval,
         .drain_on_flush = drain_on_flush,
     };
-    printf("Sending %d %s requests, %d bytes each, %d in parallel "
+    printf("Sending %u %s requests, %d bytes each, %d in parallel "
            "(starting at offset %" PRId64 ", step size %d)\n",
            data.n, data.write ? "write" : "read", data.bufsize, data.nrreq,
            data.offset, data.step);
diff --git a/tests/acceptance/qemu_img_bench.py b/tests/acceptance/qemu_img_bench.py
new file mode 100644
index 0000000000..327524ad8f
--- /dev/null
+++ b/tests/acceptance/qemu_img_bench.py
@@ -0,0 +1,34 @@ 
+# Test for the basic qemu-img bench command
+#
+# Copyright (c) 2018 Red Hat, Inc.
+#
+# Author:
+#  Cleber Rosa <crosa@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+import os
+
+from avocado_qemu import QemuImgTest
+from avocado.utils import process
+
+
+class Bench(QemuImgTest):
+    """
+    Runs the qemu-img tool with the bench command and different
+    options and verifies the expected outcome.
+
+    :avocado: enable
+    """
+    def check_invalid_count(self, count):
+        cmd = "%s bench -c %d %s" % (self.qemu_img_bin, count, self.get_data("img"))
+        result = process.run(cmd, ignore_status=True)
+        self.assertEqual(1, result.exit_status)
+        self.assertIn(b"Invalid request count", result.stderr)
+
+    def test_zero_count(self):
+        self.check_invalid_count(0)
+
+    def test_negative_count(self):
+        self.check_invalid_count(-1)
diff --git a/tests/acceptance/qemu_img_bench.py.data/img b/tests/acceptance/qemu_img_bench.py.data/img
new file mode 120000
index 0000000000..6d01ef2e85
--- /dev/null
+++ b/tests/acceptance/qemu_img_bench.py.data/img
@@ -0,0 +1 @@ 
+../../data/images/empty/raw
\ No newline at end of file