| Message ID | 1478080120-3277-1-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 02.11.2016 um 10:48 hat Ashijeet Acharya geschrieben: > This patch frees the leaked visitor in nbd_refresh_filename() and uses > visit_free() to fix it. The leak was introduced by the commit 491d6c7. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > Reviewed-by: Eric Blake <eblake@redhat.com> I don't think this would generally be called a regression, so I'd change the subject to just "nbd: Fix leaker visitor". > Changes in v2: > - Include the regression commit id in the commit message > --- > block/nbd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/nbd.c b/block/nbd.c > index 8ef1438..ff9d01a 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) > qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid)); > } > > + visit_free(ov); > qdict_flatten(opts); > bs->full_open_options = opts; > } I would prefer freeing the visitor immediately after visit_complete() so that everything visitor related is in a single place. Both of these points don't make your patch wrong, of course, but would you mind changing them? Kevin
On Wed, Nov 2, 2016 at 4:00 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 02.11.2016 um 10:48 hat Ashijeet Acharya geschrieben: >> This patch frees the leaked visitor in nbd_refresh_filename() and uses >> visit_free() to fix it. The leak was introduced by the commit 491d6c7. >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > I don't think this would generally be called a regression, so I'd change > the subject to just "nbd: Fix leaker visitor". > >> Changes in v2: >> - Include the regression commit id in the commit message >> --- >> block/nbd.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index 8ef1438..ff9d01a 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) >> qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid)); >> } >> >> + visit_free(ov); >> qdict_flatten(opts); >> bs->full_open_options = opts; >> } > > I would prefer freeing the visitor immediately after visit_complete() so > that everything visitor related is in a single place. > > Both of these points don't make your patch wrong, of course, but would > you mind changing them? Sure, I will send v3 straight away. Ashijeet > Kevin
diff --git a/block/nbd.c b/block/nbd.c index 8ef1438..ff9d01a 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid)); } + visit_free(ov); qdict_flatten(opts); bs->full_open_options = opts; }