On 04.06.2017 00:46, Eric Blake wrote: > On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote: >> Move to modern errp scheme from just LOGging errors. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> --- >> nbd/server.c | 257 >> ++++++++++++++++++++++++++++++++++------------------------- >> 1 file changed, 150 insertions(+), 107 deletions(-) >> >> @@ -143,30 +143,30 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, >> uint32_t type, >> type, opt, len); >> >> magic = cpu_to_be64(NBD_REP_MAGIC); >> - ret = write_sync(ioc, &magic, sizeof(magic), NULL); >> + ret = write_sync(ioc, &magic, sizeof(magic), errp); >> if (ret < 0) { >> - LOG("write failed (rep magic)"); >> + error_prepend(errp, "write failed (rep magic): "); >> return ret; >> } > I'm wondering how much we really want to use error_prepend(), or if we > could just get away with using the original error message unchanged. > I'm not saying to rewrite the patch now that you've done the work, so > much as asking aloud whether the additional information in the error > messages will prove useful. > > There are definitely some ripple effects from your v2 posting of the > first half of the series that will require you to rebase this, but the > overall idea is sound.
I've tried to save old messages as is (+ additional info) > >> /* Send an error reply. >> * Return -errno on error, 0 on success. */ >> -static int GCC_FMT_ATTR(4, 5) >> +static int GCC_FMT_ATTR(5, 6) >> nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type, >> - uint32_t opt, const char *fmt, ...) >> + uint32_t opt, Error **errp, const char *fmt, ...) > Having errp not be the last argument is unusual, but I don't see how you > can do any differently with the var-args nature of the call. > >> @@ -505,26 +515,33 @@ static int nbd_negotiate_options(NBDClient *client) >> * disconnecting, but that we must also tolerate >> * guests that don't wait for our reply. */ >> ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, >> - clientflags); >> - return ret < 0 && ret != -EPIPE ? ret : 1; >> + clientflags, &local_err); >> + >> + if (ret < 0 && ret != -EPIPE) { >> + error_propagate(errp, local_err); >> + return ret; >> + } >> + >> + error_free(local_err); >> + return 1; > Of course, you'll have to simplify this portion. This is probably the > one place where you actually DO want to use: > > nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, > clientflags, NULL) > > because you truly do not care whether you had an error. > >> @@ -1090,18 +1113,18 @@ static int nbd_co_receive_request(NBDRequestData >> *req, NBDRequest *request) >> >> /* Sanity checks, part 2. */ >> if (request->from + request->len > client->exp->size) { >> - LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32 >> - ", Size: %" PRIu64, request->from, request->len, >> - (uint64_t)client->exp->size); >> + error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" >> PRIu32 >> + ", Size: %" PRIu64, request->from, request->len, >> + (uint64_t)client->exp->size); >> return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL; > Question - now that we are consistently setting a decent errp, do we > have any callers that care about specific -errno return values, or could > we further simplify the functions by just returning -1 (instead of > worrying about -errno) on failures? I think it is not bad to have informative error codes.. Example is previous discussion about EPIPE. > > >> @@ -1244,21 +1268,33 @@ static coroutine_fn void nbd_trip(void *opaque) >> >> reply: >> + if (local_err) { >> + error_report_err(local_err); >> + local_err = NULL; >> + } > This says that after we detect an error, we report it, > >> + >> + if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) { >> + error_setg(&local_err, "Failed to send reply"); >> + goto disconnect; >> + } > ...but then blindly try to send the reply anyways, forgetting that we > ever detected the original error. Is that going to bite us? Reported error is blk_* error, which should be returned to client. But I think it is not bad to report is on server too (and this corresponds to the old behavior). Creating extra "Error *blk_err" may help. > -- Best regards, Vladimir.