1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] sheepdog: Set error when connection fails

Fam Zheng posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170420040003.31074-1-famz@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
[Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Fam Zheng 7 years ago 
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/sheepdog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
         qemu_set_nonblock(fd);
     } else {
         fd = -EIO;
+        error_setg(errp, "Failed to connect to sheepdog server");
     }
 
     return fd;
-- 
2.9.3
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Philippe Mathieu-Daudé 7 years ago 
On 04/20/2017 01:00 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/sheepdog.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fb9203e..7e889ee 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>          qemu_set_nonblock(fd);
>      } else {
>          fd = -EIO;
> +        error_setg(errp, "Failed to connect to sheepdog server");
>      }
>
>      return fd;
>
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Jeff Cody 7 years ago 
On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/sheepdog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fb9203e..7e889ee 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>          qemu_set_nonblock(fd);
>      } else {
>          fd = -EIO;
> +        error_setg(errp, "Failed to connect to sheepdog server");
>      }

A bit odd that we don't just return right away in this function after a
failed called to socket_connect(), but that has nothing to do with your
patch.

Reviewed-by: Jeff Cody <jcody@redhat.com>


>  
>      return fd;
> -- 
> 2.9.3
>
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Jeff Cody 7 years ago 
On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/sheepdog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fb9203e..7e889ee 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>          qemu_set_nonblock(fd);
>      } else {
>          fd = -EIO;
> +        error_setg(errp, "Failed to connect to sheepdog server");
>      }
>  
>      return fd;
> -- 
> 2.9.3
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Daniel P. Berrange 7 years ago 
On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/sheepdog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index fb9203e..7e889ee 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>          qemu_set_nonblock(fd);
>      } else {
>          fd = -EIO;
> +        error_setg(errp, "Failed to connect to sheepdog server");
>      }

This doesn't make much sense to me. The lines just above the
diff context have this:

    fd = socket_connect(s->addr, errp, NULL, NULL);

socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'. So AFAICT the new error_setg is
just throwing away the real detailed error message in favour of
a generic message.

So I'm puzzelled why we need to change anything - error reporting
should already be working fine.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Jeff Cody 7 years ago 
On Thu, Apr 20, 2017 at 04:30:16PM +0100, Daniel P. Berrange wrote:
> On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/sheepdog.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index fb9203e..7e889ee 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> >          qemu_set_nonblock(fd);
> >      } else {
> >          fd = -EIO;
> > +        error_setg(errp, "Failed to connect to sheepdog server");
> >      }
> 
> This doesn't make much sense to me. The lines just above the
> diff context have this:
> 
>     fd = socket_connect(s->addr, errp, NULL, NULL);
> 
> socket_connect should have already reported an error on "errp"
> in the scenario that 'fd == -1'. So AFAICT the new error_setg is
> just throwing away the real detailed error message in favour of
> a generic message.
> 
> So I'm puzzelled why we need to change anything - error reporting
> should already be working fine.
> 

Indeed, you are right. (Dequeuing patch)

It would also make more sense to check fd after the socket_connect() call
and return error then, rather than keep checking fd throughout the rest of
the function.

-Jeff
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Daniel P. Berrange 7 years ago 
On Thu, Apr 20, 2017 at 11:42:00AM -0400, Jeff Cody wrote:
> On Thu, Apr 20, 2017 at 04:30:16PM +0100, Daniel P. Berrange wrote:
> > On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  block/sheepdog.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index fb9203e..7e889ee 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> > >          qemu_set_nonblock(fd);
> > >      } else {
> > >          fd = -EIO;
> > > +        error_setg(errp, "Failed to connect to sheepdog server");
> > >      }
> > 
> > This doesn't make much sense to me. The lines just above the
> > diff context have this:
> > 
> >     fd = socket_connect(s->addr, errp, NULL, NULL);
> > 
> > socket_connect should have already reported an error on "errp"
> > in the scenario that 'fd == -1'. So AFAICT the new error_setg is
> > just throwing away the real detailed error message in favour of
> > a generic message.
> > 
> > So I'm puzzelled why we need to change anything - error reporting
> > should already be working fine.
> > 
> 
> Indeed, you are right. (Dequeuing patch)
> 
> It would also make more sense to check fd after the socket_connect() call
> and return error then, rather than keep checking fd throughout the rest of
> the function.

Yeah that would make it much more obvious that the error reporting is
correct.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Kevin Wolf 7 years ago 
Am 20.04.2017 um 17:30 hat Daniel P. Berrange geschrieben:
> On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/sheepdog.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index fb9203e..7e889ee 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> >          qemu_set_nonblock(fd);
> >      } else {
> >          fd = -EIO;
> > +        error_setg(errp, "Failed to connect to sheepdog server");
> >      }
> 
> This doesn't make much sense to me. The lines just above the
> diff context have this:
> 
>     fd = socket_connect(s->addr, errp, NULL, NULL);
> 
> socket_connect should have already reported an error on "errp"
> in the scenario that 'fd == -1'.

By the way, am I the only one who thinks that having errp anywhere else
than as the last argument is bad style? I can easily see myself missing
that this functions sets it because the last argument is NULL.

Kevin
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Jeff Cody 7 years ago 
On Thu, Apr 20, 2017 at 10:32:50PM +0200, Kevin Wolf wrote:
> Am 20.04.2017 um 17:30 hat Daniel P. Berrange geschrieben:
> > On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  block/sheepdog.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index fb9203e..7e889ee 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> > >          qemu_set_nonblock(fd);
> > >      } else {
> > >          fd = -EIO;
> > > +        error_setg(errp, "Failed to connect to sheepdog server");
> > >      }
> > 
> > This doesn't make much sense to me. The lines just above the
> > diff context have this:
> > 
> >     fd = socket_connect(s->addr, errp, NULL, NULL);
> > 
> > socket_connect should have already reported an error on "errp"
> > in the scenario that 'fd == -1'.
> 
> By the way, am I the only one who thinks that having errp anywhere else
> than as the last argument is bad style? I can easily see myself missing
> that this functions sets it because the last argument is NULL.
>

No, you are not - that is precisely why I missed it in my review.


-Jeff
Re: [Qemu-devel] [PATCH] sheepdog: Set error when connection fails
Posted by Fam Zheng 7 years ago 
On Thu, 04/20 22:32, Kevin Wolf wrote:
> Am 20.04.2017 um 17:30 hat Daniel P. Berrange geschrieben:
> > On Thu, Apr 20, 2017 at 12:00:03PM +0800, Fam Zheng wrote:
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  block/sheepdog.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index fb9203e..7e889ee 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> > >          qemu_set_nonblock(fd);
> > >      } else {
> > >          fd = -EIO;
> > > +        error_setg(errp, "Failed to connect to sheepdog server");
> > >      }
> > 
> > This doesn't make much sense to me. The lines just above the
> > diff context have this:
> > 
> >     fd = socket_connect(s->addr, errp, NULL, NULL);

Oops! :(

> > 
> > socket_connect should have already reported an error on "errp"
> > in the scenario that 'fd == -1'.
> 
> By the way, am I the only one who thinks that having errp anywhere else
> than as the last argument is bad style? I can easily see myself missing
> that this functions sets it because the last argument is NULL.

Hmm, exactly.. Socket code does this here and there, and it's hard to read.

Fam

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.