| Message ID | 20170420040003.31074-1-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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 >
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
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
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
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
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
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
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
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.
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
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; }
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
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;
Signed-off-by: Fam Zheng <famz@redhat.com> --- block/sheepdog.c | 1 + 1 file changed, 1 insertion(+)