All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
@ 2014-08-18 22:00 Maria Kustova
  2014-08-19  9:38 ` Stefan Hajnoczi
  2014-08-19  9:44 ` Fam Zheng
  0 siblings, 2 replies; 8+ messages in thread
From: Maria Kustova @ 2014-08-18 22:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, Maria Kustova, stefanha

Signed-off-by: Maria Kustova <maria.k@catit.be>
---
 tests/image-fuzzer/qcow2/fuzz.py | 15 ++++++------
 tests/image-fuzzer/runner.py     | 51 ++++++++++++++++++++--------------------
 2 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
index 6e272c6..c652dc9 100644
--- a/tests/image-fuzzer/qcow2/fuzz.py
+++ b/tests/image-fuzzer/qcow2/fuzz.py
@@ -123,7 +123,7 @@ def string_validator(current, strings):
     return validator(current, random.choice, strings)
 
 
-def selector(current, constraints, validate=int_validator):
+def selector(current, constraints, validate=None):
     """Select one value from all defined by constraints.
 
     Each constraint produces one random value satisfying to it. The function
@@ -131,6 +131,9 @@ def selector(current, constraints, validate=int_validator):
     constraints overlaps).
     """
 
+    if validate is None:
+        validate = int_validator
+
     def iter_validate(c):
         """Apply validate() only to constraints represented as lists.
 
@@ -337,9 +340,8 @@ def l1_entry(current):
     constraints = UINT64_V
     # Reserved bits are ignored
     # Added a possibility when only flags are fuzzed
-    offset = 0x7fffffffffffffff & random.choice([selector(current,
-                                                          constraints),
-                                                 current])
+    offset = 0x7fffffffffffffff & \
+             random.choice([selector(current, constraints), current])
     is_cow = random.randint(0, 1)
     return offset + (is_cow << UINT64_M)
 
@@ -349,9 +351,8 @@ def l2_entry(current):
     constraints = UINT64_V
     # Reserved bits are ignored
     # Add a possibility when only flags are fuzzed
-    offset = 0x3ffffffffffffffe & random.choice([selector(current,
-                                                          constraints),
-                                                 current])
+    offset = 0x3ffffffffffffffe & \
+             random.choice([selector(current, constraints), current])
     is_compressed = random.randint(0, 1)
     is_cow = random.randint(0, 1)
     is_zero = random.randint(0, 1)
diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index fd97c40..b142577 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -73,7 +73,7 @@ def run_app(fd, q_args):
         """Exception for signal.alarm events."""
         pass
 
-    def handler(*arg):
+    def handler(*args):
         """Notify that an alarm event occurred."""
         raise Alarm
 
@@ -137,8 +137,8 @@ class TestEnv(object):
         self.init_path = os.getcwd()
         self.work_dir = work_dir
         self.current_dir = os.path.join(work_dir, 'test-' + test_id)
-        self.qemu_img = os.environ.get('QEMU_IMG', 'qemu-img')\
-                                  .strip().split(' ')
+        self.qemu_img = \
+            os.environ.get('QEMU_IMG', 'qemu-img').strip().split(' ')
         self.qemu_io = os.environ.get('QEMU_IO', 'qemu-io').strip().split(' ')
         strings = ['%s%p%x%d', '.1024d', '%.2049d', '%p%p%p%p', '%x%x%x%x',
                    '%d%d%d%d', '%s%s%s%s', '%99999999999s', '%08x', '%%20d',
@@ -247,10 +247,8 @@ class TestEnv(object):
 
         os.chdir(self.current_dir)
         backing_file_name, backing_file_fmt = self._create_backing_file()
-        img_size = image_generator.create_image('test.img',
-                                                backing_file_name,
-                                                backing_file_fmt,
-                                                fuzz_config)
+        img_size = image_generator.create_image(
+            'test.img', backing_file_name, backing_file_fmt, fuzz_config)
         for item in commands:
             shutil.copy('test.img', 'copy.img')
             # 'off' and 'len' are multiple of the sector size
@@ -263,7 +261,7 @@ class TestEnv(object):
             elif item[0] == 'qemu-io':
                 current_cmd = list(self.qemu_io)
             else:
-                multilog("Warning: test command '%s' is not defined.\n" \
+                multilog("Warning: test command '%s' is not defined.\n"
                          % item[0], sys.stderr, self.log, self.parent_log)
                 continue
             # Replace all placeholders with their real values
@@ -279,29 +277,30 @@ class TestEnv(object):
                            "Backing file: %s\n" \
                            % (self.seed, " ".join(current_cmd),
                               self.current_dir, backing_file_name)
-
             temp_log = StringIO.StringIO()
             try:
                 retcode = run_app(temp_log, current_cmd)
             except OSError, e:
-                multilog(test_summary + "Error: Start of '%s' failed. " \
-                         "Reason: %s\n\n" % (os.path.basename(
-                             current_cmd[0]), e[1]),
+                multilog(test_summary +
+                         ("Error: Start of '%s' failed. Reason: %s\n\n"
+                          % (os.path.basename(current_cmd[0]), e[1])),
                          sys.stderr, self.log, self.parent_log)
                 raise TestException
 
             if retcode < 0:
                 self.log.write(temp_log.getvalue())
-                multilog(test_summary + "FAIL: Test terminated by signal " +
-                         "%s\n\n" % str_signal(-retcode), sys.stderr, self.log,
-                         self.parent_log)
+                multilog(test_summary +
+                         ("FAIL: Test terminated by signal %s\n\n"
+                          % str_signal(-retcode)),
+                         sys.stderr, self.log, self.parent_log)
                 self.failed = True
             else:
                 if self.log_all:
                     self.log.write(temp_log.getvalue())
-                    multilog(test_summary + "PASS: Application exited with" +
-                             " the code '%d'\n\n" % retcode, sys.stdout,
-                             self.log, self.parent_log)
+                    multilog(test_summary +
+                             ("PASS: Application exited with the code '%d'\n\n"
+                              % retcode),
+                             sys.stdout, self.log, self.parent_log)
             temp_log.close()
             os.remove('copy.img')
 
@@ -321,8 +320,9 @@ if __name__ == '__main__':
 
         Set up test environment in TEST_DIR and run a test in it. A module for
         test image generation should be specified via IMG_GENERATOR.
+
         Example:
-        runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test ../qcow2
+          runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test ../qcow2
 
         Optional arguments:
           -h, --help                    display this help and exit
@@ -340,20 +340,22 @@ if __name__ == '__main__':
 
         '--command' accepts a JSON array of commands. Each command presents
         an application under test with all its paramaters as a list of strings,
-        e.g.
-          ["qemu-io", "$test_img", "-c", "write $off $len"]
+        e.g. ["qemu-io", "$test_img", "-c", "write $off $len"].
 
         Supported application aliases: 'qemu-img' and 'qemu-io'.
+
         Supported argument aliases: $test_img for the fuzzed image, $off
         for an offset, $len for length.
 
         Values for $off and $len will be generated based on the virtual disk
-        size of the fuzzed image
+        size of the fuzzed image.
+
         Paths to 'qemu-img' and 'qemu-io' are retrevied from 'QEMU_IMG' and
-        'QEMU_IO' environment variables
+        'QEMU_IO' environment variables.
 
         '--config' accepts a JSON array of fields to be fuzzed, e.g.
-          '[["header"], ["header", "version"]]'
+        '[["header"], ["header", "version"]]'.
+
         Each of the list elements can consist of a complex image element only
         as ["header"] or ["feature_name_table"] or an exact field as
         ["header", "version"]. In the first case random portion of the element
@@ -403,7 +405,6 @@ if __name__ == '__main__':
     seed = None
     config = None
     duration = None
-
     for opt, arg in opts:
         if opt in ('-h', '--help'):
             usage()
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-18 22:00 [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements Maria Kustova
@ 2014-08-19  9:38 ` Stefan Hajnoczi
  2014-08-19  9:50   ` M.Kustova
  2014-08-19  9:44 ` Fam Zheng
  1 sibling, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-08-19  9:38 UTC (permalink / raw)
  To: Maria Kustova; +Cc: kwolf, famz, qemu-devel, Maria Kustova

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

On Tue, Aug 19, 2014 at 02:00:24AM +0400, Maria Kustova wrote:
> diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
> index 6e272c6..c652dc9 100644
> --- a/tests/image-fuzzer/qcow2/fuzz.py
> +++ b/tests/image-fuzzer/qcow2/fuzz.py
> @@ -123,7 +123,7 @@ def string_validator(current, strings):
>      return validator(current, random.choice, strings)
>  
>  
> -def selector(current, constraints, validate=int_validator):
> +def selector(current, constraints, validate=None):
>      """Select one value from all defined by constraints.
>  
>      Each constraint produces one random value satisfying to it. The function
> @@ -131,6 +131,9 @@ def selector(current, constraints, validate=int_validator):
>      constraints overlaps).
>      """
>  
> +    if validate is None:
> +        validate = int_validator
> +
>      def iter_validate(c):
>          """Apply validate() only to constraints represented as lists.
>  

