Re: [Qemu-block] [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 <address@hidden> 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 <address@hidden>
> > >> > ---
> > >> >  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;
}