diff mbox

[PATCHv4,8/8] pseries: Clean up error reporting in htab migration functions

Message ID 1453174759-22837-9-git-send-email-david@gibson.dropbear.id.au (mailing list archive)
State New, archived
Headers show

Commit Message

David Gibson Jan. 19, 2016, 3:39 a.m. UTC 
The functions for migrating the hash page table on pseries machine type
(htab_save_setup() and htab_load()) can report some errors with an
explicit fprintf() before returning an appropriate error code.  Change these
to use error_report() instead.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Alexey Kardashevskiy Jan. 19, 2016, 4:31 a.m. UTC | #1 
On 01/19/2016 02:39 PM, David Gibson wrote:
> The functions for migrating the hash page table on pseries machine type
> (htab_save_setup() and htab_load()) can report some errors with an
> explicit fprintf() before returning an appropriate error code.  Change these
> to use error_report() instead.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>	

> ---
>   hw/ppc/spapr.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3cfacb9..1eb7d03 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1526,7 +1526,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>       int fd = -1;
>
>       if (version_id < 1 || version_id > 1) {
> -        fprintf(stderr, "htab_load() bad version\n");
> +        error_report("htab_load() bad version");
>           return -EINVAL;
>       }
>
> @@ -1547,8 +1547,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>
>           fd = kvmppc_get_htab_fd(true);
>           if (fd < 0) {
> -            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
> -                    strerror(errno));
> +            error_report("Unable to open fd to restore KVM hash table: %s",
> +                         strerror(errno));
>           }
>       }
>
> @@ -1568,9 +1568,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>           if ((index + n_valid + n_invalid) >
>               (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
>               /* Bad index in stream */
> -            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
> -                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
> -                    spapr->htab_shift);
> +            error_report(
> +                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
> +                index, n_valid, n_invalid, spapr->htab_shift);
>               return -EINVAL;
>           }
>
>
Markus Armbruster Jan. 19, 2016, 7:44 a.m. UTC | #2 
David Gibson <david@gibson.dropbear.id.au> writes:

> The functions for migrating the hash page table on pseries machine type
> (htab_save_setup() and htab_load()) can report some errors with an
> explicit fprintf() before returning an appropriate error code.  Change these
> to use error_report() instead.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3cfacb9..1eb7d03 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c

Lost this hunk:

  @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
           spapr->htab_fd = kvmppc_get_htab_fd(false);
           spapr->htab_fd_stale = false;
           if (spapr->htab_fd < 0) {
  -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
  -                    strerror(errno));
  +            error_report(
  +                "Unable to open fd for reading hash table from KVM: %s",
  +                strerror(errno));
               return -1;
           }
       }

Intentional?

> @@ -1526,7 +1526,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>      int fd = -1;
>  
>      if (version_id < 1 || version_id > 1) {
> -        fprintf(stderr, "htab_load() bad version\n");
> +        error_report("htab_load() bad version");
>          return -EINVAL;
>      }
>  
> @@ -1547,8 +1547,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>  
>          fd = kvmppc_get_htab_fd(true);
>          if (fd < 0) {
> -            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
> -                    strerror(errno));
> +            error_report("Unable to open fd to restore KVM hash table: %s",
> +                         strerror(errno));
>          }
>      }
>  
> @@ -1568,9 +1568,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>          if ((index + n_valid + n_invalid) >
>              (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
>              /* Bad index in stream */
> -            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
> -                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
> -                    spapr->htab_shift);
> +            error_report(
> +                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
> +                index, n_valid, n_invalid, spapr->htab_shift);
>              return -EINVAL;
>          }
David Gibson Jan. 19, 2016, 8:25 a.m. UTC | #3 
On Tue, Jan 19, 2016 at 08:44:51AM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > The functions for migrating the hash page table on pseries machine type
> > (htab_save_setup() and htab_load()) can report some errors with an
> > explicit fprintf() before returning an appropriate error code.  Change these
> > to use error_report() instead.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  hw/ppc/spapr.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 3cfacb9..1eb7d03 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> 
> Lost this hunk:
> 
>   @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>            spapr->htab_fd = kvmppc_get_htab_fd(false);
>            spapr->htab_fd_stale = false;
>            if (spapr->htab_fd < 0) {
>   -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
>   -                    strerror(errno));
>   +            error_report(
>   +                "Unable to open fd for reading hash table from KVM: %s",
>   +                strerror(errno));
>                return -1;
>            }
>        }
> 
> Intentional?

