[Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02  9:32 ` Fam Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-06-02  9:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, qemu-trivial, Jeff Cody, qemu-block

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/sheepdog.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index bd7cbed..a22f838 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2556,7 +2556,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
         }
 
         if (ret < 0) {
-            error_report("failed to save vmstate %s", strerror(errno));
+            if (load) {
+                error_report("failed to load vmstate %s", strerror(errno));
+            } else {
+                error_report("failed to save vmstate %s", strerror(errno));
+            }
             goto cleanup;
         }
 
-- 
2.4.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02  9:32 ` Fam Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-06-02  9:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, qemu-trivial, Jeff Cody, qemu-block

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/sheepdog.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index bd7cbed..a22f838 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2556,7 +2556,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
         }
 
         if (ret < 0) {
-            error_report("failed to save vmstate %s", strerror(errno));
+            if (load) {
+                error_report("failed to load vmstate %s", strerror(errno));
+            } else {
+                error_report("failed to save vmstate %s", strerror(errno));
+            }
             goto cleanup;
         }
 
-- 
2.4.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate
  2015-06-02  9:32 ` [Qemu-devel] " Fam Zheng
@ 2015-06-02  9:45   ` Kevin Wolf
  -1 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2015-06-02  9:45 UTC (permalink / raw)
  To: Fam Zheng
  Cc: qemu-block, qemu-trivial, Jeff Cody, qemu-devel, mitake.hitoshi,
	namei.unix

[ CC to Sheepdog maintainers ]

Am 02.06.2015 um 11:32 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/sheepdog.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index bd7cbed..a22f838 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2556,7 +2556,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
>          }
>  
>          if (ret < 0) {
> -            error_report("failed to save vmstate %s", strerror(errno));
> +            if (load) {
> +                error_report("failed to load vmstate %s", strerror(errno));
> +            } else {
> +                error_report("failed to save vmstate %s", strerror(errno));
> +            }
>              goto cleanup;
>          }

Why do we even print this message? We usually don't do this for a failed
request, and much less so if we don't actually add any information that
isn't covered by the return code. qemu_savevm_state() will already set
an error that is even visible in QMP.

In fact, errno doesn't even contain anything relevant here, or in most
(all?) other places that it's used in the sheepdog block driver.

I think we might be better off just removing the message.

Kevin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02  9:45   ` Kevin Wolf
  0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2015-06-02  9:45 UTC (permalink / raw)
  To: Fam Zheng
  Cc: qemu-block, qemu-trivial, Jeff Cody, qemu-devel, mitake.hitoshi,
	namei.unix

[ CC to Sheepdog maintainers ]

Am 02.06.2015 um 11:32 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/sheepdog.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index bd7cbed..a22f838 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2556,7 +2556,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
>          }
>  
>          if (ret < 0) {
> -            error_report("failed to save vmstate %s", strerror(errno));
> +            if (load) {
> +                error_report("failed to load vmstate %s", strerror(errno));
> +            } else {
> +                error_report("failed to save vmstate %s", strerror(errno));
> +            }
>              goto cleanup;
>          }

Why do we even print this message? We usually don't do this for a failed
request, and much less so if we don't actually add any information that
isn't covered by the return code. qemu_savevm_state() will already set
an error that is even visible in QMP.

In fact, errno doesn't even contain anything relevant here, or in most
(all?) other places that it's used in the sheepdog block driver.

I think we might be better off just removing the message.

Kevin

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate
  2015-06-02  9:45   ` [Qemu-devel] " Kevin Wolf
@ 2015-06-02 10:22     ` Fam Zheng
  -1 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-06-02 10:22 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: qemu-block, qemu-trivial, Jeff Cody, qemu-devel, mitake.hitoshi,
	namei.unix

On Tue, 06/02 11:45, Kevin Wolf wrote:
> [ CC to Sheepdog maintainers ]
> 
> Am 02.06.2015 um 11:32 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/sheepdog.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index bd7cbed..a22f838 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -2556,7 +2556,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
> >          }
> >  
> >          if (ret < 0) {
> > -            error_report("failed to save vmstate %s", strerror(errno));
> > +            if (load) {
> > +                error_report("failed to load vmstate %s", strerror(errno));
> > +            } else {
> > +                error_report("failed to save vmstate %s", strerror(errno));
> > +            }
> >              goto cleanup;
> >          }
> 
> Why do we even print this message? We usually don't do this for a failed
> request, and much less so if we don't actually add any information that
> isn't covered by the return code. qemu_savevm_state() will already set
> an error that is even visible in QMP.
> 
> In fact, errno doesn't even contain anything relevant here, or in most
> (all?) other places that it's used in the sheepdog block driver.

Now I followed the called function and agree that reporting errno is not
helpful.  Maybe it's better to do a clean up on error reportings in this
driver, later.

Fam


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02 10:22     ` Fam Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-06-02 10:22 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: qemu-block, qemu-trivial, Jeff Cody, qemu-devel, mitake.hitoshi,
	namei.unix

