Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Daniel Henrique Barboza]

On 12/18/23 14:53, Peter Maydell wrote:
> On Mon, 18 Dec 2023 at 17:22, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > On 12/18/23 13:22, Natanael Copa wrote:
> > > strerrorname_np is non-portable and breaks building with musl libc.
> > >
> > > Use strerror(errno) instead, like we do other places.
> > >
> > > Cc: qemu-stable@nongnu.org
> > > Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error 
> > > msg)
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
> > > Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
> > > Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> > > ---
> > >    target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
> > >    1 file changed, 8 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > > index 45b6cf1cfa..117e33cf90 100644
> > > --- a/target/riscv/kvm/kvm-cpu.c
> > > +++ b/target/riscv/kvm/kvm-cpu.c
> > > @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
> > >                    multi_ext_cfg->supported = false;
> > >                    val = false;
> > >                } else {
> > > -                error_report("Unable to read ISA_EXT KVM register %s, "
> > > -                             "error code: %s", multi_ext_cfg->name,
> > > -                             strerrorname_np(errno));
> > > +                error_report("Unable to read ISA_EXT KVM register %s: %s",
> > > +                             multi_ext_cfg->name, strerror(errno));
> >
> >
> > The reason I did this change, as described in 082e9e4a58ba mentioned in the 
> > commit
> > message, was precisely to avoid things like this:
> >
> > qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error: no such 
> > file or directory
> >
> > The generic description of the error works well with file descriptors and so on 
> > but it's
> > weird in the KVM context. This patch is re-introducing it.
>
> We don't seem to worry about that in any of the other
> KVM code -- accel/kvm/ has lots of places that
> use strerror() or error_setg_errno().

I don't know how this is being used in other parts of accel/kvm, but in this 
particular
instance we're handling the errors from get_one_reg. The kernel docs describes 
the errors
the API may return as:

--------
Errors include:

ENOENT - no such register
EINVAL - invalid register ID, or no such register (...)
EPERM - (arm64) register access not allowed before vcpu finalization
---------


The API interprets ENOENT as "no such register", but strerror(errno) in this 
case will output
"no such file or directory". The generic description is forcing me to think 
"this error
makes no sense ... oh, this might be the description of ENOENT". At this point 
having an
"error code 2" instead is clearer to me.


Thanks,

Daniel

> thanks
> -- PMM