On 11/29/19 1:25 AM, pannengy...@huawei.com wrote:
From: PanNengyuan <pannengy...@huawei.com>
The BDRVNBDState cleanup code is common in two places, add
nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
Stefano Garzarella).
Signed-off-by: PanNengyuan <pannengy...@huawei.com>
---
block/nbd.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 1239761..5805979 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
static int nbd_client_connect(BlockDriverState *bs, Error **errp);
+static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
+
Why do you need a static function prototype? Just implement the
function prior to its first use, then you won't need a forward declaration.
static void nbd_channel_error(BDRVNBDState *s, int ret)
{
if (ret == -EIO) {
@@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs,
Error **errp)
}
}
+static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
+{
+ object_unref(OBJECT(s->tlscreds));
+ qapi_free_SocketAddress(s->saddr);
+ g_free(s->export);
+ g_free(s->tlscredsid);
+ g_free(s->x_dirty_bitmap);
+}
In fact, it appears that you did just that, as the first use...
Bike-shedding: the name 'nbd_free_bdrvstate_prop' doesn't seem right to
me - when I see a function with 'free' in the name taking a single
pointer, I assume that the given pointer (here, BDRVNBDState *s) is
freed - but your function does NOT free then incoming pointer. Rather,
you are clearing out the contents within a pre-allocated object which
remains allocated. What's more, since the object remains allocated, I'm
surprised that you are not setting fields to NULL to prevent
use-after-free bugs.
Either this function should also free s (in which case naming it merely
'nbd_free_bdrvstate' might be better), or you should consider naming it
'nbd_clear_bdrvstate' and assigning cleared fields to NULL.
+
/*
* Parse nbd_open options
*/
@@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs,
QDict *options,
error:
if (ret < 0) {
- object_unref(OBJECT(s->tlscreds));
- qapi_free_SocketAddress(s->saddr);
- g_free(s->export);
- g_free(s->tlscredsid);
+ nbd_free_bdrvstate_prop(s);
...is here.
}
qemu_opts_del(opts);
return ret;
@@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
BDRVNBDState *s = bs->opaque;
nbd_client_close(bs);
-
- object_unref(OBJECT(s->tlscreds));
- qapi_free_SocketAddress(s->saddr);
- g_free(s->export);
- g_free(s->tlscredsid);
- g_free(s->x_dirty_bitmap);
+ nbd_free_bdrvstate_prop(s);
}
static int64_t nbd_getlength(BlockDriverState *bs)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
