Kevin Wolf <kw...@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 <f...@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.