[Qemu-devel] [PATCH master+0.14 0/2] blockdev memory leaks

[Qemu-devel] [PATCH master+0.14 0/2] blockdev memory leaks

[Markus Armbruster]

Markus Armbruster (2):
  blockdev: Plug memory leak in drive_uninit()
  blockdev: Plug memory leak in drive_init() error paths

 blockdev.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH master+0.14 1/2] blockdev: Plug memory leak in drive_uninit()

[Markus Armbruster]

Started leaking in commit 1dae12e6.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ecfadc1..24d7658 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -182,6 +182,7 @@ static void drive_uninit(DriveInfo *dinfo)
 {
     qemu_opts_del(dinfo->opts);
     bdrv_delete(dinfo->bdrv);
+    qemu_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     qemu_free(dinfo);
 }
-- 
1.7.2.3

[Qemu-devel] [PATCH master+0.14 2/2] blockdev: Plug memory leak in drive_init() error paths

[Markus Armbruster]

Should have spotted this when doing commit 319ae529.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 24d7658..0690cc8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -526,7 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     } else if (ro == 1) {
         if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
             error_report("readonly not supported by this bus type");
-            return NULL;
+            goto err;
         }
     }
 
@@ -536,12 +536,19 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     if (ret < 0) {
         error_report("could not open disk image %s: %s",
                      file, strerror(-ret));
-        return NULL;
+        goto err;
     }
 
     if (bdrv_key_required(dinfo->bdrv))
         autostart = 0;
     return dinfo;
+
+err:
+    bdrv_delete(dinfo->bdrv);
+    qemu_free(dinfo->id);
+    QTAILQ_REMOVE(&drives, dinfo, next);
+    qemu_free(dinfo);
+    return NULL;
 }
 
 void do_commit(Monitor *mon, const QDict *qdict)
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH master+0.14 0/2] blockdev memory leaks

[Kevin Wolf]

Am 08.02.2011 15:12, schrieb Markus Armbruster:
> Markus Armbruster (2):
>   blockdev: Plug memory leak in drive_uninit()
>   blockdev: Plug memory leak in drive_init() error paths
> 
>  blockdev.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)

Thanks, applied all to the block branch.

Kevin

[Qemu-devel] Re: [PATCH master+0.14 0/2] blockdev memory leaks

[Kevin Wolf]

Hi Justin,

Am 08.02.2011 15:12, schrieb Markus Armbruster:
> Markus Armbruster (2):
>   blockdev: Plug memory leak in drive_uninit()
>   blockdev: Plug memory leak in drive_init() error paths
> 
>  blockdev.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)

How this series made its way into stable was a bit surprising for me.
You may not be aware yet of the expectations that I (and probably
others) have in the process of patches being applied to stable. No harm
done, but maybe something to consider for future patches, so let me just
mention some points:

I saw that you already merged these patches into the stable tree, even
though they are not master yet. I think usually stable should only get
cherry-picks from master. There are exceptions of course (e.g. when
something will be fixed differently in master), but I don't think this
is one of them.