Yes.  As noted in the cover letter, this conflicts with another series
I'm working on, which will obsolete this change anyway.
Markus Armbruster Jan. 19, 2016, 12:21 p.m. UTC | #4 
David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, Jan 19, 2016 at 08:44:51AM +0100, Markus Armbruster wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > The functions for migrating the hash page table on pseries machine type
>> > (htab_save_setup() and htab_load()) can report some errors with an
>> > explicit fprintf() before returning an appropriate error code.  Change these
>> > to use error_report() instead.
>> >
>> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> > Reviewed-by: Thomas Huth <thuth@redhat.com>
>> > ---
>> >  hw/ppc/spapr.c | 12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> > index 3cfacb9..1eb7d03 100644
>> > --- a/hw/ppc/spapr.c
>> > +++ b/hw/ppc/spapr.c
>> 
>> Lost this hunk:
>> 
>>   @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>>            spapr->htab_fd = kvmppc_get_htab_fd(false);
>>            spapr->htab_fd_stale = false;
>>            if (spapr->htab_fd < 0) {
>>   -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
>>   -                    strerror(errno));
>>   +            error_report(
>>   +                "Unable to open fd for reading hash table from KVM: %s",
>>   +                strerror(errno));
>>                return -1;
>>            }
>>        }
>> 
>> Intentional?
>
> Yes.  As noted in the cover letter, this conflicts with another series
> I'm working on, which will obsolete this change anyway.

The patch doesn't quite hold what the commit message's promises, but I
guess that's okay.  An "except for htab_save_setup()" in the commit
message wouldn't hurt, though.
David Gibson Jan. 20, 2016, 1:57 a.m. UTC | #5 
On Tue, Jan 19, 2016 at 01:21:34PM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Tue, Jan 19, 2016 at 08:44:51AM +0100, Markus Armbruster wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > The functions for migrating the hash page table on pseries machine type
> >> > (htab_save_setup() and htab_load()) can report some errors with an
> >> > explicit fprintf() before returning an appropriate error code.  Change these
> >> > to use error_report() instead.
> >> >
> >> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> > ---
> >> >  hw/ppc/spapr.c | 12 ++++++------
> >> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> > index 3cfacb9..1eb7d03 100644
> >> > --- a/hw/ppc/spapr.c
> >> > +++ b/hw/ppc/spapr.c
> >> 
> >> Lost this hunk:
> >> 
> >>   @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> >>            spapr->htab_fd = kvmppc_get_htab_fd(false);
> >>            spapr->htab_fd_stale = false;
> >>            if (spapr->htab_fd < 0) {
> >>   -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
> >>   -                    strerror(errno));
> >>   +            error_report(
> >>   +                "Unable to open fd for reading hash table from KVM: %s",
> >>   +                strerror(errno));
> >>                return -1;
> >>            }
> >>        }
> >> 
> >> Intentional?
> >
> > Yes.  As noted in the cover letter, this conflicts with another series
> > I'm working on, which will obsolete this change anyway.
> 
> The patch doesn't quite hold what the commit message's promises, but I
> guess that's okay.  An "except for htab_save_setup()" in the commit
> message wouldn't hurt, though.

Ok.
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3cfacb9..1eb7d03 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1526,7 +1526,7 @@  static int htab_load(QEMUFile *f, void *opaque, int version_id)
     int fd = -1;
 
     if (version_id < 1 || version_id > 1) {
-        fprintf(stderr, "htab_load() bad version\n");
+        error_report("htab_load() bad version");
         return -EINVAL;
     }
 
@@ -1547,8 +1547,8 @@  static int htab_load(QEMUFile *f, void *opaque, int version_id)
 
         fd = kvmppc_get_htab_fd(true);
         if (fd < 0) {
-            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
-                    strerror(errno));
+            error_report("Unable to open fd to restore KVM hash table: %s",
+                         strerror(errno));
         }
     }
 
@@ -1568,9 +1568,9 @@  static int htab_load(QEMUFile *f, void *opaque, int version_id)
         if ((index + n_valid + n_invalid) >
             (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
             /* Bad index in stream */
-            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
-                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
-                    spapr->htab_shift);
+            error_report(
+                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
+                index, n_valid, n_invalid, spapr->htab_shift);
             return -EINVAL;
         }