From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52278) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIYyr-0004RM-5c for qemu-devel@nongnu.org; Tue, 03 Feb 2015 03:37:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIYyo-0003Ar-2w for qemu-devel@nongnu.org; Tue, 03 Feb 2015 03:37:08 -0500 Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:41190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIYyn-0003Aj-JU for qemu-devel@nongnu.org; Tue, 03 Feb 2015 03:37:05 -0500 Received: by mail-wi0-f175.google.com with SMTP id fb4so22412643wid.2 for ; Tue, 03 Feb 2015 00:37:05 -0800 (PST) Sender: Paolo Bonzini Message-ID: <54D088AC.8030607@redhat.com> Date: Tue, 03 Feb 2015 09:37:00 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1422913238-7280-1-git-send-email-mreitz@redhat.com> <1422913238-7280-2-git-send-email-mreitz@redhat.com> In-Reply-To: <1422913238-7280-2-git-send-email-mreitz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] nbd: Drop BDS backpointer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 02/02/2015 22:40, Max Reitz wrote: > Before this patch, the "opaque" pointer in an NBD BDS points to a > BDRVNBDState, which contains an NbdClientSession object, which in turn > contains a pointer to the BDS. This pointer may become invalid due to > bdrv_swap(), so drop it, and instead pass the BDS directly to the > nbd-client.c functions which then retrieve the NbdClientSession object > from there. Looks good, but please change function names from nbd_client_session_foo to nbd_client_foo or even just nbd_foo if they do not take an NbdClientSession* as the first parameter. Thanks, Paolo > Signed-off-by: Max Reitz > --- > block/nbd-client.c | 95 ++++++++++++++++++++++++++++-------------------------- > block/nbd-client.h | 20 ++++++------ > block/nbd.c | 37 ++++++++------------- > 3 files changed, 73 insertions(+), 79 deletions(-) > > diff --git a/block/nbd-client.c b/block/nbd-client.c > index 28bfb62..4ede714 100644 > --- a/block/nbd-client.c > +++ b/block/nbd-client.c > @@ -43,20 +43,23 @@ static void nbd_recv_coroutines_enter_all(NbdClientSession *s) > } > } > > -static void nbd_teardown_connection(NbdClientSession *client) > +static void nbd_teardown_connection(BlockDriverState *bs) > { > + NbdClientSession *client = nbd_get_client_session(bs); > + > /* finish any pending coroutines */ > shutdown(client->sock, 2); > nbd_recv_coroutines_enter_all(client); > > - nbd_client_session_detach_aio_context(client); > + nbd_client_session_detach_aio_context(bs); > closesocket(client->sock); > client->sock = -1; > } > > static void nbd_reply_ready(void *opaque) > { > - NbdClientSession *s = opaque; > + BlockDriverState *bs = opaque; > + NbdClientSession *s = nbd_get_client_session(bs); > uint64_t i; > int ret; > > @@ -89,28 +92,29 @@ static void nbd_reply_ready(void *opaque) > } > > fail: > - nbd_teardown_connection(s); > + nbd_teardown_connection(bs); > } > > static void nbd_restart_write(void *opaque) > { > - NbdClientSession *s = opaque; > + BlockDriverState *bs = opaque; > > - qemu_coroutine_enter(s->send_coroutine, NULL); > + qemu_coroutine_enter(nbd_get_client_session(bs)->send_coroutine, NULL); > } > > -static int nbd_co_send_request(NbdClientSession *s, > - struct nbd_request *request, > - QEMUIOVector *qiov, int offset) > +static int nbd_co_send_request(BlockDriverState *bs, > + struct nbd_request *request, > + QEMUIOVector *qiov, int offset) > { > + NbdClientSession *s = nbd_get_client_session(bs); > AioContext *aio_context; > int rc, ret; > > qemu_co_mutex_lock(&s->send_mutex); > s->send_coroutine = qemu_coroutine_self(); > - aio_context = bdrv_get_aio_context(s->bs); > + aio_context = bdrv_get_aio_context(bs); > aio_set_fd_handler(aio_context, s->sock, > - nbd_reply_ready, nbd_restart_write, s); > + nbd_reply_ready, nbd_restart_write, bs); > if (qiov) { > if (!s->is_unix) { > socket_set_cork(s->sock, 1); > @@ -129,7 +133,7 @@ static int nbd_co_send_request(NbdClientSession *s, > } else { > rc = nbd_send_request(s->sock, request); > } > - aio_set_fd_handler(aio_context, s->sock, nbd_reply_ready, NULL, s); > + aio_set_fd_handler(aio_context, s->sock, nbd_reply_ready, NULL, bs); > s->send_coroutine = NULL; > qemu_co_mutex_unlock(&s->send_mutex); > return rc; > @@ -195,10 +199,11 @@ static void nbd_coroutine_end(NbdClientSession *s, > } > } > > -static int nbd_co_readv_1(NbdClientSession *client, int64_t sector_num, > +static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov, > int offset) > { > + NbdClientSession *client = nbd_get_client_session(bs); > struct nbd_request request = { .type = NBD_CMD_READ }; > struct nbd_reply reply; > ssize_t ret; > @@ -207,7 +212,7 @@ static int nbd_co_readv_1(NbdClientSession *client, int64_t sector_num, > request.len = nb_sectors * 512; > > nbd_coroutine_start(client, &request); > - ret = nbd_co_send_request(client, &request, NULL, 0); > + ret = nbd_co_send_request(bs, &request, NULL, 0); > if (ret < 0) { > reply.error = -ret; > } else { > @@ -218,15 +223,16 @@ static int nbd_co_readv_1(NbdClientSession *client, int64_t sector_num, > > } > > -static int nbd_co_writev_1(NbdClientSession *client, int64_t sector_num, > +static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov, > int offset) > { > + NbdClientSession *client = nbd_get_client_session(bs); > struct nbd_request request = { .type = NBD_CMD_WRITE }; > struct nbd_reply reply; > ssize_t ret; > > - if (!bdrv_enable_write_cache(client->bs) && > + if (!bdrv_enable_write_cache(bs) && > (client->nbdflags & NBD_FLAG_SEND_FUA)) { > request.type |= NBD_CMD_FLAG_FUA; > } > @@ -235,7 +241,7 @@ static int nbd_co_writev_1(NbdClientSession *client, int64_t sector_num, > request.len = nb_sectors * 512; > > nbd_coroutine_start(client, &request); > - ret = nbd_co_send_request(client, &request, qiov, offset); > + ret = nbd_co_send_request(bs, &request, qiov, offset); > if (ret < 0) { > reply.error = -ret; > } else { > @@ -249,14 +255,13 @@ static int nbd_co_writev_1(NbdClientSession *client, int64_t sector_num, > * remain aligned to 4K. */ > #define NBD_MAX_SECTORS 2040 > > -int nbd_client_session_co_readv(NbdClientSession *client, int64_t sector_num, > - int nb_sectors, QEMUIOVector *qiov) > +int nbd_client_session_co_readv(BlockDriverState *bs, int64_t sector_num, > + int nb_sectors, QEMUIOVector *qiov) > { > int offset = 0; > int ret; > while (nb_sectors > NBD_MAX_SECTORS) { > - ret = nbd_co_readv_1(client, sector_num, > - NBD_MAX_SECTORS, qiov, offset); > + ret = nbd_co_readv_1(bs, sector_num, NBD_MAX_SECTORS, qiov, offset); > if (ret < 0) { > return ret; > } > @@ -264,17 +269,16 @@ int nbd_client_session_co_readv(NbdClientSession *client, int64_t sector_num, > sector_num += NBD_MAX_SECTORS; > nb_sectors -= NBD_MAX_SECTORS; > } > - return nbd_co_readv_1(client, sector_num, nb_sectors, qiov, offset); > + return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset); > } > > -int nbd_client_session_co_writev(NbdClientSession *client, int64_t sector_num, > +int nbd_client_session_co_writev(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > int offset = 0; > int ret; > while (nb_sectors > NBD_MAX_SECTORS) { > - ret = nbd_co_writev_1(client, sector_num, > - NBD_MAX_SECTORS, qiov, offset); > + ret = nbd_co_writev_1(bs, sector_num, NBD_MAX_SECTORS, qiov, offset); > if (ret < 0) { > return ret; > } > @@ -282,11 +286,12 @@ int nbd_client_session_co_writev(NbdClientSession *client, int64_t sector_num, > sector_num += NBD_MAX_SECTORS; > nb_sectors -= NBD_MAX_SECTORS; > } > - return nbd_co_writev_1(client, sector_num, nb_sectors, qiov, offset); > + return nbd_co_writev_1(bs, sector_num, nb_sectors, qiov, offset); > } > > -int nbd_client_session_co_flush(NbdClientSession *client) > +int nbd_client_session_co_flush(BlockDriverState *bs) > { > + NbdClientSession *client = nbd_get_client_session(bs); > struct nbd_request request = { .type = NBD_CMD_FLUSH }; > struct nbd_reply reply; > ssize_t ret; > @@ -303,7 +308,7 @@ int nbd_client_session_co_flush(NbdClientSession *client) > request.len = 0; > > nbd_coroutine_start(client, &request); > - ret = nbd_co_send_request(client, &request, NULL, 0); > + ret = nbd_co_send_request(bs, &request, NULL, 0); > if (ret < 0) { > reply.error = -ret; > } else { > @@ -313,9 +318,10 @@ int nbd_client_session_co_flush(NbdClientSession *client) > return -reply.error; > } > > -int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num, > - int nb_sectors) > +int nbd_client_session_co_discard(BlockDriverState *bs, int64_t sector_num, > + int nb_sectors) > { > + NbdClientSession *client = nbd_get_client_session(bs); > struct nbd_request request = { .type = NBD_CMD_TRIM }; > struct nbd_reply reply; > ssize_t ret; > @@ -327,7 +333,7 @@ int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num, > request.len = nb_sectors * 512; > > nbd_coroutine_start(client, &request); > - ret = nbd_co_send_request(client, &request, NULL, 0); > + ret = nbd_co_send_request(bs, &request, NULL, 0); > if (ret < 0) { > reply.error = -ret; > } else { > @@ -338,43 +344,41 @@ int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num, > > } > > -void nbd_client_session_detach_aio_context(NbdClientSession *client) > +void nbd_client_session_detach_aio_context(BlockDriverState *bs) > { > - aio_set_fd_handler(bdrv_get_aio_context(client->bs), client->sock, > - NULL, NULL, NULL); > + aio_set_fd_handler(bdrv_get_aio_context(bs), > + nbd_get_client_session(bs)->sock, NULL, NULL, NULL); > } > > -void nbd_client_session_attach_aio_context(NbdClientSession *client, > +void nbd_client_session_attach_aio_context(BlockDriverState *bs, > AioContext *new_context) > { > - aio_set_fd_handler(new_context, client->sock, > - nbd_reply_ready, NULL, client); > + aio_set_fd_handler(new_context, nbd_get_client_session(bs)->sock, > + nbd_reply_ready, NULL, bs); > } > > -void nbd_client_session_close(NbdClientSession *client) > +void nbd_client_session_close(BlockDriverState *bs) > { > + NbdClientSession *client = nbd_get_client_session(bs); > struct nbd_request request = { > .type = NBD_CMD_DISC, > .from = 0, > .len = 0 > }; > > - if (!client->bs) { > - return; > - } > if (client->sock == -1) { > return; > } > > nbd_send_request(client->sock, &request); > > - nbd_teardown_connection(client); > - client->bs = NULL; > + nbd_teardown_connection(bs); > } > > -int nbd_client_session_init(NbdClientSession *client, BlockDriverState *bs, > +int nbd_client_session_init(BlockDriverState *bs, > int sock, const char *export, Error **errp) > { > + NbdClientSession *client = nbd_get_client_session(bs); > int ret; > > /* NBD handshake */ > @@ -391,13 +395,12 @@ int nbd_client_session_init(NbdClientSession *client, BlockDriverState *bs, > > qemu_co_mutex_init(&client->send_mutex); > qemu_co_mutex_init(&client->free_sema); > - client->bs = bs; > client->sock = sock; > > /* Now that we're connected, set the socket to be non-blocking and > * kick the reply mechanism. */ > qemu_set_nonblock(sock); > - nbd_client_session_attach_aio_context(client, bdrv_get_aio_context(bs)); > + nbd_client_session_attach_aio_context(bs, bdrv_get_aio_context(bs)); > > logout("Established connection with NBD server\n"); > return 0; > diff --git a/block/nbd-client.h b/block/nbd-client.h > index cfeecc2..49daccc 100644 > --- a/block/nbd-client.h > +++ b/block/nbd-client.h > @@ -31,24 +31,24 @@ typedef struct NbdClientSession { > struct nbd_reply reply; > > bool is_unix; > - > - BlockDriverState *bs; > } NbdClientSession; > > -int nbd_client_session_init(NbdClientSession *client, BlockDriverState *bs, > +NbdClientSession *nbd_get_client_session(BlockDriverState *bs); > + > +int nbd_client_session_init(BlockDriverState *bs, > int sock, const char *export_name, Error **errp); > -void nbd_client_session_close(NbdClientSession *client); > +void nbd_client_session_close(BlockDriverState *bs); > > -int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num, > +int nbd_client_session_co_discard(BlockDriverState *bs, int64_t sector_num, > int nb_sectors); > -int nbd_client_session_co_flush(NbdClientSession *client); > -int nbd_client_session_co_writev(NbdClientSession *client, int64_t sector_num, > +int nbd_client_session_co_flush(BlockDriverState *bs); > +int nbd_client_session_co_writev(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov); > -int nbd_client_session_co_readv(NbdClientSession *client, int64_t sector_num, > +int nbd_client_session_co_readv(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov); > > -void nbd_client_session_detach_aio_context(NbdClientSession *client); > -void nbd_client_session_attach_aio_context(NbdClientSession *client, > +void nbd_client_session_detach_aio_context(BlockDriverState *bs); > +void nbd_client_session_attach_aio_context(BlockDriverState *bs, > AioContext *new_context); > > #endif /* NBD_CLIENT_H */ > diff --git a/block/nbd.c b/block/nbd.c > index 2e20831..f71fba0 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -224,6 +224,12 @@ static void nbd_config(BDRVNBDState *s, QDict *options, char **export, > } > } > > +NbdClientSession *nbd_get_client_session(BlockDriverState *bs) > +{ > + BDRVNBDState *s = bs->opaque; > + return &s->client; > +} > + > static int nbd_establish_connection(BlockDriverState *bs, Error **errp) > { > BDRVNBDState *s = bs->opaque; > @@ -271,7 +277,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, > } > > /* NBD handshake */ > - result = nbd_client_session_init(&s->client, bs, sock, export, errp); > + result = nbd_client_session_init(bs, sock, export, errp); > g_free(export); > return result; > } > @@ -279,35 +285,24 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, > static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > - BDRVNBDState *s = bs->opaque; > - > - return nbd_client_session_co_readv(&s->client, sector_num, > - nb_sectors, qiov); > + return nbd_client_session_co_readv(bs, sector_num, nb_sectors, qiov); > } > > static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > - BDRVNBDState *s = bs->opaque; > - > - return nbd_client_session_co_writev(&s->client, sector_num, > - nb_sectors, qiov); > + return nbd_client_session_co_writev(bs, sector_num, nb_sectors, qiov); > } > > static int nbd_co_flush(BlockDriverState *bs) > { > - BDRVNBDState *s = bs->opaque; > - > - return nbd_client_session_co_flush(&s->client); > + return nbd_client_session_co_flush(bs); > } > > static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num, > int nb_sectors) > { > - BDRVNBDState *s = bs->opaque; > - > - return nbd_client_session_co_discard(&s->client, sector_num, > - nb_sectors); > + return nbd_client_session_co_discard(bs, sector_num, nb_sectors); > } > > static void nbd_close(BlockDriverState *bs) > @@ -315,7 +310,7 @@ static void nbd_close(BlockDriverState *bs) > BDRVNBDState *s = bs->opaque; > > qemu_opts_del(s->socket_opts); > - nbd_client_session_close(&s->client); > + nbd_client_session_close(bs); > } > > static int64_t nbd_getlength(BlockDriverState *bs) > @@ -327,17 +322,13 @@ static int64_t nbd_getlength(BlockDriverState *bs) > > static void nbd_detach_aio_context(BlockDriverState *bs) > { > - BDRVNBDState *s = bs->opaque; > - > - nbd_client_session_detach_aio_context(&s->client); > + nbd_client_session_detach_aio_context(bs); > } > > static void nbd_attach_aio_context(BlockDriverState *bs, > AioContext *new_context) > { > - BDRVNBDState *s = bs->opaque; > - > - nbd_client_session_attach_aio_context(&s->client, new_context); > + nbd_client_session_attach_aio_context(bs, new_context); > } > > static void nbd_refresh_filename(BlockDriverState *bs) >