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

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

[Fam Zheng]

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

[Philippe Mathieu-Daudé]

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

[Jeff Cody]

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

[Jeff Cody]

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

[Daniel P. Berrange]

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

[Jeff Cody]

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

[Daniel P. Berrange]

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

[Kevin Wolf]

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

[Jeff Cody]

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

[Fam Zheng]

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

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

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

Yes, it's bad style because it's suprising.  Worth fixing.

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

[Daniel P. Berrange]

On Fri, Apr 21, 2017 at 07:43:36AM +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > 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.
> 
> Yes, it's bad style because it's suprising.  Worth fixing.

In fact we can simply delete the last two arguments to socket_connect()
entirely. Most code is now switched over to use QIOChannel APIs, so we
don't have anything which uses the non-blocking connect feature of
socket_connect() anymore.

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

[Fam Zheng]

On Fri, 04/21 09:31, Daniel P. Berrange wrote:
> On Fri, Apr 21, 2017 at 07:43:36AM +0200, Markus Armbruster wrote:
> > Kevin Wolf <kwolf@redhat.com> writes:
> > 
> > > 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.
> > 
> > Yes, it's bad style because it's suprising.  Worth fixing.
> 
> In fact we can simply delete the last two arguments to socket_connect()
> entirely. Most code is now switched over to use QIOChannel APIs, so we
> don't have anything which uses the non-blocking connect feature of
> socket_connect() anymore.

There is one caller that passes non-NULL last two parameters, which seems
non-trivial to convert:

static int net_socket_connect_init(NetClientState *peer,
                                   const char *model,
                                   const char *name,
                                   const char *host_str)
{
    socket_connect_data *c = g_new0(socket_connect_data, 1);
    int fd = -1;
    Error *local_error = NULL;

    c->peer = peer;
    c->model = g_strdup(model);
    c->name = g_strdup(name);
    c->saddr = socket_parse(host_str, &local_error);
    if (c->saddr == NULL) {
        goto err;
    }

    fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
    if (fd < 0) {
        goto err;
    }

    return 0;

err:
    error_report_err(local_error);
    socket_connect_data_free(c);
    return -1;
}

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

[Daniel P. Berrange]

On Fri, Apr 21, 2017 at 04:50:34PM +0800, Fam Zheng wrote:
> On Fri, 04/21 09:31, Daniel P. Berrange wrote:
> > On Fri, Apr 21, 2017 at 07:43:36AM +0200, Markus Armbruster wrote:
> > > Kevin Wolf <kwolf@redhat.com> writes:
> > > 
> > > > 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.
> > > 
> > > Yes, it's bad style because it's suprising.  Worth fixing.
> > 
> > In fact we can simply delete the last two arguments to socket_connect()
> > entirely. Most code is now switched over to use QIOChannel APIs, so we
> > don't have anything which uses the non-blocking connect feature of
> > socket_connect() anymore.
> 
> There is one caller that passes non-NULL last two parameters, which seems
> non-trivial to convert:
> 
> static int net_socket_connect_init(NetClientState *peer,
>                                    const char *model,
>                                    const char *name,
>                                    const char *host_str)
> {
>     socket_connect_data *c = g_new0(socket_connect_data, 1);
>     int fd = -1;
>     Error *local_error = NULL;
> 
>     c->peer = peer;
>     c->model = g_strdup(model);
>     c->name = g_strdup(name);
>     c->saddr = socket_parse(host_str, &local_error);
>     if (c->saddr == NULL) {
>         goto err;
>     }
> 
>     fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
>     if (fd < 0) {
>         goto err;
>     }
> 
>     return 0;
> 
> err:
>     error_report_err(local_error);
>     socket_connect_data_free(c);
>     return -1;
> }

Oh i missed that in my grep. I think it could probably be converted to
QIOChannel though, at least for the initial connection setup. Then
just steal the FD from the QIOChannel object for direct usage thereafter
or something like that.


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