On 3/14/24 21:10, Claudio Fontana wrote:
> On 3/14/24 20:44, Nina Schoetterl-Glausch wrote:
>> On Thu, 2024-03-14 at 20:00 +0100, Claudio Fontana wrote:
>>> some users were confused by this message showing under TCG:
>>>
>>> Selected CPU generation is too new. Maximum supported model
>>> in the configuration: 'xyz'
>>>
>>> Try to clarify that the maximum can depend on the accel by
>>> adding also the current accelerator to the message as such:
>>>
>>> Selected CPU generation is too new. Maximum supported model
>>> in the accelerator 'tcg' configuration: 'xyz'
>>>
>>> Signed-off-by: Claudio Fontana <cfont...@suse.de>
>>> ---
>>> target/s390x/cpu_models.c | 11 ++++++-----
>>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index 1a1c096122..0d6d8fc727 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -508,14 +508,14 @@ static void check_compatibility(const S390CPUModel
>>> *max_model,
>>>
>>> if (model->def->gen > max_model->def->gen) {
>>> error_setg(errp, "Selected CPU generation is too new. Maximum "
>>> - "supported model in the configuration: \'%s\'",
>>> - max_model->def->name);
>>> + "supported model in the accelerator \'%s\'
>>> configuration: \'%s\'",
>>> + current_accel_name(), max_model->def->name);
>>> return;
>>> } else if (model->def->gen == max_model->def->gen &&
>>> model->def->ec_ga > max_model->def->ec_ga) {
>>> error_setg(errp, "Selected CPU GA level is too new. Maximum "
>>> - "supported model in the configuration: \'%s\'",
>>> - max_model->def->name);
>>> + "supported model in the accelerator \'%s\'
>>> configuration: \'%s\'",
>>> + current_accel_name(), max_model->def->name);
>>> return;
>>> }
>>>
>>> @@ -537,7 +537,8 @@ static void check_compatibility(const S390CPUModel
>>> *max_model,
>>> error_setg(errp, " ");
>>> s390_feat_bitmap_to_ascii(missing, errp, error_prepend_missing_feat);
>>> error_prepend(errp, "Some features requested in the CPU model are not "
>>> - "available in the configuration: ");
>>> + "available in the accelerator \'%s\' configuration: ",
>>> + current_accel_name());
>>> }
>>
>> I wonder if these might not be confusing in other circumstances, e.g. when
>> running with KVM and the Linux version lacks support for some feature.
>
> Here you are referencing specifically the last hunk right? Ie the "Some
> features requested..." message.
>
>> I think something along the lines of:
>>
>> error_...(errp, "... supported by the current configuration ...", ...);
>> error_append_hint(errp, "Consider using a different accelerator, a different
>> QEMU version or, when using KVM, a different kernel");
>>
>> would be better.
>
> Interesting I'll try something along these lines.
>
>>
>> I'm not sure about line breaks in error message, I like the better
>> grepability
>> of unbroken lines but the coding style guide doesn't mention anything.
>
> better greppability in the log (as the error message in the log), or in the
> source code (or both)?
> I am generally in favor of both, but there might be constraints on line
> length, although scripts/checkpatch.pl did not complain when I attempted this
> (I wonder if bug or feature).
>
> docs/devel/style.rst on the code line length topic says:
> "Lines should be 80 characters; try not to make them longer..."
>
> and it does talk about exceptions. In the case of error message strings I
> think this could be one one of those exceptions.
>
> In terms of logs, I did not find anything either, the most pertinent section
> should be "Error handling and reporting" in the same file,
> but there is nothing about breaking up [or not] a single message in errors
> with newlines.
Ah I forgot the mythical include/qapi/error.h:
for error_setg we have:
"The resulting message should be a single phrase, with no newline or trailing
punctuation."
so this helps.
>
>>>
>>> S390CPUModel *get_max_cpu_model(Error **errp)
>>
>
> Thanks,
>
> Claudio
