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

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

[jianchunfu]

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)
 
     f = fopen("/proc/cpuinfo", "r");
     if (!f) {
+        fprintf(stderr, "Error opening /proc/cpuinfo: %s\n", strerror(errno));
         return -1;
     }
 
@@ -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;
     }
 
-- 
2.18.4

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

[Markus Armbruster]

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.

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?

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?
>