Re: [PULL 27/29] vl: deprecate -writeconfig

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/03/21 14:26, Markus Armbruster wrote:
>>> Didn't really forget; being pretty sure that there's no usage in the
>>> wild and having good reasons to remove the code, giving a firm removal
>>> date should encourage people to speak up sooner rather than later.
>> Second thoughts after agreeing to change something are okay.  Keeping
>> them for yourself not so much, because it deprives your reviewers of a
>> chance to raise further points.
>
> Sorry about that.
>
>> In this case, the point I didn't make because I wanted to reach
>> agreement on contents before nitpicking form: you're not using
>> warn_report() the way it wants to be used:
>> 
>>      /*
>>       * Print a warning message to current monitor if we have one, else to 
>> stderr.
>>       * Format arguments like sprintf(). The resulting message should be a
>> ---> * single phrase, with no newline or trailing punctuation.
>>       * Prepend the current location and append a newline.
>>       */
>>      void warn_report(const char *fmt, ...)
>
> I knew about the rules for no newline or trailing punctuation, but I 
> didn't remember the other.  I can certainly respin, that said:
>
> - the comment should say "sentence", not "phrase".  For example "a 
> single phrase" is a single phrase, while "the resulting message should 
> be a single phrase" is a single sentence.

I avoided "sentence", because good error messages aren't always
grammatically complete sentences.  My use of "phrase" may well be wrong.
I tried!  Patches welcome :)

Dicking around on the web, I just found

    https://www.postgres-xl.org/documentation/error-style-guide.html

Drop the Postgres-specific parts, and what's left is pretty close to my
thoughts on error message style.

> - I'm not sure how to interpret the rule above.  First of all, the 
> sentence mixes part that are mandatory part ("no newline", checked by 
> checkpatch.pl) is mixed with the style guide ("no trailing punctuation" 
> and "a single sentence").  Second, whether a single sentence is better 
> often depends on the case.  For example, comparing these four:
>
> WARNING: -writeconfig foo: -writeconfig is deprecated.  It will go away 
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated and will go away 
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated; it will go away 
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated
> WARNING: -writeconfig foo: it will go away in QEMU 6.2 with no replacement
>
> The first one is what was in this patch.
>
> The second does sound fine to me and it's probably what I'll use in v2, 
> with or without the "in QEMU 6.2" part.  However some could consider it 
> to be worse style due to the longer sentence.
>
> The third one is playing with the rules; a semicolon would be separating 
> two sentences.  However usage of the semicolon is quite common in error 
> messages, so maybe it would be good too?

Semicolons can be okay, as long the resulting message is still short.

Still short:

    warning: -writeconfig foo: -writeconfig is deprecated without replacement

    warning: -writeconfig foo: option is deprecated; there is no replacement

No longer short:

    warning: -writeconfig foo: -writeconfig is deprecated; it will go away in 
QEMU 6.2 with no replacement

There is no need to squeeze all the information into the "primary" error
message!  That one should state what's wrong *concisely*.  If you feel
you should explain further, or would like to advise on what could be
done to fix the problem, a separate "hint" message often reads better
than overloading the primary message with information.

When reporting to the user, use error_report() / warn_report() for the
"primary", and error_printf() for the "hint".

When setting an Error, use error_setg() for the "primary", and
error_append_hint() for the "hint".  error_report_err() will then use
error_report() and error_printf() correctly.

> The last one also complies, but it is not clear what "it" refers to so 
> it seems to be the worst in this case.  In other cases it's the obvious 
> choice, and we even have an API for it (error_append_hint... however it 
> doesn't play well with error_fatal which I'm otherwise a big fan of). 
> In this case, not so much.

Use of error_append_hint() requires ERRP_GUARD().  Without ERRP_GUARD(),
the hint indeed gets lost when errp == &error_fatal.

Properly guarded, we could have something like

    warning: -writeconfig foo: option -writeconfig is deprecated
    This option will go away with no replacement.

I'm glad you like &error_fatal, too!  I have had to defend it a few
times :)