Re: [Qemu-devel] [PATCH] image-fuzzer: Trivial readability and formatting improvements

[M.Kustova]

On Tue, Aug 19, 2014 at 1:44 PM, Fam Zheng <address@hidden> wrote:
> On Tue, 08/19 02:00, Maria Kustova wrote:
>> Signed-off-by: Maria Kustova <address@hidden>
>> ---
>>  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
>>