Re: [PATCH] target/ppc: Add error reporting when opening file fails

[Daniel Henrique Barboza]

On 6/29/22 02:56, Markus Armbruster wrote:
> jianchunfu <jianchunfu@cmss.chinamobile.com> writes:
>
> > Add error reporting before return when opening file fails.
> >
> > Signed-off-by: jianchunfu <jianchunfu@cmss.chinamobile.com>
> > ---
> >   target/ppc/kvm.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index dc93b99189..ef9a871411 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -1798,6 +1798,7 @@ static int read_cpuinfo(const char *field, char *value, 
> > int len)
> >   
>     static int read_cpuinfo(const char *field, char *value, int len)
>     {
>         FILE *f;
>         int ret = -1;
>         int field_len = strlen(field);
>         char line[512];
>
> >       f = fopen("/proc/cpuinfo", "r");
> >       if (!f) {
> > +        fprintf(stderr, "Error opening /proc/cpuinfo: %s\n", strerror(errno));
> >           return -1;
> >       }
>
>         do {
>             if (!fgets(line, sizeof(line), f)) {
>                 break;
>             }
>             if (!strncmp(line, field, field_len)) {
>                 pstrcpy(value, len, line);
>                 ret = 0;
>                 break;
>             }
>         } while (*line);
>
>         fclose(f);
>
>         return ret;
>     }
>
> This function now reports an error on one out of two failures.  The
> caller can't tell whether it reported or not.
>
> Please use error_report() for errors, warn_report() for warnings, and
> info_report() for informational messages.
>
> But is it an error?  Here's the only caller:
>
>      static uint32_t kvmppc_get_tbfreq_procfs(void)
>      {
>          char line[512];
>          char *ns;
>          uint32_t tbfreq_fallback = NANOSECONDS_PER_SECOND;
>          uint32_t tbfreq_procfs;
>
>          if (read_cpuinfo("timebase", line, sizeof(line))) {
> --->        return tbfreq_fallback;
>          }
>
>          ns = strchr(line, ':');
>          if (!ns) {
> --->        return tbfreq_fallback;
>          }
>
>          tbfreq_procfs = atoi(++ns);
>
>          /* 0 is certainly not acceptable by the guest, return fallback value */
> --->    return tbfreq_procfs ? tbfreq_procfs : tbfreq_fallback;
>      }
>
> I marked the three spots that handle errors.  All quietly return
> NANOSECONDS_PER_SECOND.  The caller can't tell whether that happened.
>
> Reporting an error when we don't actually fail is confusing.  Better
> would be something like "Can't open /proc/cpuinfo, assuming timebase X",
> where X is the value you assume.
>
> Reporting this only in one out of several cases where we assume feels
> wrong.  If it's worth reporting in one case, why isn't it worth
> reporting in the other cases?  Is it worth reporting?
>
> Aside: the use of atoi() silently maps a timebase of 0 to
> NANOSECONDS_PER_SECOND.  Not fond of this function.  Not your patch's
> problem, of course.
>
> >   
> > @@ -1906,6 +1907,7 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
> >   
> >       f = fopen(filename, "rb");
> >       if (!f) {
> > +        fprintf(stderr, "Error opening %s: %s\n", filename, strerror(errno));
> >           return -1;
> >       }
>
> Preexisting: this function returns -1 when fopen() fails, 0 when fread()
> fails or read less data than expected.  Its caller
> kvmppc_read_int_cpu_dt() passes on the return value.  However, it is
> documented to return "0 if anything goes wrong".  Bug.  Not your patch's
> fault, but it needs fixing.

It does needs fixing , but it'll require some work because this code is
called in a lot of places. I'll hand this part personally because it's less
trivial than the error you reported above with read_cpuinfo() and
kvmppc_get_tbfreq_procfs().


I'll apply the relevant bits of jianchunfu's patch in the cleanup as well.


Thanks,


Daniel

> Similar issue as above: you make the function emit an error message on
> some, but not all failures.  If it's worth reporting in one case, why
> isn't it worth reporting in the other cases?  Is it worth reporting?