Also I noticed that you didn't add your Signed-off-by when applying the
patches. As I understand it, you should do this for any patch that you
apply directly (i.e. that you don't get via a git pull)

I only caught this by chance. If you sent an email ("Thanks, applied to
...") after you have applied a patch or pulled from somewhere, it would
be more obvious to the rest of us what happens in stable.

Kevin

[Qemu-devel] Re: [PATCH master+0.14 0/2] blockdev memory leaks

[Justin M. Forbes]

On Wed, 2011-02-09 at 14:52 +0100, Kevin Wolf wrote:
> Hi Justin,
> 
> Am 08.02.2011 15:12, schrieb Markus Armbruster:
> > Markus Armbruster (2):
> >   blockdev: Plug memory leak in drive_uninit()
> >   blockdev: Plug memory leak in drive_init() error paths
> > 
> >  blockdev.c |   12 ++++++++++--
> >  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> How this series made its way into stable was a bit surprising for me.
> You may not be aware yet of the expectations that I (and probably
> others) have in the process of patches being applied to stable. No harm
> done, but maybe something to consider for future patches, so let me just
> mention some points:
> 
> I saw that you already merged these patches into the stable tree, even
> though they are not master yet. I think usually stable should only get
> cherry-picks from master. There are exceptions of course (e.g. when
> something will be fixed differently in master), but I don't think this
> is one of them.
> 
> Also I noticed that you didn't add your Signed-off-by when applying the
> patches. As I understand it, you should do this for any patch that you
> apply directly (i.e. that you don't get via a git pull)
> 
> I only caught this by chance. If you sent an email ("Thanks, applied to
> ...") after you have applied a patch or pulled from somewhere, it would
> be more obvious to the rest of us what happens in stable.

Indeed, that was my fault... I had applied them for testing, and pushed
to the wrong tree.  I have made some local changes to insure that this
does not happen in the future.  The process is, pathches should be in
master first, or their be a very good reason that they are not (bugs no
longer apply to master, or different and more invasive fix is used in
master).  At this point in the cycle, where there is so little
divergence from master, there is no reason that anything in stable is
not in master.

Justin

Re: [Qemu-devel] Re: [PATCH master+0.14 0/2] blockdev memory leaks

[Anthony Liguori]

On 02/09/2011 08:09 AM, Justin M. Forbes wrote:
> On Wed, 2011-02-09 at 14:52 +0100, Kevin Wolf wrote:
>    
>> Hi Justin,
>>
>> Am 08.02.2011 15:12, schrieb Markus Armbruster:
>>      
>>> Markus Armbruster (2):
>>>    blockdev: Plug memory leak in drive_uninit()
>>>    blockdev: Plug memory leak in drive_init() error paths
>>>
>>>   blockdev.c |   12 ++++++++++--
>>>   1 files changed, 10 insertions(+), 2 deletions(-)
>>>        
>> How this series made its way into stable was a bit surprising for me.
>> You may not be aware yet of the expectations that I (and probably
>> others) have in the process of patches being applied to stable. No harm
>> done, but maybe something to consider for future patches, so let me just
>> mention some points:
>>
>> I saw that you already merged these patches into the stable tree, even
>> though they are not master yet. I think usually stable should only get
>> cherry-picks from master. There are exceptions of course (e.g. when
>> something will be fixed differently in master), but I don't think this
>> is one of them.
>>
>> Also I noticed that you didn't add your Signed-off-by when applying the
>> patches. As I understand it, you should do this for any patch that you
>> apply directly (i.e. that you don't get via a git pull)
>>
>> I only caught this by chance. If you sent an email ("Thanks, applied to
>> ...") after you have applied a patch or pulled from somewhere, it would
>> be more obvious to the rest of us what happens in stable.
>>      
> Indeed, that was my fault...

No worries, it happens.

Regards,

Anthony Liguori

>   I had applied them for testing, and pushed
> to the wrong tree.  I have made some local changes to insure that this
> does not happen in the future.  The process is, pathches should be in
> master first, or their be a very good reason that they are not (bugs no
> longer apply to master, or different and more invasive fix is used in
> master).  At this point in the cycle, where there is so little
> divergence from master, there is no reason that anything in stable is
> not in master.
>
> Justin
>
>
>    

[Qemu-devel] Re: [PATCH master+0.14 0/2] blockdev memory leaks

[Kevin Wolf]

Am 09.02.2011 15:09, schrieb Justin M. Forbes:
> On Wed, 2011-02-09 at 14:52 +0100, Kevin Wolf wrote:
>> Hi Justin,
>>
>> Am 08.02.2011 15:12, schrieb Markus Armbruster:
>>> Markus Armbruster (2):
>>>   blockdev: Plug memory leak in drive_uninit()
>>>   blockdev: Plug memory leak in drive_init() error paths
>>>
>>>  blockdev.c |   12 ++++++++++--
>>>  1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> How this series made its way into stable was a bit surprising for me.
>> You may not be aware yet of the expectations that I (and probably
>> others) have in the process of patches being applied to stable. No harm
>> done, but maybe something to consider for future patches, so let me just
>> mention some points:
>>
>> I saw that you already merged these patches into the stable tree, even
>> though they are not master yet. I think usually stable should only get
>> cherry-picks from master. There are exceptions of course (e.g. when
>> something will be fixed differently in master), but I don't think this
>> is one of them.
>>
>> Also I noticed that you didn't add your Signed-off-by when applying the
>> patches. As I understand it, you should do this for any patch that you
>> apply directly (i.e. that you don't get via a git pull)
>>
>> I only caught this by chance. If you sent an email ("Thanks, applied to
>> ...") after you have applied a patch or pulled from somewhere, it would
>> be more obvious to the rest of us what happens in stable.
> 
> Indeed, that was my fault... I had applied them for testing, and pushed
> to the wrong tree.  I have made some local changes to insure that this
> does not happen in the future. 

Okay, if it was an accident, no problem.

I was just trying to make sure that we're all having the same
expectation of how it should work.

Kevin