Re: [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message

[Markus Armbruster <armbru@redhat.com>]

Fam Zheng <famz@redhat.com> writes:

> On Thu, 12/17 17:49, Markus Armbruster wrote:
>> vmdk_parse_extents() reports parse errors like this:
>> 
>>     error_setg(errp, "Invalid extent lines:\n%s", p);
>> 
>> where p points to the beginning of the malformed line in the image
>> descriptor.  This results in a multi-line error message
>> 
>>     Invalid extent lines:
>>     <first line that doesn't parse>
>>     <remaining text that may or may not parse, if any>
>> 
>> Error messages should not have newlines embedded.  Since the remaining
>> text is not helpful, we can simply report:
>> 
>>     Invalid extent line: <first line that doesn't parse>
>> 
>> Cc: Fam Zheng <famz@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/vmdk.c               | 19 ++++++++++++-------
>>  tests/qemu-iotests/059.out |  4 +---
>>  2 files changed, 13 insertions(+), 10 deletions(-)
>> 
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 08fa3f3..04b47e3 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -780,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>      char access[11];
>>      char type[11];
>>      char fname[512];
>> -    const char *p;
>> +    const char *p, *np;
>>      int64_t sectors = 0;
>>      int64_t flat_offset;
>>      char *extent_path;
>> @@ -805,19 +805,16 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>              continue;
>>          } else if (!strcmp(type, "FLAT")) {
>>              if (matches != 5 || flat_offset < 0) {
>> -                error_setg(errp, "Invalid extent lines: \n%s", p);
>> -                return -EINVAL;
>> +                goto invalid;
>>              }
>>          } else if (!strcmp(type, "VMFS")) {
>>              if (matches == 4) {
>>                  flat_offset = 0;
>>              } else {
>> -                error_setg(errp, "Invalid extent lines:\n%s", p);
>> -                return -EINVAL;
>> +                goto invalid;
>>              }
>>          } else if (matches != 4) {
>> -            error_setg(errp, "Invalid extent lines:\n%s", p);
>> -            return -EINVAL;
>> +            goto invalid;
>>          }
>>  
>>          if (sectors <= 0 ||
>> @@ -883,6 +880,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>          extent->type = g_strdup(type);
>>      }
>>      return 0;
>> +
>> +invalid:
>> +    np = next_line(p);
>> +    assert(np != p);
>> +    if (np[-1] == '\n')
>> +        np--;
>
> Braces are required.

Fixing...

>> +    error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
>> +    return -EINVAL;
>>  }
>>  
>>  static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
>> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
>> index d28df5b..9d506cb 100644
>> --- a/tests/qemu-iotests/059.out
>> +++ b/tests/qemu-iotests/059.out
>> @@ -2038,9 +2038,7 @@ Format specific information:
>>              format: FLAT
>>  
>>  === Testing malformed VMFS extent description line ===
>> -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent lines:
>> -RW 12582912 VMFS "dummy.IMGFMT" 1
>> -
>> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912 VMFS "dummy.IMGFMT" 1
>>  
>>  === Testing truncated sparse ===
>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 subformat=monolithicSparse
>> -- 
>> 2.4.3
>>