On Tue, 06/02 11:45, Kevin Wolf wrote:
> [ CC to Sheepdog maintainers ]
> 
> Am 02.06.2015 um 11:32 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/sheepdog.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index bd7cbed..a22f838 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -2556,7 +2556,11 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
> >          }
> >  
> >          if (ret < 0) {
> > -            error_report("failed to save vmstate %s", strerror(errno));
> > +            if (load) {
> > +                error_report("failed to load vmstate %s", strerror(errno));
> > +            } else {
> > +                error_report("failed to save vmstate %s", strerror(errno));
> > +            }
> >              goto cleanup;
> >          }
> 
> Why do we even print this message? We usually don't do this for a failed
> request, and much less so if we don't actually add any information that
> isn't covered by the return code. qemu_savevm_state() will already set
> an error that is even visible in QMP.
> 
> In fact, errno doesn't even contain anything relevant here, or in most
> (all?) other places that it's used in the sheepdog block driver.

Now I followed the called function and agree that reporting errno is not
helpful.  Maybe it's better to do a clean up on error reportings in this
driver, later.

Fam

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate
  2015-06-02  9:32 ` [Qemu-devel] " Fam Zheng
@ 2015-06-02 10:16   ` Michael Tokarev
  -1 siblings, 0 replies; 12+ messages in thread
From: Michael Tokarev @ 2015-06-02 10:16 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-trivial, Jeff Cody, qemu-block

