| Message ID | 1575012326-51324-1-git-send-email-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3,1/2] block/nbd: extract the common cleanup code | expand |
Hi! First, please, when sending more than one patch, create a cover-letter. Also, summarize (in cover letter) what was changed since previous version. 29.11.2019 10:25, pannengyuan@huawei.com wrote: > From: PanNengyuan <pannengyuan@huawei.com> Strange line. Check you git preferences. Such line appears (and make sense) when you are sending patches authored by someone else.. But here is your name, the same as in email's From:. > > 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 <pannengyuan@huawei.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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); > + > 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); > +} > + > /* > * 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); > } > 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) >
On 11/29/19 1:25 AM, pannengyuan@huawei.com wrote: > From: PanNengyuan <pannengyuan@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 <pannengyuan@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) >
On 2019/12/4 1:38, Vladimir Sementsov-Ogievskiy wrote: > Hi! > > First, please, when sending more than one patch, create a cover-letter. Also, > summarize (in cover letter) what was changed since previous version. In previous version, I only send one patch(2/2 in this version), so I only add a change summarize in 2/2 patch in this version. should I add a summarize in 1/2 patch too if 1/2 patch is a new one? > > 29.11.2019 10:25, pannengyuan@huawei.com wrote: >> From: PanNengyuan <pannengyuan@huawei.com> > > Strange line. Check you git preferences. Such line appears (and make sense) > when you are sending patches authored by someone else.. But here is your name, > the same as in email's From:. Thanks for your reminding. I will correct it in next version. > >> >> 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 <pannengyuan@huawei.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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); >> + >> 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); >> +} >> + >> /* >> * 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); >> } >> 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) >> > >
On 2019/12/4 3:00, Eric Blake wrote: > On 11/29/19 1:25 AM, pannengyuan@huawei.com wrote: >> From: PanNengyuan <pannengyuan@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 <pannengyuan@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. Yes, It's not necessary. I will change it. > >> 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. > thanks, 'nbd_clear_bdrvstate' seems nice. I will replace the name and set fields to NULL in next version. >> + >> /* >> * 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) >> >
04.12.2019 6:12, pannengyuan wrote: > > > On 2019/12/4 1:38, Vladimir Sementsov-Ogievskiy wrote: >> Hi! >> >> First, please, when sending more than one patch, create a cover-letter. Also, >> summarize (in cover letter) what was changed since previous version. > In previous version, I only send one patch(2/2 in this version), so I > only add a change summarize in 2/2 patch in this version. should I add a > summarize in 1/2 patch too if 1/2 patch is a new one? Yes, something simple like: 01: new patch 02: rebased on 01 (also, If you didn't read https://wiki.qemu.org/Contribute/SubmitAPatch, do it) > >> >> 29.11.2019 10:25, pannengyuan@huawei.com wrote: >>> From: PanNengyuan <pannengyuan@huawei.com> >> >> Strange line. Check you git preferences. Such line appears (and make sense) >> when you are sending patches authored by someone else.. But here is your name, >> the same as in email's From:. > > Thanks for your reminding. I will correct it in next version. > >> >>> >>> 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 <pannengyuan@huawei.com> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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); >>> + >>> 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); >>> +} >>> + >>> /* >>> * 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); >>> } >>> 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) >>> >> >> >
On 2019/12/4 15:19, Vladimir Sementsov-Ogievskiy wrote: > 04.12.2019 6:12, pannengyuan wrote: >> >> >> On 2019/12/4 1:38, Vladimir Sementsov-Ogievskiy wrote: >>> Hi! >>> >>> First, please, when sending more than one patch, create a cover-letter. Also, >>> summarize (in cover letter) what was changed since previous version. >> In previous version, I only send one patch(2/2 in this version), so I >> only add a change summarize in 2/2 patch in this version. should I add a >> summarize in 1/2 patch too if 1/2 patch is a new one? > > Yes, something simple like: > > 01: new patch > 02: rebased on 01 > > (also, If you didn't read https://wiki.qemu.org/Contribute/SubmitAPatch, do it) Ok, thanks. > >> >>> >>> 29.11.2019 10:25, pannengyuan@huawei.com wrote: >>>> From: PanNengyuan <pannengyuan@huawei.com> >>> >>> Strange line. Check you git preferences. Such line appears (and make sense) >>> when you are sending patches authored by someone else.. But here is your name, >>> the same as in email's From:. >> >> Thanks for your reminding. I will correct it in next version. >> >>> >>>> >>>> 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 <pannengyuan@huawei.com> >>> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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); >>>> + >>>> 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); >>>> +} >>>> + >>>> /* >>>> * 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); >>>> } >>>> 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) >>>> >>> >>> >> > >
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); + 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); +} + /* * 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); } 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)