Move to modern errp scheme from just LOGging errors.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
nbd/server.c | 268 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 161 insertions(+), 107 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index f0bff23b8b..fa493602dd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -134,7 +134,7 @@ static void nbd_client_receive_next_request(NBDClient *client);
/* Send a reply header, including length, but no payload.
* Return -errno on error, 0 on success. */
static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
- uint32_t opt, uint32_t len)
+ uint32_t opt, uint32_t len, Error **errp)
{
uint64_t magic;
@@ -142,23 +142,26 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
type, opt, len);
magic = cpu_to_be64(NBD_REP_MAGIC);
- if (nbd_write(ioc, &magic, sizeof(magic), NULL) < 0) {
- LOG("write failed (rep magic)");
+ if (nbd_write(ioc, &magic, sizeof(magic), errp) < 0) {
+ error_prepend(errp, "write failed (rep magic): ");
return -EINVAL;
}
+
opt = cpu_to_be32(opt);
- if (nbd_write(ioc, &opt, sizeof(opt), NULL) < 0) {
- LOG("write failed (rep opt)");
+ if (nbd_write(ioc, &opt, sizeof(opt), errp) < 0) {
+ error_prepend(errp, "write failed (rep opt): ");
return -EINVAL;
}
+
type = cpu_to_be32(type);
- if (nbd_write(ioc, &type, sizeof(type), NULL) < 0) {
- LOG("write failed (rep type)");
+ if (nbd_write(ioc, &type, sizeof(type), errp) < 0) {
+ error_prepend(errp, "write failed (rep type): ");
return -EINVAL;
}
+
len = cpu_to_be32(len);
- if (nbd_write(ioc, &len, sizeof(len), NULL) < 0) {
- LOG("write failed (rep data length)");
+ if (nbd_write(ioc, &len, sizeof(len), errp) < 0) {
+ error_prepend(errp, "write failed (rep data length): ");
return -EINVAL;
}
return 0;
@@ -166,16 +169,17 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
/* Send a reply header with default 0 length.
* Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
+static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt,
+ Error **errp)
{
- return nbd_negotiate_send_rep_len(ioc, type, opt, 0);
+ return nbd_negotiate_send_rep_len(ioc, type, opt, 0, errp);
}
/* 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, ...)
{
va_list va;
char *msg;
@@ -188,16 +192,17 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
len = strlen(msg);
assert(len < 4096);
TRACE("sending error message \"%s\"", msg);
- ret = nbd_negotiate_send_rep_len(ioc, type, opt, len);
+ ret = nbd_negotiate_send_rep_len(ioc, type, opt, len, errp);
if (ret < 0) {
goto out;
}
- if (nbd_write(ioc, msg, len, NULL) < 0) {
- LOG("write failed (error message)");
+ if (nbd_write(ioc, msg, len, errp) < 0) {
+ error_prepend(errp, "write failed (error message): ");
ret = -EIO;
} else {
ret = 0;
}
+
out:
g_free(msg);
return ret;
@@ -205,7 +210,8 @@ out:
/* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
* Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
+static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp,
+ Error **errp)
{
size_t name_len, desc_len;
uint32_t len;
@@ -217,53 +223,60 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
name_len = strlen(name);
desc_len = strlen(desc);
len = name_len + desc_len + sizeof(len);
- ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
+ ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len,
+ errp);
if (ret < 0) {
return ret;
}
len = cpu_to_be32(name_len);
- if (nbd_write(ioc, &len, sizeof(len), NULL) < 0) {
- LOG("write failed (name length)");
+ if (nbd_write(ioc, &len, sizeof(len), errp) < 0) {
+ error_prepend(errp, "write failed (name length): ");
return -EINVAL;
}
- if (nbd_write(ioc, name, name_len, NULL) < 0) {
- LOG("write failed (name buffer)");
+
+ if (nbd_write(ioc, name, name_len, errp) < 0) {
+ error_prepend(errp, "write failed (name buffer): ");
return -EINVAL;
}
- if (nbd_write(ioc, desc, desc_len, NULL) < 0) {
- LOG("write failed (description buffer)");
+
+ if (nbd_write(ioc, desc, desc_len, errp) < 0) {
+ error_prepend(errp, "write failed (description buffer): ");
return -EINVAL;
}
+
return 0;
}
/* Process the NBD_OPT_LIST command, with a potential series of replies.
* Return -errno on error, 0 on success. */
-static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
+static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length,
+ Error **errp)
{
NBDExport *exp;
if (length) {
- if (nbd_drop(client->ioc, length, NULL) < 0) {
+ if (nbd_drop(client->ioc, length, errp) < 0) {
return -EIO;
}
return nbd_negotiate_send_rep_err(client->ioc,
NBD_REP_ERR_INVALID, NBD_OPT_LIST,
+ errp,
"OPT_LIST should not have length");
}
/* For each export, send a NBD_REP_SERVER reply. */
QTAILQ_FOREACH(exp, &exports, next) {
- if (nbd_negotiate_send_rep_list(client->ioc, exp)) {
+ if (nbd_negotiate_send_rep_list(client->ioc, exp, errp)) {
return -EINVAL;
}
}
/* Finish with a NBD_REP_ACK. */
- return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST);
+ return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST, errp);
}
-static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
+static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
+ Error **errp)
{
char name[NBD_MAX_NAME_SIZE + 1];
@@ -272,11 +285,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
*/
TRACE("Checking length");
if (length >= sizeof(name)) {
- LOG("Bad length received");
+ error_setg(errp, "Bad length received");
return -EINVAL;
}
- if (nbd_read(client->ioc, name, length, NULL) < 0) {
- LOG("read failed");
+ if (nbd_read(client->ioc, name, length, errp) < 0) {
+ error_prepend(errp, "read failed: ");
return -EINVAL;
}
name[length] = '\0';
@@ -285,7 +298,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
client->exp = nbd_export_find(name);
if (!client->exp) {
- LOG("export not found");
+ error_setg(errp, "export not found");
return -EINVAL;
}
@@ -298,7 +311,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
/* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
* new channel for all further (now-encrypted) communication. */
static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
- uint32_t length)
+ uint32_t length,
+ Error **errp)
{
QIOChannel *ioc;
QIOChannelTLS *tioc;
@@ -307,23 +321,24 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
TRACE("Setting up TLS");
ioc = client->ioc;
if (length) {
- if (nbd_drop(ioc, length, NULL) < 0) {
+ if (nbd_drop(ioc, length, errp) < 0) {
return NULL;
}
nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
+ errp,
"OPT_STARTTLS should not have length");
return NULL;
}
if (nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
- NBD_OPT_STARTTLS) < 0) {
+ NBD_OPT_STARTTLS, errp) < 0) {
return NULL;
}
tioc = qio_channel_tls_new_server(ioc,
client->tlscreds,
client->tlsaclname,
- NULL);
+ errp);
if (!tioc) {
return NULL;
}
@@ -342,7 +357,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
g_main_loop_unref(data.loop);
if (data.error) {
object_unref(OBJECT(tioc));
- error_free(data.error);
+ error_propagate(errp, data.error);
return NULL;
}
@@ -352,14 +367,16 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
/* nbd_negotiate_options
* Process all NBD_OPT_* client option commands.
* Return:
- * -errno on error
- * 0 on successful negotiation
- * 1 if client sent NBD_OPT_ABORT, i.e. on legal disconnect
+ * -errno on error, errp is set
+ * 0 on successful negotiation, errp is not set
+ * 1 if client sent NBD_OPT_ABORT, i.e. on legal disconnect,
+ * errp is not set
*/
-static int nbd_negotiate_options(NBDClient *client)
+static int nbd_negotiate_options(NBDClient *client, Error **errp)
{
uint32_t flags;
bool fixedNewstyle = false;
+ Error *local_err = NULL;
/* Client sends:
[ 0 .. 3] client flags
@@ -375,8 +392,8 @@ static int nbd_negotiate_options(NBDClient *client)
... Rest of request
*/
- if (nbd_read(client->ioc, &flags, sizeof(flags), NULL) < 0) {
- LOG("read failed");
+ if (nbd_read(client->ioc, &flags, sizeof(flags), errp) < 0) {
+ error_prepend(errp, "read failed: ");
return -EIO;
}
TRACE("Checking client flags");
@@ -392,7 +409,7 @@ static int nbd_negotiate_options(NBDClient *client)
flags &= ~NBD_FLAG_C_NO_ZEROES;
}
if (flags != 0) {
- TRACE("Unknown client flags 0x%" PRIx32 " received", flags);
+ error_setg(errp, "Unknown client flags 0x%" PRIx32 " received", flags);
return -EIO;
}
@@ -401,26 +418,25 @@ static int nbd_negotiate_options(NBDClient *client)
uint32_t clientflags, length;
uint64_t magic;
- if (nbd_read(client->ioc, &magic, sizeof(magic), NULL) < 0) {
- LOG("read failed");
+ if (nbd_read(client->ioc, &magic, sizeof(magic), errp) < 0) {
+ error_prepend(errp, "read failed: ");
return -EINVAL;
}
TRACE("Checking opts magic");
if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
- LOG("Bad magic received");
+ error_setg(errp, "Bad magic received");
return -EINVAL;
}
if (nbd_read(client->ioc, &clientflags,
- sizeof(clientflags), NULL) < 0)
- {
- LOG("read failed");
+ sizeof(clientflags), errp) < 0) {
+ error_prepend(errp, "read failed: ");
return -EINVAL;
}
clientflags = be32_to_cpu(clientflags);
- if (nbd_read(client->ioc, &length, sizeof(length), NULL) < 0) {
- LOG("read failed");
+ if (nbd_read(client->ioc, &length, sizeof(length), errp) < 0) {
+ error_prepend(errp, "read failed: ");
return -EINVAL;
}
length = be32_to_cpu(length);
@@ -430,12 +446,12 @@ static int nbd_negotiate_options(NBDClient *client)
client->ioc == (QIOChannel *)client->sioc) {
QIOChannel *tioc;
if (!fixedNewstyle) {
- TRACE("Unsupported option 0x%" PRIx32, clientflags);
+ error_setg(errp, "Unsupported option 0x%" PRIx32, clientflags);
return -EINVAL;
}
switch (clientflags) {
case NBD_OPT_STARTTLS:
- tioc = nbd_negotiate_handle_starttls(client, length);
+ tioc = nbd_negotiate_handle_starttls(client, length, errp);
if (!tioc) {
return -EIO;
}
@@ -445,16 +461,17 @@ static int nbd_negotiate_options(NBDClient *client)
case NBD_OPT_EXPORT_NAME:
/* No way to return an error to client, so drop connection */
- TRACE("Option 0x%x not permitted before TLS", clientflags);
+ error_setg(errp, "Option 0x%x not permitted before TLS",
+ clientflags);
return -EINVAL;
default:
- if (nbd_drop(client->ioc, length, NULL) < 0) {
+ if (nbd_drop(client->ioc, length, errp) < 0) {
return -EIO;
}
ret = nbd_negotiate_send_rep_err(client->ioc,
NBD_REP_ERR_TLS_REQD,
- clientflags,
+ clientflags, errp,
"Option 0x%" PRIx32
"not permitted before TLS",
clientflags);
@@ -470,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
} else if (fixedNewstyle) {
switch (clientflags) {
case NBD_OPT_LIST:
- ret = nbd_negotiate_handle_list(client, length);
+ ret = nbd_negotiate_handle_list(client, length, errp);
if (ret < 0) {
return ret;
}
@@ -480,25 +497,33 @@ static int nbd_negotiate_options(NBDClient *client)
/* NBD spec says we must try to reply before
* disconnecting, but that we must also tolerate
* guests that don't wait for our reply. */
- nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags);
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags,
+ &local_err);
+
+ if (local_err != NULL) {
+ TRACE("Reply to NBD_OPT_ABORT request failed: %s",
+ error_get_pretty(local_err));
+ error_free(local_err);
+ }
+
return 1;
case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client, length);
+ return nbd_negotiate_handle_export_name(client, length, errp);
case NBD_OPT_STARTTLS:
- if (nbd_drop(client->ioc, length, NULL) < 0) {
+ if (nbd_drop(client->ioc, length, errp) < 0) {
return -EIO;
}
if (client->tlscreds) {
ret = nbd_negotiate_send_rep_err(client->ioc,
NBD_REP_ERR_INVALID,
- clientflags,
+ clientflags, errp,
"TLS already enabled");
} else {
ret = nbd_negotiate_send_rep_err(client->ioc,
NBD_REP_ERR_POLICY,
- clientflags,
+ clientflags, errp,
"TLS not configured");
}
if (ret < 0) {
@@ -506,12 +531,12 @@ static int nbd_negotiate_options(NBDClient *client)
}
break;
default:
- if (nbd_drop(client->ioc, length, NULL) < 0) {
+ if (nbd_drop(client->ioc, length, errp) < 0) {
return -EIO;
}
ret = nbd_negotiate_send_rep_err(client->ioc,
NBD_REP_ERR_UNSUP,
- clientflags,
+ clientflags, errp,
"Unsupported option 0x%"
PRIx32,
clientflags);
@@ -527,10 +552,10 @@ static int nbd_negotiate_options(NBDClient *client)
*/
switch (clientflags) {
case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client, length);
+ return nbd_negotiate_handle_export_name(client, length, errp);
default:
- TRACE("Unsupported option 0x%" PRIx32, clientflags);
+ error_setg(errp, "Unsupported option 0x%" PRIx32, clientflags);
return -EINVAL;
}
}
@@ -539,11 +564,12 @@ static int nbd_negotiate_options(NBDClient *client)
/* nbd_negotiate
* Return:
- * -errno on error
- * 0 on successful negotiation
- * 1 if client sent NBD_OPT_ABORT, i.e. on legal disconnect
+ * -errno on error, errp is set
+ * 0 on successful negotiation, errp is not set
+ * 1 if client sent NBD_OPT_ABORT, i.e. on legal disconnect,
+ * errp is not set
*/
-static coroutine_fn int nbd_negotiate(NBDClient *client)
+static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
{
char buf[8 + 8 + 8 + 128];
int ret;
@@ -591,21 +617,23 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
if (oldStyle) {
if (client->tlscreds) {
- TRACE("TLS cannot be enabled with oldstyle protocol");
+ error_setg(errp, "TLS cannot be enabled with oldstyle protocol");
return -EINVAL;
}
- if (nbd_write(client->ioc, buf, sizeof(buf), NULL) < 0) {
- LOG("write failed");
+ if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) {
+ error_prepend(errp, "write failed: ");
return -EINVAL;
}
} else {
- if (nbd_write(client->ioc, buf, 18, NULL) < 0) {
- LOG("write failed");
+ if (nbd_write(client->ioc, buf, 18, errp) < 0) {
+ error_prepend(errp, "write failed: ");
return -EINVAL;
}
- ret = nbd_negotiate_options(client);
+ ret = nbd_negotiate_options(client, errp);
if (ret != 0) {
- LOG("option negotiation failed");
+ if (ret < 0) {
+ error_prepend(errp, "option negotiation failed: ");
+ }
return ret;
}
@@ -614,9 +642,9 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
stq_be_p(buf + 18, client->exp->size);
stw_be_p(buf + 26, client->exp->nbdflags | myflags);
len = client->no_zeroes ? 10 : sizeof(buf) - 18;
- ret = nbd_write(client->ioc, buf + 18, len, NULL);
+ ret = nbd_write(client->ioc, buf + 18, len, errp);
if (ret < 0) {
- LOG("write failed");
+ error_prepend(errp, "write failed: ");
return ret;
}
}
@@ -626,13 +654,14 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
return 0;
}
-static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
+static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request,
+ Error **errp)
{
uint8_t buf[NBD_REQUEST_SIZE];
uint32_t magic;
int ret;
- ret = nbd_read(ioc, buf, sizeof(buf), NULL);
+ ret = nbd_read(ioc, buf, sizeof(buf), errp);
if (ret < 0) {
return ret;
}
@@ -658,7 +687,7 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
magic, request->flags, request->type, request->from, request->len);
if (magic != NBD_REQUEST_MAGIC) {
- LOG("invalid magic (got 0x%" PRIx32 ")", magic);
+ error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
return -EINVAL;
}
return 0;
@@ -1004,13 +1033,14 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
* the client (although the caller may still need to disconnect after reporting
* the error).
*/
-static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
+static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
+ Error **errp)
{
NBDClient *client = req->client;
g_assert(qemu_in_coroutine());
assert(client->recv_coroutine == qemu_coroutine_self());
- if (nbd_receive_request(client->ioc, request) < 0) {
+ if (nbd_receive_request(client->ioc, request, errp) < 0) {
return -EIO;
}
@@ -1032,27 +1062,29 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
* checks as possible until after reading any NBD_CMD_WRITE
* payload, so we can try and keep the connection alive. */
if ((request->from + request->len) < request->from) {
- LOG("integer overflow detected, you're probably being attacked");
+ error_setg(errp,
+ "integer overflow detected, you're probably being attacked");
return -EINVAL;
}
if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
if (request->len > NBD_MAX_BUFFER_SIZE) {
- LOG("len (%" PRIu32" ) is larger than max len (%u)",
- request->len, NBD_MAX_BUFFER_SIZE);
+ error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
+ request->len, NBD_MAX_BUFFER_SIZE);
return -EINVAL;
}
req->data = blk_try_blockalign(client->exp->blk, request->len);
if (req->data == NULL) {
+ error_setg(errp, "No memory");
return -ENOMEM;
}
}
if (request->type == NBD_CMD_WRITE) {
TRACE("Reading %" PRIu32 " byte(s)", request->len);
- if (nbd_read(client->ioc, req->data, request->len, NULL) < 0) {
- LOG("reading from socket failed");
+ if (nbd_read(client->ioc, req->data, request->len, errp) < 0) {
+ error_prepend(errp, "reading from socket failed: ");
return -EIO;
}
req->complete = true;
@@ -1060,18 +1092,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;
}
if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
- LOG("unsupported flags (got 0x%x)", request->flags);
+ error_setg(errp, "unsupported flags (got 0x%x)", request->flags);
return -EINVAL;
}
if (request->type != NBD_CMD_WRITE_ZEROES &&
(request->flags & NBD_CMD_FLAG_NO_HOLE)) {
- LOG("unexpected flags (got 0x%x)", request->flags);
+ error_setg(errp, "unexpected flags (got 0x%x)", request->flags);
return -EINVAL;
}
@@ -1089,6 +1121,7 @@ static coroutine_fn void nbd_trip(void *opaque)
int ret;
int flags;
int reply_data_len = 0;
+ Error *local_err = NULL;
TRACE("Reading request.");
if (client->closing) {
@@ -1097,7 +1130,7 @@ static coroutine_fn void nbd_trip(void *opaque)
}
req = nbd_request_get(client);
- ret = nbd_co_receive_request(req, &request);
+ ret = nbd_co_receive_request(req, &request, &local_err);
client->recv_coroutine = NULL;
nbd_client_receive_next_request(client);
if (ret == -EIO) {
@@ -1128,7 +1161,7 @@ static coroutine_fn void nbd_trip(void *opaque)
if (request.flags & NBD_CMD_FLAG_FUA) {
ret = blk_co_flush(exp->blk);
if (ret < 0) {
- LOG("flush failed");
+ error_setg_errno(&local_err, -ret, "flush failed");
reply.error = -ret;
break;
}
@@ -1137,7 +1170,7 @@ static coroutine_fn void nbd_trip(void *opaque)
ret = blk_pread(exp->blk, request.from + exp->dev_offset,
req->data, request.len);
if (ret < 0) {
- LOG("reading from file failed");
+ error_setg_errno(&local_err, -ret, "reading from file failed");
reply.error = -ret;
break;
}
@@ -1164,7 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
req->data, request.len, flags);
if (ret < 0) {
- LOG("writing to file failed");
+ error_setg_errno(&local_err, -ret, "writing to file failed");
reply.error = -ret;
}
@@ -1173,7 +1206,7 @@ static coroutine_fn void nbd_trip(void *opaque)
TRACE("Request type is WRITE_ZEROES");
if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
- TRACE("Server is read-only, return error");
+ error_setg(&local_err, "Server is read-only, return error");
reply.error = EROFS;
break;
}
@@ -1190,7 +1223,7 @@ static coroutine_fn void nbd_trip(void *opaque)
ret = blk_pwrite_zeroes(exp->blk, request.from + exp->dev_offset,
request.len, flags);
if (ret < 0) {
- LOG("writing to file failed");
+ error_setg_errno(&local_err, -ret, "writing to file failed");
reply.error = -ret;
}
@@ -1204,7 +1237,7 @@ static coroutine_fn void nbd_trip(void *opaque)
ret = blk_co_flush(exp->blk);
if (ret < 0) {
- LOG("flush failed");
+ error_setg_errno(&local_err, -ret, "flush failed");
reply.error = -ret;
}
@@ -1214,21 +1247,35 @@ static coroutine_fn void nbd_trip(void *opaque)
ret = blk_co_pdiscard(exp->blk, request.from + exp->dev_offset,
request.len);
if (ret < 0) {
- LOG("discard failed");
+ error_setg_errno(&local_err, -ret, "discard failed");
reply.error = -ret;
}
break;
default:
- LOG("invalid request type (%" PRIu32 ") received", request.type);
+ error_setg(&local_err, "invalid request type (%" PRIu32 ") received",
+ request.type);
reply.error = EINVAL;
}
reply:
+ if (local_err) {
+ /* If we are here local_err is not fatal error, already stored in
+ * reply.error */
+ error_report_err(local_err);
+ local_err = NULL;
+ }
+
+ if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
+ error_setg(&local_err, "Failed to send reply");
+ goto disconnect;
+ }
+
/* We must disconnect after NBD_CMD_WRITE if we did not
* read the payload.
*/
- if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
+ if (!req->complete) {
+ error_setg(&local_err, "Request handling failed in intermediate state");
goto disconnect;
}
@@ -1240,6 +1287,9 @@ done:
return;
disconnect:
+ if (local_err) {
+ error_reportf_err(local_err, "Disconnect client, due to: ");
+ }
nbd_request_put(req);
client_close(client, true);
nbd_client_put(client);
@@ -1258,6 +1308,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
{
NBDClient *client = opaque;
NBDExport *exp = client->exp;
+ Error *local_err = NULL;
if (exp) {
nbd_export_get(exp);
@@ -1265,7 +1316,10 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
}
qemu_co_mutex_init(&client->send_lock);
- if (nbd_negotiate(client)) {
+ if (nbd_negotiate(client, &local_err)) {
+ if (local_err) {
+ error_report_err(local_err);
+ }
client_close(client, false);
return;
}
--
2.11.1
On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> nbd/server.c | 268 +++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 161 insertions(+), 107 deletions(-)
Unfortunately longer, but I'm okay with that (and we already did it
client-side, so this is just a case of a patch series being spread out
over time).
>
> /* 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, ...)
> {
Markus, this violates our usual idiom of errp last in code that is not
directly operating on an Error (the actual Error implementation in
error.c being the main obvious exception that lists errp first), but I
don't see any better approach. Do you have any thoughts on it?
> - if (nbd_read(client->ioc, name, length, NULL) < 0) {
> - LOG("read failed");
> + if (nbd_read(client->ioc, name, length, errp) < 0) {
> + error_prepend(errp, "read failed: ");
> return -EINVAL;
> }
I'm still not convinced we need quite as many error_prepend() calls (it
may be simpler to just use the underlying error message as-is, without
trying to prepend) - but deciding where it is fluff is a cleanup while
this patch is all about a mechanical conversion, so I'm not going to
hold up this patch while we solve that debate.
> tioc = qio_channel_tls_new_server(ioc,
> client->tlscreds,
> client->tlsaclname,
> - NULL);
> + errp);
Yay for wiring things up to no longer ignore original error messages.
> @@ -352,14 +367,16 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
> /* nbd_negotiate_options
> * Process all NBD_OPT_* client option commands.
> * Return:
> - * -errno on error
> - * 0 on successful negotiation
> - * 1 if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> + * -errno on error, errp is set
> + * 0 on successful negotiation, errp is not set
> + * 1 if client sent NBD_OPT_ABORT, i.e. on legal disconnect,
> + * errp is not set
You'll have some rebase churn based on my comments on 1/6 (where I
suggested s/legal/valid/). I'm not sure the change about documenting
whether errp is set is even necessary (we have enough common usage of
errp, that it is kind of easy to just assume that everyone knows errp is
set only on negative return); but it doesn't hurt to keep it.
> @@ -480,25 +497,33 @@ static int nbd_negotiate_options(NBDClient *client)
> /* NBD spec says we must try to reply before
> * disconnecting, but that we must also tolerate
> * guests that don't wait for our reply. */
> - nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags);
> + nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags,
> + &local_err);
> +
> + if (local_err != NULL) {
> + TRACE("Reply to NBD_OPT_ABORT request failed: %s",
> + error_get_pretty(local_err));
> + error_free(local_err);
> + }
> +
Not sure this is necessary. I would have just used:
nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags, NULL);
and then you don't even need a local_err variable.
> @@ -1089,6 +1121,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> int ret;
> int flags;
> int reply_data_len = 0;
> + Error *local_err = NULL;
>
> TRACE("Reading request.");
> if (client->closing) {
> @@ -1097,7 +1130,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> @@ -1128,7 +1161,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> if (request.flags & NBD_CMD_FLAG_FUA) {
> ret = blk_co_flush(exp->blk);
> if (ret < 0) {
> - LOG("flush failed");
> + error_setg_errno(&local_err, -ret, "flush failed");
> reply.error = -ret;
> break;
> }
> reply:
> + if (local_err) {
> + /* If we are here local_err is not fatal error, already stored in
> + * reply.error */
> + error_report_err(local_err);
> + local_err = NULL;
> + }
So we still end up printing the error directly to stderr, the way LOG()
used to do; but in light of the recent CVEs on 'qemu-nbd -t' not being
robust to misbehaved clients, it does feel like the right thing (the
server detecting an error does not mean that the server should quit,
merely that it should log that a client is no longer connected).
Overall the change looks good to me, but I'll wait to see if Markus has
any design advice before giving my R-b.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Markus, what do you think? I'll resend tomorrow with Eric's comments
applied, should I change something with this errp?
29.06.2017 22:27, Eric Blake wrote:
> On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
[...]
>> -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, ...)
>> {
> Markus, this violates our usual idiom of errp last in code that is not
> directly operating on an Error (the actual Error implementation in
> error.c being the main obvious exception that lists errp first), but I
> don't see any better approach. Do you have any thoughts on it?
>
>> - if (nbd_read(client->ioc, name, length, NULL) < 0) {
>> - LOG("read failed");
>> + if (nbd_read(client->ioc, name, length, errp) < 0) {
>> + error_prepend(errp, "read failed: ");
>> return -EINVAL;
>> }
[...]
>
> Overall the change looks good to me, but I'll wait to see if Markus has
> any design advice before giving my R-b.
>
--
Best regards,
Vladimir
On 05/07/2017 14:33, Vladimir Sementsov-Ogievskiy wrote:
>
>
>>> -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, ...)
>>> {
>> Markus, this violates our usual idiom of errp last in code that is not
>> directly operating on an Error (the actual Error implementation in
>> error.c being the main obvious exception that lists errp first), but I
>> don't see any better approach. Do you have any thoughts on it?
I think it's okay as it matches functions like vfprintf.
Paolo
Eric Blake <eblake@redhat.com> writes:
> On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Move to modern errp scheme from just LOGging errors.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> nbd/server.c | 268 +++++++++++++++++++++++++++++++++++------------------------
>> 1 file changed, 161 insertions(+), 107 deletions(-)
>
> Unfortunately longer, but I'm okay with that (and we already did it
> client-side, so this is just a case of a patch series being spread out
> over time).
>
>
>>
>> /* 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, ...)
>> {
>
> Markus, this violates our usual idiom of errp last in code that is not
> directly operating on an Error (the actual Error implementation in
> error.c being the main obvious exception that lists errp first), but I
> don't see any better approach. Do you have any thoughts on it?
The convention to put Error **errp last comes from GLib's GError.
Here's what it has to say about combining GError with variable
arguments:
The last argument of a function that returns an error should be a
location where a GError can be placed (i.e. "GError** error"). If
GError is used with varargs, the GError** should be the last
argument before the "...".
https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html
Separating the format string from the variable arguments is ugly,
though. I can't find examples of this ugliness in /usr/include/glib. I
can find a few for Vladimir's solution:
* g_output_stream_printf() in gio/goutputstream.h: format string
followed by arguments
* g_initable_new() in gio/ginitable.h: key name followed by key value,
..., NULL
* g_subprocess_new() in gio/gsubprocess.h: argv0, ..., NULL
* g_subprocess_launcher_spawn() in gio/gsubprocesslauncher.h: argv0,
..., NULL
* g_markup_collect_attributes() in gmarkup.h: attribute name followed by
attribute value, ..., NULL
Let's follow this precedence. In short, Vladimir's patch is okay as is.
[...]
On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> nbd/server.c | 268 +++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 161 insertions(+), 107 deletions(-)
> -static coroutine_fn int nbd_negotiate(NBDClient *client)
> +static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
> {
> char buf[8 + 8 + 8 + 128];
> int ret;
Now that you have this,
> qio_channel_set_blocking(client->ioc, false, NULL);
>
you should probably pass errp through to qio_channel_set_blocking and
return early if that fails (either here or in one of the followup
patches) [hmm, I note that most callers of qio_channel_set_blocking()
ignore errors - outside of NBD, so that's a separate cleanup series]
> @@ -591,21 +617,23 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
>
> if (oldStyle) {
> if (client->tlscreds) {
> - TRACE("TLS cannot be enabled with oldstyle protocol");
> + error_setg(errp, "TLS cannot be enabled with oldstyle protocol");
> return -EINVAL;
This code is unreachable, since earlier we have:
> oldStyle = client->exp != NULL && !client->tlscreds;
You could just as easily delete the conditional. In fact, our code has:
if (oldStyle) {
do stuff
} else {
do stuff
}
if (oldStyle) {
do stuff
} else {
do stuff
}
and it would be simple to merge that into a single if statement.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
07.07.2017 17:40, Eric Blake wrote:
> On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Move to modern errp scheme from just LOGging errors.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> nbd/server.c | 268 +++++++++++++++++++++++++++++++++++------------------------
>> 1 file changed, 161 insertions(+), 107 deletions(-)
>> -static coroutine_fn int nbd_negotiate(NBDClient *client)
>> +static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>> {
>> char buf[8 + 8 + 8 + 128];
>> int ret;
> Now that you have this,
>
>> qio_channel_set_blocking(client->ioc, false, NULL);
>>
> you should probably pass errp through to qio_channel_set_blocking and
> return early if that fails (either here or in one of the followup
> patches) [hmm, I note that most callers of qio_channel_set_blocking()
> ignore errors - outside of NBD, so that's a separate cleanup series]
I've looked through realizations of ioc_klass->io_set_blocking - looks
like they all are returning zero always.
>
>
>> @@ -591,21 +617,23 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
>>
>> if (oldStyle) {
>> if (client->tlscreds) {
>> - TRACE("TLS cannot be enabled with oldstyle protocol");
>> + error_setg(errp, "TLS cannot be enabled with oldstyle protocol");
>> return -EINVAL;
> This code is unreachable, since earlier we have:
>
>> oldStyle = client->exp != NULL && !client->tlscreds;
> You could just as easily delete the conditional. In fact, our code has:
>
> if (oldStyle) {
> do stuff
> } else {
> do stuff
> }
>
> if (oldStyle) {
> do stuff
> } else {
> do stuff
> }
>
> and it would be simple to merge that into a single if statement.
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.