| Message ID | 20240809064940.1788169-1-anisinha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] kvm: replace fprintf with error_report/printf() in kvm_init() | expand |
Hi Ani, On 9/8/24 08:49, Ani Sinha wrote: > error_report() is more appropriate for error situations. Replace fprintf with > error_report. Cosmetic. No functional change. > > CC: qemu-trivial@nongnu.org > CC: zhao1.liu@intel.com (Pointless to carry Cc line when patch is already reviewed next line) > Reviewed-by: Zhao Liu <zhao1.liu@intel.com> > Signed-off-by: Ani Sinha <anisinha@redhat.com> > --- > accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 22 deletions(-) > > changelog: > v2: fix a bug. > v3: replace one instance of error_report() with error_printf(). added tags. > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 75d11a07b2..5bc9d35b61 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) > QLIST_INIT(&s->kvm_parked_vcpus); > s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); > if (s->fd == -1) { > - fprintf(stderr, "Could not access KVM kernel module: %m\n"); > + error_report("Could not access KVM kernel module: %m"); > ret = -errno; > goto err; > } > @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) > if (ret >= 0) { > ret = -EINVAL; > } > - fprintf(stderr, "kvm version too old\n"); > + error_report("kvm version too old"); > goto err; > } > > if (ret > KVM_API_VERSION) { > ret = -EINVAL; > - fprintf(stderr, "kvm version not supported\n"); > + error_report("kvm version not supported"); > goto err; > } > > @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) > } while (ret == -EINTR); > > if (ret < 0) { > - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, > - strerror(-ret)); > + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, > + strerror(-ret)); > > #ifdef TARGET_S390X > if (ret == -EINVAL) { > - fprintf(stderr, > - "Host kernel setup problem detected. Please verify:\n"); > - fprintf(stderr, "- for kernels supporting the switch_amode or" > - " user_mode parameters, whether\n"); > - fprintf(stderr, > - " user space is running in primary address space\n"); > - fprintf(stderr, > - "- for kernels supporting the vm.allocate_pgste sysctl, " > - "whether it is enabled\n"); > + error_report("Host kernel setup problem detected. \n" Should we use error_printf_unless_qmp() for the following? " Please verify:"); > + error_report("- for kernels supporting the switch_amode or" > + " user_mode parameters, whether"); > + error_report(" user space is running in primary address space"); > + error_report("- for kernels supporting the vm.allocate_pgste " > + "sysctl, whether it is enabled"); > } > #elif defined(TARGET_PPC) > if (ret == -EINVAL) { > - fprintf(stderr, > - "PPC KVM module is not loaded. \n" Ditto. " Try modprobe kvm_%s.\n", > - (type == 2) ? "pr" : "hv"); > + error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.", > + (type == 2) ? "pr" : "hv"); > } > #endif > goto err; > @@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms) > nc->name, nc->num, soft_vcpus_limit); > > if (nc->num > hard_vcpus_limit) { > - fprintf(stderr, "Number of %s cpus requested (%d) exceeds " > - "the maximum cpus supported by KVM (%d)\n", > - nc->name, nc->num, hard_vcpus_limit); > + error_report("Number of %s cpus requested (%d) exceeds " > + "the maximum cpus supported by KVM (%d)", > + nc->name, nc->num, hard_vcpus_limit); > exit(1); > } > } > @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms) > } > if (missing_cap) { > ret = -EINVAL; > - fprintf(stderr, "kvm does not support %s\n%s", > - missing_cap->name, upgrade_note); > + error_printf("kvm does not support %s\n%s", > + missing_cap->name, upgrade_note); Similarly, should we print upgrade_note using error_printf_unless_qmp? > goto err; > } >
On Fri, Aug 9, 2024 at 2:06 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Hi Ani, > > On 9/8/24 08:49, Ani Sinha wrote: > > error_report() is more appropriate for error situations. Replace fprintf with > > error_report. Cosmetic. No functional change. > > > > CC: qemu-trivial@nongnu.org > > CC: zhao1.liu@intel.com > > (Pointless to carry Cc line when patch is already reviewed next line) > > > Reviewed-by: Zhao Liu <zhao1.liu@intel.com> > > Signed-off-by: Ani Sinha <anisinha@redhat.com> > > --- > > accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- > > 1 file changed, 18 insertions(+), 22 deletions(-) > > > > changelog: > > v2: fix a bug. > > v3: replace one instance of error_report() with error_printf(). added tags. > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index 75d11a07b2..5bc9d35b61 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) > > QLIST_INIT(&s->kvm_parked_vcpus); > > s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); > > if (s->fd == -1) { > > - fprintf(stderr, "Could not access KVM kernel module: %m\n"); > > + error_report("Could not access KVM kernel module: %m"); > > ret = -errno; > > goto err; > > } > > @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) > > if (ret >= 0) { > > ret = -EINVAL; > > } > > - fprintf(stderr, "kvm version too old\n"); > > + error_report("kvm version too old"); > > goto err; > > } > > > > if (ret > KVM_API_VERSION) { > > ret = -EINVAL; > > - fprintf(stderr, "kvm version not supported\n"); > > + error_report("kvm version not supported"); > > goto err; > > } > > > > @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) > > } while (ret == -EINTR); > > > > if (ret < 0) { > > - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, > > - strerror(-ret)); > > + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, > > + strerror(-ret)); > > > > #ifdef TARGET_S390X > > if (ret == -EINVAL) { > > - fprintf(stderr, > > - "Host kernel setup problem detected. Please verify:\n"); > > - fprintf(stderr, "- for kernels supporting the switch_amode or" > > - " user_mode parameters, whether\n"); > > - fprintf(stderr, > > - " user space is running in primary address space\n"); > > - fprintf(stderr, > > - "- for kernels supporting the vm.allocate_pgste sysctl, " > > - "whether it is enabled\n"); > > + error_report("Host kernel setup problem detected. > > \n" > > Should we use error_printf_unless_qmp() for the following? Do you believe that qemu_init() -> configure_accelerators() -> do_configure_accelerator,() -> accel_init_machine() -> kvm_init() can be called from QMP context? > > " Please verify:"); > > + error_report("- for kernels supporting the switch_amode or" > > + " user_mode parameters, whether"); > > + error_report(" user space is running in primary address space"); > > + error_report("- for kernels supporting the vm.allocate_pgste " > > + "sysctl, whether it is enabled"); > > } > > #elif defined(TARGET_PPC) > > if (ret == -EINVAL) { > > - fprintf(stderr, > > - "PPC KVM module is not loaded. > > \n" > > Ditto. > > " Try modprobe kvm_%s.\n", > > - (type == 2) ? "pr" : "hv"); > > + error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.", > > + (type == 2) ? "pr" : "hv"); > > } > > #endif > > goto err; > > @@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms) > > nc->name, nc->num, soft_vcpus_limit); > > > > if (nc->num > hard_vcpus_limit) { > > - fprintf(stderr, "Number of %s cpus requested (%d) exceeds " > > - "the maximum cpus supported by KVM (%d)\n", > > - nc->name, nc->num, hard_vcpus_limit); > > + error_report("Number of %s cpus requested (%d) exceeds " > > + "the maximum cpus supported by KVM (%d)", > > + nc->name, nc->num, hard_vcpus_limit); > > exit(1); > > } > > } > > @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms) > > } > > if (missing_cap) { > > ret = -EINVAL; > > - fprintf(stderr, "kvm does not support %s\n%s", > > - missing_cap->name, upgrade_note); > > + error_printf("kvm does not support %s\n%s", > > + missing_cap->name, upgrade_note); > > Similarly, should we print upgrade_note using error_printf_unless_qmp? > > > goto err; > > } > > >
On Mon, 12 Aug, 2024, 3:23 pm Ani Sinha, <anisinha@redhat.com> wrote: > On Fri, Aug 9, 2024 at 2:06 PM Philippe Mathieu-Daudé <philmd@linaro.org> > wrote: > > > > Hi Ani, > > > > On 9/8/24 08:49, Ani Sinha wrote: > > > error_report() is more appropriate for error situations. Replace > fprintf with > > > error_report. Cosmetic. No functional change. > > > > > > CC: qemu-trivial@nongnu.org > > > CC: zhao1.liu@intel.com > > > > (Pointless to carry Cc line when patch is already reviewed next line) > > > > > Reviewed-by: Zhao Liu <zhao1.liu@intel.com> > > > Signed-off-by: Ani Sinha <anisinha@redhat.com> > > > --- > > > accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- > > > 1 file changed, 18 insertions(+), 22 deletions(-) > > > > > > changelog: > > > v2: fix a bug. > > > v3: replace one instance of error_report() with error_printf(). added > tags. > > > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > > index 75d11a07b2..5bc9d35b61 100644 > > > --- a/accel/kvm/kvm-all.c > > > +++ b/accel/kvm/kvm-all.c > > > @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) > > > QLIST_INIT(&s->kvm_parked_vcpus); > > > s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); > > > if (s->fd == -1) { > > > - fprintf(stderr, "Could not access KVM kernel module: %m\n"); > > > + error_report("Could not access KVM kernel module: %m"); > > > ret = -errno; > > > goto err; > > > } > > > @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) > > > if (ret >= 0) { > > > ret = -EINVAL; > > > } > > > - fprintf(stderr, "kvm version too old\n"); > > > + error_report("kvm version too old"); > > > goto err; > > > } > > > > > > if (ret > KVM_API_VERSION) { > > > ret = -EINVAL; > > > - fprintf(stderr, "kvm version not supported\n"); > > > + error_report("kvm version not supported"); > > > goto err; > > > } > > > > > > @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) > > > } while (ret == -EINTR); > > > > > > if (ret < 0) { > > > - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, > > > - strerror(-ret)); > > > + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, > > > + strerror(-ret)); > > > > > > #ifdef TARGET_S390X > > > if (ret == -EINVAL) { > > > - fprintf(stderr, > > > - "Host kernel setup problem detected. Please > verify:\n"); > > > - fprintf(stderr, "- for kernels supporting the > switch_amode or" > > > - " user_mode parameters, whether\n"); > > > - fprintf(stderr, > > > - " user space is running in primary address > space\n"); > > > - fprintf(stderr, > > > - "- for kernels supporting the vm.allocate_pgste > sysctl, " > > > - "whether it is enabled\n"); > > > + error_report("Host kernel setup problem detected. > > > > \n" > > > > Should we use error_printf_unless_qmp() for the following? > > Do you believe that qemu_init() -> configure_accelerators() -> > do_configure_accelerator,() -> accel_init_machine() -> kvm_init() can > be called from QMP context? > To clarify, that is the only path I saw that calls kvm_init() > > > > " Please verify:"); > > > + error_report("- for kernels supporting the switch_amode > or" > > > + " user_mode parameters, whether"); > > > + error_report(" user space is running in primary address > space"); > > > + error_report("- for kernels supporting the > vm.allocate_pgste " > > > + "sysctl, whether it is enabled"); > > > } > > > #elif defined(TARGET_PPC) > > > if (ret == -EINVAL) { > > > - fprintf(stderr, > > > - "PPC KVM module is not loaded. > > > > \n" > > > > Ditto. > > > > " Try modprobe kvm_%s.\n", > > > - (type == 2) ? "pr" : "hv"); > > > + error_report("PPC KVM module is not loaded. Try modprobe > kvm_%s.", > > > + (type == 2) ? "pr" : "hv"); > > > } > > > #endif > > > goto err; > > > @@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms) > > > nc->name, nc->num, soft_vcpus_limit); > > > > > > if (nc->num > hard_vcpus_limit) { > > > - fprintf(stderr, "Number of %s cpus requested (%d) > exceeds " > > > - "the maximum cpus supported by KVM (%d)\n", > > > - nc->name, nc->num, hard_vcpus_limit); > > > + error_report("Number of %s cpus requested (%d) > exceeds " > > > + "the maximum cpus supported by KVM (%d)", > > > + nc->name, nc->num, hard_vcpus_limit); > > > exit(1); > > > } > > > } > > > @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms) > > > } > > > if (missing_cap) { > > > ret = -EINVAL; > > > - fprintf(stderr, "kvm does not support %s\n%s", > > > - missing_cap->name, upgrade_note); > > > + error_printf("kvm does not support %s\n%s", > > > + missing_cap->name, upgrade_note); > > > > Similarly, should we print upgrade_note using error_printf_unless_qmp? > > > > > goto err; > > > } > > > > > >
On 12/8/24 11:59, Ani Sinha wrote: > > > On Mon, 12 Aug, 2024, 3:23 pm Ani Sinha, <anisinha@redhat.com > <mailto:anisinha@redhat.com>> wrote: > > On Fri, Aug 9, 2024 at 2:06 PM Philippe Mathieu-Daudé > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > > > Hi Ani, > > > > On 9/8/24 08:49, Ani Sinha wrote: > > > error_report() is more appropriate for error situations. > Replace fprintf with > > > error_report. Cosmetic. No functional change. > > > > > > CC: qemu-trivial@nongnu.org <mailto:qemu-trivial@nongnu.org> > > > CC: zhao1.liu@intel.com <mailto:zhao1.liu@intel.com> > > > > (Pointless to carry Cc line when patch is already reviewed next line) > > > > > Reviewed-by: Zhao Liu <zhao1.liu@intel.com > <mailto:zhao1.liu@intel.com>> > > > Signed-off-by: Ani Sinha <anisinha@redhat.com > <mailto:anisinha@redhat.com>> > > > --- > > > accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- > > > 1 file changed, 18 insertions(+), 22 deletions(-) > > > > > > changelog: > > > v2: fix a bug. > > > v3: replace one instance of error_report() with error_printf(). > added tags. > > > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > > index 75d11a07b2..5bc9d35b61 100644 > > > --- a/accel/kvm/kvm-all.c > > > +++ b/accel/kvm/kvm-all.c > > > @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) > > > QLIST_INIT(&s->kvm_parked_vcpus); > > > s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); > > > if (s->fd == -1) { > > > - fprintf(stderr, "Could not access KVM kernel module: > %m\n"); > > > + error_report("Could not access KVM kernel module: %m"); > > > ret = -errno; > > > goto err; > > > } > > > @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) > > > if (ret >= 0) { > > > ret = -EINVAL; > > > } > > > - fprintf(stderr, "kvm version too old\n"); > > > + error_report("kvm version too old"); > > > goto err; > > > } > > > > > > if (ret > KVM_API_VERSION) { > > > ret = -EINVAL; > > > - fprintf(stderr, "kvm version not supported\n"); > > > + error_report("kvm version not supported"); > > > goto err; > > > } > > > > > > @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) > > > } while (ret == -EINTR); > > > > > > if (ret < 0) { > > > - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d > %s\n", -ret, > > > - strerror(-ret)); > > > + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, > > > + strerror(-ret)); > > > > > > #ifdef TARGET_S390X > > > if (ret == -EINVAL) { > > > - fprintf(stderr, > > > - "Host kernel setup problem detected. > Please verify:\n"); > > > - fprintf(stderr, "- for kernels supporting the > switch_amode or" > > > - " user_mode parameters, whether\n"); > > > - fprintf(stderr, > > > - " user space is running in primary > address space\n"); > > > - fprintf(stderr, > > > - "- for kernels supporting the > vm.allocate_pgste sysctl, " > > > - "whether it is enabled\n"); > > > + error_report("Host kernel setup problem detected. > > > > \n" > > > > Should we use error_printf_unless_qmp() for the following? > > Do you believe that qemu_init() -> configure_accelerators() -> > do_configure_accelerator,() -> accel_init_machine() -> kvm_init() can > be called from QMP context? > > > To clarify, that is the only path I saw that calls kvm_init() We don't know whether this code can end refactored or not. Personally I rather consistent API uses, since snipped of code are often used as example. Up to the maintainer. > > > > " Please verify:"); > > > + error_report("- for kernels supporting the > switch_amode or" > > > + " user_mode parameters, whether"); > > > + error_report(" user space is running in primary > address space"); > > > + error_report("- for kernels supporting the > vm.allocate_pgste " > > > + "sysctl, whether it is enabled"); > > > } > > > #elif defined(TARGET_PPC) > > > if (ret == -EINVAL) { > > > - fprintf(stderr, > > > - "PPC KVM module is not loaded. > > > > \n" > > > > Ditto. > > > > " Try modprobe kvm_%s.\n", > > > - (type == 2) ? "pr" : "hv"); > > > + error_report("PPC KVM module is not loaded. Try > modprobe kvm_%s.", > > > + (type == 2) ? "pr" : "hv"); > > > } > > > #endif > > > goto err; > > > @@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms) > > > nc->name, nc->num, soft_vcpus_limit); > > > > > > if (nc->num > hard_vcpus_limit) { > > > - fprintf(stderr, "Number of %s cpus requested > (%d) exceeds " > > > - "the maximum cpus supported by KVM > (%d)\n", > > > - nc->name, nc->num, hard_vcpus_limit); > > > + error_report("Number of %s cpus requested (%d) > exceeds " > > > + "the maximum cpus supported by > KVM (%d)", > > > + nc->name, nc->num, hard_vcpus_limit); > > > exit(1); > > > } > > > } > > > @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms) > > > } > > > if (missing_cap) { > > > ret = -EINVAL; > > > - fprintf(stderr, "kvm does not support %s\n%s", > > > - missing_cap->name, upgrade_note); > > > + error_printf("kvm does not support %s\n%s", > > > + missing_cap->name, upgrade_note); > > > > Similarly, should we print upgrade_note using > error_printf_unless_qmp? > > > > > goto err; > > > } > > > > > >
> On 16 Aug 2024, at 11:51 AM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 12/8/24 11:59, Ani Sinha wrote: >> On Mon, 12 Aug, 2024, 3:23 pm Ani Sinha, <anisinha@redhat.com <mailto:anisinha@redhat.com>> wrote: >> On Fri, Aug 9, 2024 at 2:06 PM Philippe Mathieu-Daudé >> <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: >> > >> > Hi Ani, >> > >> > On 9/8/24 08:49, Ani Sinha wrote: >> > > error_report() is more appropriate for error situations. >> Replace fprintf with >> > > error_report. Cosmetic. No functional change. >> > > >> > > CC: qemu-trivial@nongnu.org <mailto:qemu-trivial@nongnu.org> >> > > CC: zhao1.liu@intel.com <mailto:zhao1.liu@intel.com> >> > >> > (Pointless to carry Cc line when patch is already reviewed next line) >> > >> > > Reviewed-by: Zhao Liu <zhao1.liu@intel.com >> <mailto:zhao1.liu@intel.com>> >> > > Signed-off-by: Ani Sinha <anisinha@redhat.com >> <mailto:anisinha@redhat.com>> >> > > --- >> > > accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- >> > > 1 file changed, 18 insertions(+), 22 deletions(-) >> > > >> > > changelog: >> > > v2: fix a bug. >> > > v3: replace one instance of error_report() with error_printf(). >> added tags. >> > > >> > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> > > index 75d11a07b2..5bc9d35b61 100644 >> > > --- a/accel/kvm/kvm-all.c >> > > +++ b/accel/kvm/kvm-all.c >> > > @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) >> > > QLIST_INIT(&s->kvm_parked_vcpus); >> > > s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); >> > > if (s->fd == -1) { >> > > - fprintf(stderr, "Could not access KVM kernel module: >> %m\n"); >> > > + error_report("Could not access KVM kernel module: %m"); >> > > ret = -errno; >> > > goto err; >> > > } >> > > @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) >> > > if (ret >= 0) { >> > > ret = -EINVAL; >> > > } >> > > - fprintf(stderr, "kvm version too old\n"); >> > > + error_report("kvm version too old"); >> > > goto err; >> > > } >> > > >> > > if (ret > KVM_API_VERSION) { >> > > ret = -EINVAL; >> > > - fprintf(stderr, "kvm version not supported\n"); >> > > + error_report("kvm version not supported"); >> > > goto err; >> > > } >> > > >> > > @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) >> > > } while (ret == -EINTR); >> > > >> > > if (ret < 0) { >> > > - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d >> %s\n", -ret, >> > > - strerror(-ret)); >> > > + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, >> > > + strerror(-ret)); >> > > >> > > #ifdef TARGET_S390X >> > > if (ret == -EINVAL) { >> > > - fprintf(stderr, >> > > - "Host kernel setup problem detected. >> Please verify:\n"); >> > > - fprintf(stderr, "- for kernels supporting the >> switch_amode or" >> > > - " user_mode parameters, whether\n"); >> > > - fprintf(stderr, >> > > - " user space is running in primary >> address space\n"); >> > > - fprintf(stderr, >> > > - "- for kernels supporting the >> vm.allocate_pgste sysctl, " >> > > - "whether it is enabled\n"); >> > > + error_report("Host kernel setup problem detected. >> > >> > \n" >> > >> > Should we use error_printf_unless_qmp() for the following? >> Do you believe that qemu_init() -> configure_accelerators() -> >> do_configure_accelerator,() -> accel_init_machine() -> kvm_init() can >> be called from QMP context? >> To clarify, that is the only path I saw that calls kvm_init() > > We don't know whether this code can end refactored or not. Ok personally I think we can cross the bridge when we get there. > Personally I rather consistent API uses, since snipped of > code are often used as example. Up to the maintainer. OK up to Paolo then :-) > >> > >> > " Please verify:"); >> > > + error_report("- for kernels supporting the >> switch_amode or" >> > > + " user_mode parameters, whether"); >> > > + error_report(" user space is running in primary >> address space"); >> > > + error_report("- for kernels supporting the >> vm.allocate_pgste " >> > > + "sysctl, whether it is enabled"); >> > > } >> > > #elif defined(TARGET_PPC) >> > > if (ret == -EINVAL) { >> > > - fprintf(stderr, >> > > - "PPC KVM module is not loaded. >> > >> > \n" >> > >> > Ditto. >> > >> > " Try modprobe kvm_%s.\n", >> > > - (type == 2) ? "pr" : "hv"); >> > > + error_report("PPC KVM module is not loaded. Try >> modprobe kvm_%s.", >> > > + (type == 2) ? "pr" : "hv"); >> > > } >> > > #endif >> > > goto err; >> > > @@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms) >> > > nc->name, nc->num, soft_vcpus_limit); >> > > >> > > if (nc->num > hard_vcpus_limit) { >> > > - fprintf(stderr, "Number of %s cpus requested >> (%d) exceeds " >> > > - "the maximum cpus supported by KVM >> (%d)\n", >> > > - nc->name, nc->num, hard_vcpus_limit); >> > > + error_report("Number of %s cpus requested (%d) >> exceeds " >> > > + "the maximum cpus supported by >> KVM (%d)", >> > > + nc->name, nc->num, hard_vcpus_limit); >> > > exit(1); >> > > } >> > > } >> > > @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms) >> > > } >> > > if (missing_cap) { >> > > ret = -EINVAL; >> > > - fprintf(stderr, "kvm does not support %s\n%s", >> > > - missing_cap->name, upgrade_note); >> > > + error_printf("kvm does not support %s\n%s", >> > > + missing_cap->name, upgrade_note); >> > >> > Similarly, should we print upgrade_note using >> error_printf_unless_qmp? >> > >> > > goto err; >> > > } >> > > >> >
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Hi Ani, > > On 9/8/24 08:49, Ani Sinha wrote: >> error_report() is more appropriate for error situations. Replace fprintf with >> error_report. Cosmetic. No functional change. >> CC: qemu-trivial@nongnu.org >> CC: zhao1.liu@intel.com > > (Pointless to carry Cc line when patch is already reviewed next line) > >> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> >> Signed-off-by: Ani Sinha <anisinha@redhat.com> >> --- >> accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- >> 1 file changed, 18 insertions(+), 22 deletions(-) >> changelog: >> v2: fix a bug. >> v3: replace one instance of error_report() with error_printf(). added tags. >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index 75d11a07b2..5bc9d35b61 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) >> QLIST_INIT(&s->kvm_parked_vcpus); >> s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); >> if (s->fd == -1) { >> - fprintf(stderr, "Could not access KVM kernel module: %m\n"); >> + error_report("Could not access KVM kernel module: %m"); >> ret = -errno; >> goto err; >> } >> @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) >> if (ret >= 0) { >> ret = -EINVAL; >> } >> - fprintf(stderr, "kvm version too old\n"); >> + error_report("kvm version too old"); >> goto err; >> } >> if (ret > KVM_API_VERSION) { >> ret = -EINVAL; >> - fprintf(stderr, "kvm version not supported\n"); >> + error_report("kvm version not supported"); >> goto err; >> } >> @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) >> } while (ret == -EINTR); >> if (ret < 0) { >> - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, >> - strerror(-ret)); >> + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, >> + strerror(-ret)); >> #ifdef TARGET_S390X >> if (ret == -EINVAL) { >> - fprintf(stderr, >> - "Host kernel setup problem detected. Please verify:\n"); >> - fprintf(stderr, "- for kernels supporting the switch_amode or" >> - " user_mode parameters, whether\n"); >> - fprintf(stderr, >> - " user space is running in primary address space\n"); >> - fprintf(stderr, >> - "- for kernels supporting the vm.allocate_pgste sysctl, " >> - "whether it is enabled\n"); >> + error_report("Host kernel setup problem detected. > > \n" > > Should we use error_printf_unless_qmp() for the following? > > " Please verify:"); >> + error_report("- for kernels supporting the switch_amode or" >> + " user_mode parameters, whether"); >> + error_report(" user space is running in primary address space"); >> + error_report("- for kernels supporting the vm.allocate_pgste " >> + "sysctl, whether it is enabled"); Do not put newlines into error messages. error_report()'s function comment demands "The resulting message should be a single phrase, with no newline or trailing punctuation." You can do this: error_report(... the actual error message ...); error_printf(... hints on what to do about it ...); Questions? [...]
> On 27 Aug 2024, at 12:00 PM, Markus Armbruster <armbru@redhat.com> wrote: > > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> Hi Ani, >> >> On 9/8/24 08:49, Ani Sinha wrote: >>> error_report() is more appropriate for error situations. Replace fprintf with >>> error_report. Cosmetic. No functional change. >>> CC: qemu-trivial@nongnu.org >>> CC: zhao1.liu@intel.com >> >> (Pointless to carry Cc line when patch is already reviewed next line) >> >>> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> >>> Signed-off-by: Ani Sinha <anisinha@redhat.com> >>> --- >>> accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- >>> 1 file changed, 18 insertions(+), 22 deletions(-) >>> changelog: >>> v2: fix a bug. >>> v3: replace one instance of error_report() with error_printf(). added tags. >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>> index 75d11a07b2..5bc9d35b61 100644 >>> --- a/accel/kvm/kvm-all.c >>> +++ b/accel/kvm/kvm-all.c >>> @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) >>> QLIST_INIT(&s->kvm_parked_vcpus); >>> s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); >>> if (s->fd == -1) { >>> - fprintf(stderr, "Could not access KVM kernel module: %m\n"); >>> + error_report("Could not access KVM kernel module: %m"); >>> ret = -errno; >>> goto err; >>> } >>> @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) >>> if (ret >= 0) { >>> ret = -EINVAL; >>> } >>> - fprintf(stderr, "kvm version too old\n"); >>> + error_report("kvm version too old"); >>> goto err; >>> } >>> if (ret > KVM_API_VERSION) { >>> ret = -EINVAL; >>> - fprintf(stderr, "kvm version not supported\n"); >>> + error_report("kvm version not supported"); >>> goto err; >>> } >>> @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) >>> } while (ret == -EINTR); >>> if (ret < 0) { >>> - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, >>> - strerror(-ret)); >>> + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, >>> + strerror(-ret)); >>> #ifdef TARGET_S390X >>> if (ret == -EINVAL) { >>> - fprintf(stderr, >>> - "Host kernel setup problem detected. Please verify:\n"); >>> - fprintf(stderr, "- for kernels supporting the switch_amode or" >>> - " user_mode parameters, whether\n"); >>> - fprintf(stderr, >>> - " user space is running in primary address space\n"); >>> - fprintf(stderr, >>> - "- for kernels supporting the vm.allocate_pgste sysctl, " >>> - "whether it is enabled\n"); >>> + error_report("Host kernel setup problem detected. >> >> \n" >> >> Should we use error_printf_unless_qmp() for the following? >> >> " Please verify:"); >>> + error_report("- for kernels supporting the switch_amode or" >>> + " user_mode parameters, whether"); >>> + error_report(" user space is running in primary address space"); >>> + error_report("- for kernels supporting the vm.allocate_pgste " >>> + "sysctl, whether it is enabled"); > > Do not put newlines into error messages. error_report()'s function > comment demands "The resulting message should be a single phrase, with > no newline or trailing punctuation." > > You can do this: > > error_report(... the actual error message ...); > error_printf(... hints on what to do about it ...); > > Questions? Do you see any newlines in my proposed patch?
Ani Sinha <anisinha@redhat.com> writes: >> On 27 Aug 2024, at 12:00 PM, Markus Armbruster <armbru@redhat.com> wrote: >> >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >> >>> Hi Ani, >>> >>> On 9/8/24 08:49, Ani Sinha wrote: >>>> error_report() is more appropriate for error situations. Replace fprintf with >>>> error_report. Cosmetic. No functional change. >>>> CC: qemu-trivial@nongnu.org >>>> CC: zhao1.liu@intel.com >>> >>> (Pointless to carry Cc line when patch is already reviewed next line) >>> >>>> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> >>>> Signed-off-by: Ani Sinha <anisinha@redhat.com> >>>> --- >>>> accel/kvm/kvm-all.c | 40 ++++++++++++++++++---------------------- >>>> 1 file changed, 18 insertions(+), 22 deletions(-) >>>> changelog: >>>> v2: fix a bug. >>>> v3: replace one instance of error_report() with error_printf(). added tags. >>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>>> index 75d11a07b2..5bc9d35b61 100644 >>>> --- a/accel/kvm/kvm-all.c >>>> +++ b/accel/kvm/kvm-all.c >>>> @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) >>>> QLIST_INIT(&s->kvm_parked_vcpus); >>>> s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); >>>> if (s->fd == -1) { >>>> - fprintf(stderr, "Could not access KVM kernel module: %m\n"); >>>> + error_report("Could not access KVM kernel module: %m"); >>>> ret = -errno; >>>> goto err; >>>> } >>>> @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) >>>> if (ret >= 0) { >>>> ret = -EINVAL; >>>> } >>>> - fprintf(stderr, "kvm version too old\n"); >>>> + error_report("kvm version too old"); >>>> goto err; >>>> } >>>> if (ret > KVM_API_VERSION) { >>>> ret = -EINVAL; >>>> - fprintf(stderr, "kvm version not supported\n"); >>>> + error_report("kvm version not supported"); >>>> goto err; >>>> } >>>> @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) >>>> } while (ret == -EINTR); >>>> if (ret < 0) { >>>> - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, >>>> - strerror(-ret)); >>>> + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, >>>> + strerror(-ret)); >>>> #ifdef TARGET_S390X >>>> if (ret == -EINVAL) { >>>> - fprintf(stderr, >>>> - "Host kernel setup problem detected. Please verify:\n"); >>>> - fprintf(stderr, "- for kernels supporting the switch_amode or" >>>> - " user_mode parameters, whether\n"); >>>> - fprintf(stderr, >>>> - " user space is running in primary address space\n"); >>>> - fprintf(stderr, >>>> - "- for kernels supporting the vm.allocate_pgste sysctl, " >>>> - "whether it is enabled\n"); >>>> + error_report("Host kernel setup problem detected. >>> >>> \n" >>> >>> Should we use error_printf_unless_qmp() for the following? >>> >>> " Please verify:"); >>>> + error_report("- for kernels supporting the switch_amode or" >>>> + " user_mode parameters, whether"); >>>> + error_report(" user space is running in primary address space"); >>>> + error_report("- for kernels supporting the vm.allocate_pgste " >>>> + "sysctl, whether it is enabled"); >> >> Do not put newlines into error messages. error_report()'s function >> comment demands "The resulting message should be a single phrase, with >> no newline or trailing punctuation." >> >> You can do this: >> >> error_report(... the actual error message ...); >> error_printf(... hints on what to do about it ...); >> >> Questions? > > Do you see any newlines in my proposed patch? I see some in Philippe's suggestion. Your patch's use of multiple error_report() for a single error condition is inappropriate. Questions?
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 75d11a07b2..5bc9d35b61 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms) QLIST_INIT(&s->kvm_parked_vcpus); s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR); if (s->fd == -1) { - fprintf(stderr, "Could not access KVM kernel module: %m\n"); + error_report("Could not access KVM kernel module: %m"); ret = -errno; goto err; } @@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms) if (ret >= 0) { ret = -EINVAL; } - fprintf(stderr, "kvm version too old\n"); + error_report("kvm version too old"); goto err; } if (ret > KVM_API_VERSION) { ret = -EINVAL; - fprintf(stderr, "kvm version not supported\n"); + error_report("kvm version not supported"); goto err; } @@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms) } while (ret == -EINTR); if (ret < 0) { - fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret, - strerror(-ret)); + error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret, + strerror(-ret)); #ifdef TARGET_S390X if (ret == -EINVAL) { - fprintf(stderr, - "Host kernel setup problem detected. Please verify:\n"); - fprintf(stderr, "- for kernels supporting the switch_amode or" - " user_mode parameters, whether\n"); - fprintf(stderr, - " user space is running in primary address space\n"); - fprintf(stderr, - "- for kernels supporting the vm.allocate_pgste sysctl, " - "whether it is enabled\n"); + error_report("Host kernel setup problem detected. Please verify:"); + error_report("- for kernels supporting the switch_amode or" + " user_mode parameters, whether"); + error_report(" user space is running in primary address space"); + error_report("- for kernels supporting the vm.allocate_pgste " + "sysctl, whether it is enabled"); } #elif defined(TARGET_PPC) if (ret == -EINVAL) { - fprintf(stderr, - "PPC KVM module is not loaded. Try modprobe kvm_%s.\n", - (type == 2) ? "pr" : "hv"); + error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.", + (type == 2) ? "pr" : "hv"); } #endif goto err; @@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms) nc->name, nc->num, soft_vcpus_limit); if (nc->num > hard_vcpus_limit) { - fprintf(stderr, "Number of %s cpus requested (%d) exceeds " - "the maximum cpus supported by KVM (%d)\n", - nc->name, nc->num, hard_vcpus_limit); + error_report("Number of %s cpus requested (%d) exceeds " + "the maximum cpus supported by KVM (%d)", + nc->name, nc->num, hard_vcpus_limit); exit(1); } } @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms) } if (missing_cap) { ret = -EINVAL; - fprintf(stderr, "kvm does not support %s\n%s", - missing_cap->name, upgrade_note); + error_printf("kvm does not support %s\n%s", + missing_cap->name, upgrade_note); goto err; }