I don't understand the benefit of this change.  The default initializer
syntax did what was intended, now it has been made more complicated with
a None value that gets converted to int_validator later.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-18 22:00 [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements Maria Kustova
  2014-08-19  9:38 ` Stefan Hajnoczi
@ 2014-08-19  9:44 ` Fam Zheng
  2014-08-19  9:55   ` M.Kustova
  1 sibling, 1 reply; 8+ messages in thread
From: Fam Zheng @ 2014-08-19  9:44 UTC (permalink / raw)
  To: Maria Kustova; +Cc: kwolf, qemu-devel, stefanha, Maria Kustova

On Tue, 08/19 02:00, Maria Kustova wrote:
> Signed-off-by: Maria Kustova <maria.k@catit.be>
> ---
>  tests/image-fuzzer/qcow2/fuzz.py | 15 ++++++------
>  tests/image-fuzzer/runner.py     | 51 ++++++++++++++++++++--------------------
>  2 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
> index 6e272c6..c652dc9 100644
> --- a/tests/image-fuzzer/qcow2/fuzz.py
> +++ b/tests/image-fuzzer/qcow2/fuzz.py
> @@ -123,7 +123,7 @@ def string_validator(current, strings):
>      return validator(current, random.choice, strings)
>  
>  
> -def selector(current, constraints, validate=int_validator):
> +def selector(current, constraints, validate=None):
>      """Select one value from all defined by constraints.
>  
>      Each constraint produces one random value satisfying to it. The function
> @@ -131,6 +131,9 @@ def selector(current, constraints, validate=int_validator):
>      constraints overlaps).
>      """
>  
> +    if validate is None:
> +        validate = int_validator
> +
>      def iter_validate(c):
>          """Apply validate() only to constraints represented as lists.
>  
> @@ -337,9 +340,8 @@ def l1_entry(current):
>      constraints = UINT64_V
>      # Reserved bits are ignored
>      # Added a possibility when only flags are fuzzed
> -    offset = 0x7fffffffffffffff & random.choice([selector(current,
> -                                                          constraints),
> -                                                 current])
> +    offset = 0x7fffffffffffffff & \
> +             random.choice([selector(current, constraints), current])
>      is_cow = random.randint(0, 1)
>      return offset + (is_cow << UINT64_M)
>  
> @@ -349,9 +351,8 @@ def l2_entry(current):
>      constraints = UINT64_V
>      # Reserved bits are ignored
>      # Add a possibility when only flags are fuzzed
> -    offset = 0x3ffffffffffffffe & random.choice([selector(current,
> -                                                          constraints),
> -                                                 current])
> +    offset = 0x3ffffffffffffffe & \
> +             random.choice([selector(current, constraints), current])
>      is_compressed = random.randint(0, 1)
>      is_cow = random.randint(0, 1)
>      is_zero = random.randint(0, 1)
> diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
> index fd97c40..b142577 100755
> --- a/tests/image-fuzzer/runner.py
> +++ b/tests/image-fuzzer/runner.py
> @@ -73,7 +73,7 @@ def run_app(fd, q_args):
>          """Exception for signal.alarm events."""
>          pass
>  
> -    def handler(*arg):
> +    def handler(*args):
>          """Notify that an alarm event occurred."""
>          raise Alarm
>  
> @@ -137,8 +137,8 @@ class TestEnv(object):
>          self.init_path = os.getcwd()
>          self.work_dir = work_dir
>          self.current_dir = os.path.join(work_dir, 'test-' + test_id)
> -        self.qemu_img = os.environ.get('QEMU_IMG', 'qemu-img')\
> -                                  .strip().split(' ')
> +        self.qemu_img = \
> +            os.environ.get('QEMU_IMG', 'qemu-img').strip().split(' ')
>          self.qemu_io = os.environ.get('QEMU_IO', 'qemu-io').strip().split(' ')
>          strings = ['%s%p%x%d', '.1024d', '%.2049d', '%p%p%p%p', '%x%x%x%x',
>                     '%d%d%d%d', '%s%s%s%s', '%99999999999s', '%08x', '%%20d',
> @@ -247,10 +247,8 @@ class TestEnv(object):
>  
>          os.chdir(self.current_dir)
>          backing_file_name, backing_file_fmt = self._create_backing_file()
> -        img_size = image_generator.create_image('test.img',
> -                                                backing_file_name,
> -                                                backing_file_fmt,
> -                                                fuzz_config)

Actually this one is fine, but the new way is good too.

> +        img_size = image_generator.create_image(
> +            'test.img', backing_file_name, backing_file_fmt, fuzz_config)
>          for item in commands:
>              shutil.copy('test.img', 'copy.img')
>              # 'off' and 'len' are multiple of the sector size
> @@ -263,7 +261,7 @@ class TestEnv(object):
>              elif item[0] == 'qemu-io':
>                  current_cmd = list(self.qemu_io)
>              else:
> -                multilog("Warning: test command '%s' is not defined.\n" \
> +                multilog("Warning: test command '%s' is not defined.\n"
>                           % item[0], sys.stderr, self.log, self.parent_log)
>                  continue
>              # Replace all placeholders with their real values
> @@ -279,29 +277,30 @@ class TestEnv(object):
>                             "Backing file: %s\n" \
>                             % (self.seed, " ".join(current_cmd),
>                                self.current_dir, backing_file_name)
> -
>              temp_log = StringIO.StringIO()
>              try:
>                  retcode = run_app(temp_log, current_cmd)
>              except OSError, e:
> -                multilog(test_summary + "Error: Start of '%s' failed. " \
> -                         "Reason: %s\n\n" % (os.path.basename(
> -                             current_cmd[0]), e[1]),
> +                multilog(test_summary +
> +                         ("Error: Start of '%s' failed. Reason: %s\n\n"
> +                          % (os.path.basename(current_cmd[0]), e[1])),

I prefer the old one. I don't like the special case in python syntax: '(val)'
is just val, but '(val1, val2)' is a tuple.

>                           sys.stderr, self.log, self.parent_log)
>                  raise TestException
>  
>              if retcode < 0:
>                  self.log.write(temp_log.getvalue())
> -                multilog(test_summary + "FAIL: Test terminated by signal " +
> -                         "%s\n\n" % str_signal(-retcode), sys.stderr, self.log,
> -                         self.parent_log)
> +                multilog(test_summary +
> +                         ("FAIL: Test terminated by signal %s\n\n"
> +                          % str_signal(-retcode)),
> +                         sys.stderr, self.log, self.parent_log)

The same here.

>                  self.failed = True
>              else:
>                  if self.log_all:
>                      self.log.write(temp_log.getvalue())
> -                    multilog(test_summary + "PASS: Application exited with" +
> -                             " the code '%d'\n\n" % retcode, sys.stdout,
> -                             self.log, self.parent_log)
> +                    multilog(test_summary +
> +                             ("PASS: Application exited with the code '%d'\n\n"
> +                              % retcode),
> +                             sys.stdout, self.log, self.parent_log)

And here.

Fam

>              temp_log.close()
>              os.remove('copy.img')
>  
> @@ -321,8 +320,9 @@ if __name__ == '__main__':
>  
>          Set up test environment in TEST_DIR and run a test in it. A module for
>          test image generation should be specified via IMG_GENERATOR.
> +
>          Example:
> -        runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test ../qcow2
> +          runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test ../qcow2
>  
>          Optional arguments:
>            -h, --help                    display this help and exit
> @@ -340,20 +340,22 @@ if __name__ == '__main__':
>  
>          '--command' accepts a JSON array of commands. Each command presents
>          an application under test with all its paramaters as a list of strings,
> -        e.g.
> -          ["qemu-io", "$test_img", "-c", "write $off $len"]
> +        e.g. ["qemu-io", "$test_img", "-c", "write $off $len"].
>  
>          Supported application aliases: 'qemu-img' and 'qemu-io'.
> +
>          Supported argument aliases: $test_img for the fuzzed image, $off
>          for an offset, $len for length.
>  
>          Values for $off and $len will be generated based on the virtual disk
> -        size of the fuzzed image
> +        size of the fuzzed image.
> +
>          Paths to 'qemu-img' and 'qemu-io' are retrevied from 'QEMU_IMG' and
> -        'QEMU_IO' environment variables
> +        'QEMU_IO' environment variables.
>  
>          '--config' accepts a JSON array of fields to be fuzzed, e.g.
> -          '[["header"], ["header", "version"]]'
> +        '[["header"], ["header", "version"]]'.
> +
>          Each of the list elements can consist of a complex image element only
>          as ["header"] or ["feature_name_table"] or an exact field as
>          ["header", "version"]. In the first case random portion of the element
> @@ -403,7 +405,6 @@ if __name__ == '__main__':
>      seed = None
>      config = None
>      duration = None
> -
>      for opt, arg in opts:
>          if opt in ('-h', '--help'):
>              usage()
> -- 
> 1.9.3
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-19  9:38 ` Stefan Hajnoczi
@ 2014-08-19  9:50   ` M.Kustova
  2014-08-22 10:13     ` Stefan Hajnoczi
  0 siblings, 1 reply; 8+ messages in thread
From: M.Kustova @ 2014-08-19  9:50 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Kevin Wolf, Fam Zheng, qemu-devel

On Tue, Aug 19, 2014 at 1:38 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Tue, Aug 19, 2014 at 02:00:24AM +0400, Maria Kustova wrote:
>> diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
>> index 6e272c6..c652dc9 100644
>> --- a/tests/image-fuzzer/qcow2/fuzz.py
>> +++ b/tests/image-fuzzer/qcow2/fuzz.py
>> @@ -123,7 +123,7 @@ def string_validator(current, strings):
>>      return validator(current, random.choice, strings)
>>
>>
>> -def selector(current, constraints, validate=int_validator):
>> +def selector(current, constraints, validate=None):
>>      """Select one value from all defined by constraints.
>>
>>      Each constraint produces one random value satisfying to it. The function
>> @@ -131,6 +131,9 @@ def selector(current, constraints, validate=int_validator):
>>      constraints overlaps).
>>      """
>>
>> +    if validate is None:
>> +        validate = int_validator
>> +
>>      def iter_validate(c):
>>          """Apply validate() only to constraints represented as lists.
>>
>
> I don't understand the benefit of this change.  The default initializer
> syntax did what was intended, now it has been made more complicated with
> a None value that gets converted to int_validator later.

It's a convention rule that allows keep the function signature
unchanged in case of modified function internals.
In other words, wrapper functions should not change in response of the
internal function change.

Maria.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-19  9:44 ` Fam Zheng
@ 2014-08-19  9:55   ` M.Kustova
  2014-08-19 10:57     ` Markus Armbruster
  0 siblings, 1 reply; 8+ messages in thread
From: M.Kustova @ 2014-08-19  9:55 UTC (permalink / raw)
  To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi

On Tue, Aug 19, 2014 at 1:44 PM, Fam Zheng <famz@redhat.com> wrote:
> On Tue, 08/19 02:00, Maria Kustova wrote:
>> Signed-off-by: Maria Kustova <maria.k@catit.be>
>> ---
>>  tests/image-fuzzer/qcow2/fuzz.py | 15 ++++++------
>>  tests/image-fuzzer/runner.py     | 51 ++++++++++++++++++++--------------------
>>  2 files changed, 34 insertions(+), 32 deletions(-)
>>
>> diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
>> index 6e272c6..c652dc9 100644
>> --- a/tests/image-fuzzer/qcow2/fuzz.py
>> +++ b/tests/image-fuzzer/qcow2/fuzz.py
>> @@ -123,7 +123,7 @@ def string_validator(current, strings):
>>      return validator(current, random.choice, strings)
>>
>>
>> -def selector(current, constraints, validate=int_validator):
>> +def selector(current, constraints, validate=None):
>>      """Select one value from all defined by constraints.
>>
>>      Each constraint produces one random value satisfying to it. The function
>> @@ -131,6 +131,9 @@ def selector(current, constraints, validate=int_validator):
>>      constraints overlaps).
>>      """
>>
>> +    if validate is None:
>> +        validate = int_validator
>> +
>>      def iter_validate(c):
>>          """Apply validate() only to constraints represented as lists.
>>
>> @@ -337,9 +340,8 @@ def l1_entry(current):
>>      constraints = UINT64_V
>>      # Reserved bits are ignored
>>      # Added a possibility when only flags are fuzzed
>> -    offset = 0x7fffffffffffffff & random.choice([selector(current,
>> -                                                          constraints),
>> -                                                 current])
>> +    offset = 0x7fffffffffffffff & \
>> +             random.choice([selector(current, constraints), current])
>>      is_cow = random.randint(0, 1)
>>      return offset + (is_cow << UINT64_M)
>>
>> @@ -349,9 +351,8 @@ def l2_entry(current):
>>      constraints = UINT64_V
>>      # Reserved bits are ignored
>>      # Add a possibility when only flags are fuzzed
>> -    offset = 0x3ffffffffffffffe & random.choice([selector(current,
>> -                                                          constraints),
>> -                                                 current])
>> +    offset = 0x3ffffffffffffffe & \
>> +             random.choice([selector(current, constraints), current])
>>      is_compressed = random.randint(0, 1)
>>      is_cow = random.randint(0, 1)
>>      is_zero = random.randint(0, 1)
>> diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
>> index fd97c40..b142577 100755
>> --- a/tests/image-fuzzer/runner.py
>> +++ b/tests/image-fuzzer/runner.py
>> @@ -73,7 +73,7 @@ def run_app(fd, q_args):
>>          """Exception for signal.alarm events."""
>>          pass
>>
>> -    def handler(*arg):
>> +    def handler(*args):
>>          """Notify that an alarm event occurred."""
>>          raise Alarm
>>
>> @@ -137,8 +137,8 @@ class TestEnv(object):
>>          self.init_path = os.getcwd()
>>          self.work_dir = work_dir
>>          self.current_dir = os.path.join(work_dir, 'test-' + test_id)
>> -        self.qemu_img = os.environ.get('QEMU_IMG', 'qemu-img')\
>> -                                  .strip().split(' ')
>> +        self.qemu_img = \
>> +            os.environ.get('QEMU_IMG', 'qemu-img').strip().split(' ')
>>          self.qemu_io = os.environ.get('QEMU_IO', 'qemu-io').strip().split(' ')
>>          strings = ['%s%p%x%d', '.1024d', '%.2049d', '%p%p%p%p', '%x%x%x%x',
>>                     '%d%d%d%d', '%s%s%s%s', '%99999999999s', '%08x', '%%20d',
>> @@ -247,10 +247,8 @@ class TestEnv(object):
>>
>>          os.chdir(self.current_dir)
>>          backing_file_name, backing_file_fmt = self._create_backing_file()
>> -        img_size = image_generator.create_image('test.img',
>> -                                                backing_file_name,
>> -                                                backing_file_fmt,
>> -                                                fuzz_config)
>
> Actually this one is fine, but the new way is good too.
>
>> +        img_size = image_generator.create_image(
>> +            'test.img', backing_file_name, backing_file_fmt, fuzz_config)
>>          for item in commands:
>>              shutil.copy('test.img', 'copy.img')
>>              # 'off' and 'len' are multiple of the sector size
>> @@ -263,7 +261,7 @@ class TestEnv(object):
>>              elif item[0] == 'qemu-io':
>>                  current_cmd = list(self.qemu_io)
>>              else:
>> -                multilog("Warning: test command '%s' is not defined.\n" \
>> +                multilog("Warning: test command '%s' is not defined.\n"
>>                           % item[0], sys.stderr, self.log, self.parent_log)
>>                  continue
>>              # Replace all placeholders with their real values
>> @@ -279,29 +277,30 @@ class TestEnv(object):
>>                             "Backing file: %s\n" \
>>                             % (self.seed, " ".join(current_cmd),
>>                                self.current_dir, backing_file_name)
>> -
>>              temp_log = StringIO.StringIO()
>>              try:
>>                  retcode = run_app(temp_log, current_cmd)
>>              except OSError, e:
>> -                multilog(test_summary + "Error: Start of '%s' failed. " \
>> -                         "Reason: %s\n\n" % (os.path.basename(
>> -                             current_cmd[0]), e[1]),
>> +                multilog(test_summary +
>> +                         ("Error: Start of '%s' failed. Reason: %s\n\n"
>> +                          % (os.path.basename(current_cmd[0]), e[1])),
>
> I prefer the old one. I don't like the special case in python syntax: '(val)'
> is just val, but '(val1, val2)' is a tuple.

This 'tuple' format provides that '%' substitution will be applied
only to the string in the parentheses,
instead of an entire string (in this case including test summary).
The same rationale for cases below.

>
>>                           sys.stderr, self.log, self.parent_log)
>>                  raise TestException
>>
>>              if retcode < 0:
>>                  self.log.write(temp_log.getvalue())
>> -                multilog(test_summary + "FAIL: Test terminated by signal " +
>> -                         "%s\n\n" % str_signal(-retcode), sys.stderr, self.log,
>> -                         self.parent_log)
>> +                multilog(test_summary +
>> +                         ("FAIL: Test terminated by signal %s\n\n"
>> +                          % str_signal(-retcode)),
>> +                         sys.stderr, self.log, self.parent_log)
>
> The same here.
>
>>                  self.failed = True
>>              else:
>>                  if self.log_all:
>>                      self.log.write(temp_log.getvalue())
>> -                    multilog(test_summary + "PASS: Application exited with" +
>> -                             " the code '%d'\n\n" % retcode, sys.stdout,
>> -                             self.log, self.parent_log)
>> +                    multilog(test_summary +
>> +                             ("PASS: Application exited with the code '%d'\n\n"
>> +                              % retcode),
>> +                             sys.stdout, self.log, self.parent_log)
>
> And here.
>
> Fam
>
>>              temp_log.close()
>>              os.remove('copy.img')
>>
>> @@ -321,8 +320,9 @@ if __name__ == '__main__':
>>
>>          Set up test environment in TEST_DIR and run a test in it. A module for
>>          test image generation should be specified via IMG_GENERATOR.
>> +
>>          Example:
>> -        runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test ../qcow2
>> +          runner.py -c '[["qemu-img", "info", "$test_img"]]' /tmp/test ../qcow2
>>
>>          Optional arguments:
>>            -h, --help                    display this help and exit
>> @@ -340,20 +340,22 @@ if __name__ == '__main__':
>>
>>          '--command' accepts a JSON array of commands. Each command presents
>>          an application under test with all its paramaters as a list of strings,
>> -        e.g.
>> -          ["qemu-io", "$test_img", "-c", "write $off $len"]
>> +        e.g. ["qemu-io", "$test_img", "-c", "write $off $len"].
>>
>>          Supported application aliases: 'qemu-img' and 'qemu-io'.
>> +
>>          Supported argument aliases: $test_img for the fuzzed image, $off
>>          for an offset, $len for length.
>>
>>          Values for $off and $len will be generated based on the virtual disk
>> -        size of the fuzzed image
>> +        size of the fuzzed image.
>> +
>>          Paths to 'qemu-img' and 'qemu-io' are retrevied from 'QEMU_IMG' and
>> -        'QEMU_IO' environment variables
>> +        'QEMU_IO' environment variables.
>>
>>          '--config' accepts a JSON array of fields to be fuzzed, e.g.
>> -          '[["header"], ["header", "version"]]'
>> +        '[["header"], ["header", "version"]]'.
>> +
>>          Each of the list elements can consist of a complex image element only
>>          as ["header"] or ["feature_name_table"] or an exact field as
>>          ["header", "version"]. In the first case random portion of the element
>> @@ -403,7 +405,6 @@ if __name__ == '__main__':
>>      seed = None
>>      config = None
>>      duration = None
>> -
>>      for opt, arg in opts:
>>          if opt in ('-h', '--help'):
>>              usage()
>> --
>> 1.9.3
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-19  9:55   ` M.Kustova
@ 2014-08-19 10:57     ` Markus Armbruster
  2014-08-19 16:04       ` M.Kustova
  0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2014-08-19 10:57 UTC (permalink / raw)
  To: M.Kustova; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi

"M.Kustova" <maxa@catit.be> writes:

> On Tue, Aug 19, 2014 at 1:44 PM, Fam Zheng <famz@redhat.com> wrote:
>> On Tue, 08/19 02:00, Maria Kustova wrote:
[...]
>>> diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
>>> index fd97c40..b142577 100755
>>> --- a/tests/image-fuzzer/runner.py
>>> +++ b/tests/image-fuzzer/runner.py
[...]
>>> @@ -279,29 +277,30 @@ class TestEnv(object):
>>>                             "Backing file: %s\n" \
>>>                             % (self.seed, " ".join(current_cmd),
>>>                                self.current_dir, backing_file_name)
>>> -
>>>              temp_log = StringIO.StringIO()
>>>              try:
>>>                  retcode = run_app(temp_log, current_cmd)
>>>              except OSError, e:
>>> -                multilog(test_summary + "Error: Start of '%s' failed. " \
>>> -                         "Reason: %s\n\n" % (os.path.basename(
>>> -                             current_cmd[0]), e[1]),
>>> +                multilog(test_summary +
>>> +                         ("Error: Start of '%s' failed. Reason: %s\n\n"
>>> +                          % (os.path.basename(current_cmd[0]), e[1])),
>>
>> I prefer the old one. I don't like the special case in python syntax: '(val)'
>> is just val, but '(val1, val2)' is a tuple.
>
> This 'tuple' format provides that '%' substitution will be applied
> only to the string in the parentheses,
> instead of an entire string (in this case including test summary).
> The same rationale for cases below.

What about something like

                multilog("%sError: Start of '%s' failed. Reason: %s\n\n"
                         % (test_summary, os.path.basename(current_cmd[0]),
                            e[1]),
                         sys.stderr, self.log, self.parent_log)

[...]

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-19 10:57     ` Markus Armbruster
@ 2014-08-19 16:04       ` M.Kustova
  0 siblings, 0 replies; 8+ messages in thread
From: M.Kustova @ 2014-08-19 16:04 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, Stefan Hajnoczi

On Tue, Aug 19, 2014 at 2:57 PM, Markus Armbruster <armbru@redhat.com> wrote:
> "M.Kustova" <maxa@catit.be> writes:
>
>> On Tue, Aug 19, 2014 at 1:44 PM, Fam Zheng <famz@redhat.com> wrote:
>>> On Tue, 08/19 02:00, Maria Kustova wrote:
> [...]
>>>> diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
>>>> index fd97c40..b142577 100755
>>>> --- a/tests/image-fuzzer/runner.py
>>>> +++ b/tests/image-fuzzer/runner.py
> [...]
>>>> @@ -279,29 +277,30 @@ class TestEnv(object):
>>>>                             "Backing file: %s\n" \
>>>>                             % (self.seed, " ".join(current_cmd),
>>>>                                self.current_dir, backing_file_name)
>>>> -
>>>>              temp_log = StringIO.StringIO()
>>>>              try:
>>>>                  retcode = run_app(temp_log, current_cmd)
>>>>              except OSError, e:
>>>> -                multilog(test_summary + "Error: Start of '%s' failed. " \
>>>> -                         "Reason: %s\n\n" % (os.path.basename(
>>>> -                             current_cmd[0]), e[1]),
>>>> +                multilog(test_summary +
>>>> +                         ("Error: Start of '%s' failed. Reason: %s\n\n"
>>>> +                          % (os.path.basename(current_cmd[0]), e[1])),
>>>
>>> I prefer the old one. I don't like the special case in python syntax: '(val)'
>>> is just val, but '(val1, val2)' is a tuple.
>>
>> This 'tuple' format provides that '%' substitution will be applied
>> only to the string in the parentheses,
>> instead of an entire string (in this case including test summary).
>> The same rationale for cases below.
>
> What about something like
>
>                 multilog("%sError: Start of '%s' failed. Reason: %s\n\n"
>                          % (test_summary, os.path.basename(current_cmd[0]),
>                             e[1]),
>                          sys.stderr, self.log, self.parent_log)
>
>
 Accepted. Thank you.

Maria.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements
  2014-08-19  9:50   ` M.Kustova
@ 2014-08-22 10:13     ` Stefan Hajnoczi
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-08-22 10:13 UTC (permalink / raw)
  To: M.Kustova; +Cc: Kevin Wolf, Fam Zheng, qemu-devel

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

On Tue, Aug 19, 2014 at 01:50:27PM +0400, M.Kustova wrote:
> On Tue, Aug 19, 2014 at 1:38 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Tue, Aug 19, 2014 at 02:00:24AM +0400, Maria Kustova wrote:
> >> diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
> >> index 6e272c6..c652dc9 100644
> >> --- a/tests/image-fuzzer/qcow2/fuzz.py
> >> +++ b/tests/image-fuzzer/qcow2/fuzz.py
> >> @@ -123,7 +123,7 @@ def string_validator(current, strings):
> >>      return validator(current, random.choice, strings)
> >>
> >>
> >> -def selector(current, constraints, validate=int_validator):
> >> +def selector(current, constraints, validate=None):
> >>      """Select one value from all defined by constraints.
> >>
> >>      Each constraint produces one random value satisfying to it. The function
> >> @@ -131,6 +131,9 @@ def selector(current, constraints, validate=int_validator):
> >>      constraints overlaps).
> >>      """
> >>
> >> +    if validate is None:
> >> +        validate = int_validator
> >> +
> >>      def iter_validate(c):
> >>          """Apply validate() only to constraints represented as lists.
> >>
> >
> > I don't understand the benefit of this change.  The default initializer
> > syntax did what was intended, now it has been made more complicated with
> > a None value that gets converted to int_validator later.
> 
> It's a convention rule that allows keep the function signature
> unchanged in case of modified function internals.
> In other words, wrapper functions should not change in response of the
> internal function change.

I don't think that is necessary.  This is not a stable API that cannot
change.

Also, what exactly creates an API commitment here?  Hiding int_validator
doesn't hide anything.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-08-22 10:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-18 22:00 [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements Maria Kustova
2014-08-19  9:38 ` Stefan Hajnoczi
2014-08-19  9:50   ` M.Kustova
2014-08-22 10:13     ` Stefan Hajnoczi
2014-08-19  9:44 ` Fam Zheng
2014-08-19  9:55   ` M.Kustova
2014-08-19 10:57     ` Markus Armbruster
2014-08-19 16:04       ` M.Kustova

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.