target/s390x/cpu_models.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
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 <cfontana@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());
}
S390CPUModel *get_max_cpu_model(Error **errp)
--
2.26.2
Am 14.03.24 um 20:00 schrieb Claudio Fontana: > 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 <cfontana@suse.de> Independent on which message we end up with (see comments in this mail thread), I agree that improving the error message is helpful. Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com> > --- > 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()); > } > > S390CPUModel *get_max_cpu_model(Error **errp)
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 <cfontana@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. 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. 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. > > S390CPUModel *get_max_cpu_model(Error **errp)
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 <cfontana@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. >> >> S390CPUModel *get_max_cpu_model(Error **errp) > Thanks, Claudio
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 <cfontana@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
© 2016 - 2024 Red Hat, Inc.