| Message ID | 20180507154458.73945-6-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD reconnect: preliminary refactoring | expand |
On 05/07/2018 10:44 AM, Vladimir Sementsov-Ogievskiy wrote: > We have several paranoiac checks for ioc != NULL. But ioc may become > NULL only on close, which should not happen during requests handling. > Also, we check ioc only sometimes, not after each yield, which is > inconsistent. Let's drop these checks. Can (or even should) any of these be replaced by asserts that ioc is not NULL? > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/nbd-client.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-)
07.05.2018 21:08, Eric Blake wrote: > On 05/07/2018 10:44 AM, Vladimir Sementsov-Ogievskiy wrote: >> We have several paranoiac checks for ioc != NULL. But ioc may become >> NULL only on close, which should not happen during requests handling. >> Also, we check ioc only sometimes, not after each yield, which is >> inconsistent. Let's drop these checks. > > Can (or even should) any of these be replaced by asserts that ioc is > not NULL? > >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/nbd-client.c | 14 +------------- >> 1 file changed, 1 insertion(+), 13 deletions(-) > No problem, I can add them. Actually in most of cases we will crash very soon on next QIO_CHANNEL_GET_CLASS(ioc). The exclusions (looked through) are: - "if (!s->ioc || s->quit) {" case, if reply is not simple. - zero-length io requests in other cases, if they are possible
diff --git a/block/nbd-client.c b/block/nbd-client.c index dd712c59b3..87d90d9026 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -51,10 +51,6 @@ static void nbd_teardown_connection(BlockDriverState *bs) { NBDClientSession *client = nbd_get_client_session(bs); - if (!client->ioc) { /* Already closed */ - return; - } - /* finish any pending coroutines */ qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, @@ -150,10 +146,6 @@ static int nbd_co_send_request(BlockDriverState *bs, rc = -EIO; goto err; } - if (!s->ioc) { - rc = -EPIPE; - goto err; - } if (qiov) { qio_channel_set_cork(s->ioc, true); @@ -426,7 +418,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( s->requests[i].receiving = true; qemu_coroutine_yield(); s->requests[i].receiving = false; - if (!s->ioc || s->quit) { + if (s->quit) { error_setg(errp, "Connection closed"); return -EIO; } @@ -967,10 +959,6 @@ void nbd_client_close(BlockDriverState *bs) NBDClientSession *client = nbd_get_client_session(bs); NBDRequest request = { .type = NBD_CMD_DISC }; - if (client->ioc == NULL) { - return; - } - nbd_send_request(client->ioc, &request); nbd_teardown_connection(bs);
We have several paranoiac checks for ioc != NULL. But ioc may become NULL only on close, which should not happen during requests handling. Also, we check ioc only sometimes, not after each yield, which is inconsistent. Let's drop these checks. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd-client.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)