02.06.2015 12:32, Fam Zheng wrote:
>          if (ret < 0) {
> -            error_report("failed to save vmstate %s", strerror(errno));
> +            if (load) {
> +                error_report("failed to load vmstate %s", strerror(errno));
> +            } else {
> +                error_report("failed to save vmstate %s", strerror(errno));
> +            }

 +            error_report("failed to %s vmstate: %s", load ? "load" : "save", strerror(errno));

(note also the addition of ":")
(besides what Kevin said).

Thanks,

/mjt


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02 10:16   ` Michael Tokarev
  0 siblings, 0 replies; 12+ messages in thread
From: Michael Tokarev @ 2015-06-02 10:16 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-trivial, Jeff Cody, qemu-block

02.06.2015 12:32, Fam Zheng wrote:
>          if (ret < 0) {
> -            error_report("failed to save vmstate %s", strerror(errno));
> +            if (load) {
> +                error_report("failed to load vmstate %s", strerror(errno));
> +            } else {
> +                error_report("failed to save vmstate %s", strerror(errno));
> +            }

 +            error_report("failed to %s vmstate: %s", load ? "load" : "save", strerror(errno));

(note also the addition of ":")
(besides what Kevin said).

Thanks,

/mjt

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate
  2015-06-02 10:16   ` [Qemu-devel] " Michael Tokarev
@ 2015-06-02 10:26     ` Fam Zheng
  -1 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-06-02 10:26 UTC (permalink / raw)
  To: Michael Tokarev
  Cc: Kevin Wolf, qemu-trivial, Jeff Cody, qemu-devel, qemu-block

On Tue, 06/02 13:16, Michael Tokarev wrote:
> 02.06.2015 12:32, Fam Zheng wrote:
> >          if (ret < 0) {
> > -            error_report("failed to save vmstate %s", strerror(errno));
> > +            if (load) {
> > +                error_report("failed to load vmstate %s", strerror(errno));
> > +            } else {
> > +                error_report("failed to save vmstate %s", strerror(errno));
> > +            }
> 
>  +            error_report("failed to %s vmstate: %s", load ? "load" : "save", strerror(errno));

The reason I didn't use a one-liner was, "git grep 'failed to load vmstate'"
in the code base would just work, besides my eyes also like the readability.

> 
> (note also the addition of ":")

Yes that applies to all error_report() in this file.

> (besides what Kevin said).
> 
> Thanks,

Thanks,

Fam


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02 10:26     ` Fam Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-06-02 10:26 UTC (permalink / raw)
  To: Michael Tokarev
  Cc: Kevin Wolf, qemu-trivial, Jeff Cody, qemu-devel, qemu-block

On Tue, 06/02 13:16, Michael Tokarev wrote:
> 02.06.2015 12:32, Fam Zheng wrote:
> >          if (ret < 0) {
> > -            error_report("failed to save vmstate %s", strerror(errno));
> > +            if (load) {
> > +                error_report("failed to load vmstate %s", strerror(errno));
> > +            } else {
> > +                error_report("failed to save vmstate %s", strerror(errno));
> > +            }
> 
>  +            error_report("failed to %s vmstate: %s", load ? "load" : "save", strerror(errno));

The reason I didn't use a one-liner was, "git grep 'failed to load vmstate'"
in the code base would just work, besides my eyes also like the readability.

> 
> (note also the addition of ":")

Yes that applies to all error_report() in this file.

> (besides what Kevin said).
> 
> Thanks,

Thanks,

Fam

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-trivial] [Qemu-block] [PATCH] sheepdog: Fix error message if failed to load vmstate
  2015-06-02 10:26     ` [Qemu-devel] " Fam Zheng
@ 2015-06-02 19:16       ` John Snow
  -1 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2015-06-02 19:16 UTC (permalink / raw)
  To: Fam Zheng, Michael Tokarev; +Cc: qemu-trivial, qemu-devel, qemu-block



On 06/02/2015 06:26 AM, Fam Zheng wrote:
> On Tue, 06/02 13:16, Michael Tokarev wrote:
>> 02.06.2015 12:32, Fam Zheng wrote:
>>>          if (ret < 0) {
>>> -            error_report("failed to save vmstate %s", strerror(errno));
>>> +            if (load) {
>>> +                error_report("failed to load vmstate %s", strerror(errno));
>>> +            } else {
>>> +                error_report("failed to save vmstate %s", strerror(errno));
>>> +            }
>>
>>  +            error_report("failed to %s vmstate: %s", load ? "load" : "save", strerror(errno));
> 
> The reason I didn't use a one-liner was, "git grep 'failed to load vmstate'"
> in the code base would just work, besides my eyes also like the readability.
> 

+1, Error messages should be kept on one line and intact where possible
and convenient. Greppable code is happy code.

>>
>> (note also the addition of ":")
> 
> Yes that applies to all error_report() in this file.
> 
>> (besides what Kevin said).
>>
>> Thanks,
> 
> Thanks,
> 
> Fam
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] sheepdog: Fix error message if failed to load vmstate
@ 2015-06-02 19:16       ` John Snow
  0 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2015-06-02 19:16 UTC (permalink / raw)
  To: Fam Zheng, Michael Tokarev; +Cc: qemu-trivial, qemu-devel, qemu-block



On 06/02/2015 06:26 AM, Fam Zheng wrote:
> On Tue, 06/02 13:16, Michael Tokarev wrote:
>> 02.06.2015 12:32, Fam Zheng wrote:
>>>          if (ret < 0) {
>>> -            error_report("failed to save vmstate %s", strerror(errno));
>>> +            if (load) {
>>> +                error_report("failed to load vmstate %s", strerror(errno));
>>> +            } else {
>>> +                error_report("failed to save vmstate %s", strerror(errno));
>>> +            }
>>
>>  +            error_report("failed to %s vmstate: %s", load ? "load" : "save", strerror(errno));
> 
> The reason I didn't use a one-liner was, "git grep 'failed to load vmstate'"
> in the code base would just work, besides my eyes also like the readability.
> 

+1, Error messages should be kept on one line and intact where possible
and convenient. Greppable code is happy code.

>>
>> (note also the addition of ":")
> 
> Yes that applies to all error_report() in this file.
> 
>> (besides what Kevin said).
>>
>> Thanks,
> 
> Thanks,
> 
> Fam
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-06-02 19:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-02  9:32 [Qemu-trivial] [PATCH] sheepdog: Fix error message if failed to load vmstate Fam Zheng
2015-06-02  9:32 ` [Qemu-devel] " Fam Zheng
2015-06-02  9:45 ` [Qemu-trivial] " Kevin Wolf
2015-06-02  9:45   ` [Qemu-devel] " Kevin Wolf
2015-06-02 10:22   ` [Qemu-trivial] " Fam Zheng
2015-06-02 10:22     ` [Qemu-devel] " Fam Zheng
2015-06-02 10:16 ` [Qemu-trivial] " Michael Tokarev
2015-06-02 10:16   ` [Qemu-devel] " Michael Tokarev
2015-06-02 10:26   ` [Qemu-trivial] " Fam Zheng
2015-06-02 10:26     ` [Qemu-devel] " Fam Zheng
2015-06-02 19:16     ` [Qemu-trivial] [Qemu-block] " John Snow
2015-06-02 19:16       ` [Qemu-devel] " John Snow

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.