Re: [Qemu-devel] [PATCH 05/11] qerror: Clean up QERR_ macros to expand into a single string

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/13/2015 08:20 AM, Markus Armbruster wrote:
>> These macros expand into error class enumeration constant, comma,
>> string.  Unclean.  Has been that way since commit 13f59ae.
>> 
>> The error class is always ERROR_CLASS_GENERIC_ERROR since the previous
>> commit.
>> 
>> Clean up as follows:
>> 
>> * Prepend every use of a QERR_ macro by ERROR_CLASS_GENERIC_ERROR, and
>>   delete it from the QERR_ macro.  No change after preprocessing.
>> 
>> * Rewrite error_set(ERROR_CLASS_GENERIC_ERROR, ...) into
>>   error_setg(...).  Again, no change after preprocessing.
>
> Were these conversions done via coccinelle? If so, it would be nice to
> mention the script used in the commit message.  But that doesn't affect
> the correctness of the patch itself.

The semantic patch I used is almost 200 repetitive lines, because I
derived it from qerror.h with regexp-magic.  I doubt including it is
useful.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>>  util/qemu-option.c               | 22 ++++++++------
>>  53 files changed, 362 insertions(+), 358 deletions(-)
>
> Close to 1:1 lineup; difference is due to altered line wraps in a few spots.
>
>> 
>> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
>> index 849bd7a..225221e 100644
>> --- a/backends/rng-egd.c
>> +++ b/backends/rng-egd.c
>> @@ -140,8 +140,8 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
>>      RngEgd *s = RNG_EGD(b);
>>  
>>      if (s->chr_name == NULL) {
>> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> -                  "chardev", "a valid character device");
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "chardev",
>> +                   "a valid character device");
>
> Interesting line rewrap.

Coccinelle.

>> +++ b/backends/rng-random.c
>> @@ -74,8 +74,8 @@ static void rng_random_opened(RngBackend *b, Error **errp)
>>      RndRandom *s = RNG_RANDOM(b);
>>  
>>      if (s->filename == NULL) {
>> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> -                  "filename", "a valid filename");
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "filename",
>> +                   "a valid filename");
>
> and again
>
>
>> +++ b/block/quorum.c
>> @@ -800,8 +800,8 @@ static int quorum_valid_threshold(int threshold, int 
>> num_children, Error **errp)
>>  {
>>  
>>      if (threshold < 1) {
>> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> -                  "vote-threshold", "value >= 1");
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "vote-threshold",
>> +                   "value >= 1");
>
> looks like there are several of them. They don't hurt, so I'll quit
> pointing them out.

I'll try to avoid uncalled-for rewraps in v2.

>>  #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>> - ERROR_CLASS_GENERIC_ERROR, "Property %s.%s doesn't take value %"
>> PRId64 " (minimum: %" PRId64 ", maximum: %" PRId64 ")"
>> + "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64
>> ", maximum: %" PRId64 ")"
>
> Still a long line; worth wrapping with another \ mid-string?

Normally yes, but these macros should die, hopefully soon.

>> +++ b/tpm.c
>> @@ -140,20 +140,22 @@ static int configure_tpm(QemuOpts *opts)
>>  
>>      id = qemu_opts_id(opts);
>>      if (id == NULL) {
>> -        qerror_report(QERR_MISSING_PARAMETER, "id");
>> +        qerror_report(ERROR_CLASS_GENERIC_ERROR, QERR_MISSING_PARAMETER, 
>> "id");
>>          return 1;
>
> Not quite the same s/error_set/error_setg/ change as everywhere else,
> but still correct.
>
> There may be additional merge conflicts depending on timing of getting
> this in, but it is mechanical resolution, and the sooner we get this in,
> the better.

Yup.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!