[Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Chen Gang]

In dump_init(), when failure occurs, need notice about 'fd' and memory
mapping. So call dump_cleanup() for it (need let all initializations at
front).

Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
checking.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 dump.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/dump.c b/dump.c
index ce646bc..71d3e94 100644
--- a/dump.c
+++ b/dump.c
@@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
 
 static int dump_cleanup(DumpState *s)
 {
-    int ret = 0;
-
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
-    if (s->fd != -1) {
-        close(s->fd);
-    }
+    close(s->fd);
     if (s->resume) {
         vm_start();
     }
 
-    return ret;
+    return 0;
 }
 
 static void dump_error(DumpState *s, const char *reason)
@@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
     s->begin = begin;
     s->length = length;
 
+    memory_mapping_list_init(&s->list);
+
     guest_phys_blocks_init(&s->guest_phys_blocks);
     guest_phys_blocks_append(&s->guest_phys_blocks);
 
@@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
     }
 
     /* get memory mapping */
-    memory_mapping_list_init(&s->list);
     if (paging) {
         qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
         if (err != NULL) {
@@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
     return 0;
 
 cleanup:
-    guest_phys_blocks_free(&s->guest_phys_blocks);
-
-    if (s->resume) {
-        vm_start();
-    }
-
+    dump_cleanup(s);
     return -1;
 }
 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Laszlo Ersek]

comments below

On 08/03/14 17:28, Chen Gang wrote:
> In dump_init(), when failure occurs, need notice about 'fd' and memory
> mapping. So call dump_cleanup() for it (need let all initializations at
> front).
> 
> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
> checking.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  dump.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)

Please explain what is leaked and how.

The only possibility I can see (without digging very hard) is that
qemu_get_guest_memory_mapping() succeeds and lzo_init() fails (which
should never happen in practice).

Regarding s->fd itself, I'm beyond trying to understand its lifecycle.
Qemu uses a bad ownership model wherein functions, in case of an
internal error, release resources they got from their callers. I'm
unable to reason in such a model. The only function to close fd *ever*
should be qmp_dump_guest_memory() (and that one should close fd with a
direct close() call). Currently fd is basically a global variable,
because the entire dump function tree has access to it (and closes it if
there's an error).

Anyway I guess it's OK to call dump_cleanup() to close s->fd just in case.

If you have a Coverity report, please share it.

Then,

> diff --git a/dump.c b/dump.c
> index ce646bc..71d3e94 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  
>  static int dump_cleanup(DumpState *s)
>  {
> -    int ret = 0;
> -

I agree with this change.

>      guest_phys_blocks_free(&s->guest_phys_blocks);
>      memory_mapping_list_free(&s->list);
> -    if (s->fd != -1) {
> -        close(s->fd);
> -    }
> +    close(s->fd);

I disagree. It clobbers errno if s->fd is -1. Even though we don't
particularly care about errno, it sort of disturbs be. Or can you prove
s->fd is never -1 here?

>      if (s->resume) {
>          vm_start();
>      }
>  
> -    return ret;
> +    return 0;
>  }
>  
>  static void dump_error(DumpState *s, const char *reason)
> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      s->begin = begin;
>      s->length = length;
>  
> +    memory_mapping_list_init(&s->list);
> +
>      guest_phys_blocks_init(&s->guest_phys_blocks);
>      guest_phys_blocks_append(&s->guest_phys_blocks);
>  
> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      }
>  
>      /* get memory mapping */
> -    memory_mapping_list_init(&s->list);
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>          if (err != NULL) {
> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      return 0;
>  
>  cleanup:
> -    guest_phys_blocks_free(&s->guest_phys_blocks);
> -
> -    if (s->resume) {
> -        vm_start();
> -    }
> -
> +    dump_cleanup(s);
>      return -1;
>  }
>  
> 

This code is ripe for a generic lifecycle tracking overhaul, but since
my view of ownership tracking is marginal in the qemu developer
community, I'm not motivated.

NB: I'm not nacking your patch, just please explain it better.

Thanks
Laszlo

[Qemu-devel] Cc'ing emails [was: [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()]

[Michael Tokarev]

Please stop Cc'ing me emails sent to, at least, qemu-trivial@.

I'm about to filter personal emails which are also sent to
some mailinglists I receive.  I'd not do that, because this is
a good practice to Cc someone like that for various really
important or urgent emails, and if I to apply such a filter,
these urgent emails will be filtered too, obviously.

I'm not sure how people treat these cases or deal with them.
We are subscribed to, in particular, qemu-devel@, and active
maintainers look there too, so receive more than one copy of
many emails.

It is becoming worse.  With get_maintainer.pl pulling addresses
of people who made changes or commits to files by default,
contributing to the project becomes a bit dangerous.  Because
as a result, once you fix something, you're essentially being
subscribed to a spam list, because other contributors start
Ccing you for the patches with which you have absolutely nothing
to do, and if a discussion emerges, you can't opt out of it
anymore (especially for patches which raise hot discussions).
So I'd rather think twice before contributing anything...

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Chen Gang]

On 08/03/2014 11:56 PM, Laszlo Ersek wrote:
> comments below
> 

Excuse me for replying late, firstly.

> On 08/03/14 17:28, Chen Gang wrote:
>> In dump_init(), when failure occurs, need notice about 'fd' and memory
>> mapping. So call dump_cleanup() for it (need let all initializations at
>> front).
>>
>> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
>> checking.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  dump.c | 18 +++++-------------
>>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> Please explain what is leaked and how.
> 

Oh, sorry for the title misleading, need change to "Fix resource leak"
instead of "Fix memory leak".

> The only possibility I can see (without digging very hard) is that
> qemu_get_guest_memory_mapping() succeeds and lzo_init() fails (which
> should never happen in practice).
>

Yeah, what you said sounds reasonable to me.
 
> Regarding s->fd itself, I'm beyond trying to understand its lifecycle.
> Qemu uses a bad ownership model wherein functions, in case of an
> internal error, release resources they got from their callers. I'm
> unable to reason in such a model.

Yeah, what you said sounds reasonable to me.

>                                   The only function to close fd *ever*
> should be qmp_dump_guest_memory() (and that one should close fd with a
> direct close() call). Currently fd is basically a global variable,
> because the entire dump function tree has access to it (and closes it if
> there's an error).
> 

Although 's->fd' seems a global variable, but it is only have effect
within this file. It starts from qemu_open() or monitor_get_fd() in
qmp_dump_guest_memory(), and also end in qmp_dump_guest_memory().

dump_cleanup() is a static function, and qmp_dump_guest_memory() is
the only extern function which related with dump_cleanup() (I assume
'dump.c' will not be included directly by other C files).


> Anyway I guess it's OK to call dump_cleanup() to close s->fd just in case.
> 
> If you have a Coverity report, please share it.
> 

Excuse me, I only found it by reading source code (vi, grep, find ...), no
other additional tools.

> Then,
> 
>> diff --git a/dump.c b/dump.c
>> index ce646bc..71d3e94 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>>  
>>  static int dump_cleanup(DumpState *s)
>>  {
>> -    int ret = 0;
>> -
> 
> I agree with this change.
> 

Thanks.

>>      guest_phys_blocks_free(&s->guest_phys_blocks);
>>      memory_mapping_list_free(&s->list);
>> -    if (s->fd != -1) {
>> -        close(s->fd);
>> -    }
>> +    close(s->fd);
> 
> I disagree. It clobbers errno if s->fd is -1. Even though we don't
> particularly care about errno, it sort of disturbs be. Or can you prove
> s->fd is never -1 here?
> 

In our case, s->fd must be valid, or will return with failure in
qmp_dump_guest_memory().

For dump_cleanup(), at present, it is only a static function for share
code which need assume many things (e.g. only can be called once), not
generic enough.

But for simplify thinking, for me, it will be OK to let it be a generic
function, e.g: ('guest_phys_blocks_free' and 'memory_mapping_list_free'
know about NULL)

diff --git a/dump.c b/dump.c
index ce646bc..3f1ec5b 100644
--- a/dump.c
+++ b/dump.c
@@ -71,18 +71,18 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
 
 static int dump_cleanup(DumpState *s)
 {
-    int ret = 0;
-
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
     if (s->fd != -1) {
         close(s->fd);
+        s->fd = -1;
     }
     if (s->resume) {
         vm_start();
+        s->resume = false;
     }
 
-    return ret;
+    return 0;
 }
 
 static void dump_error(DumpState *s, const char *reason)


>>      if (s->resume) {
>>          vm_start();
>>      }
>>  
>> -    return ret;
>> +    return 0;
>>  }
>>  
>>  static void dump_error(DumpState *s, const char *reason)
>> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>>      s->begin = begin;
>>      s->length = length;
>>  
>> +    memory_mapping_list_init(&s->list);
>> +
>>      guest_phys_blocks_init(&s->guest_phys_blocks);
>>      guest_phys_blocks_append(&s->guest_phys_blocks);
>>  
>> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>>      }
>>  
>>      /* get memory mapping */
>> -    memory_mapping_list_init(&s->list);
>>      if (paging) {
>>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>>          if (err != NULL) {
>> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>>      return 0;
>>  
>>  cleanup:
>> -    guest_phys_blocks_free(&s->guest_phys_blocks);
>> -
>> -    if (s->resume) {
>> -        vm_start();
>> -    }
>> -
>> +    dump_cleanup(s);
>>      return -1;
>>  }
>>  
>>
> 
> This code is ripe for a generic lifecycle tracking overhaul, but since
> my view of ownership tracking is marginal in the qemu developer
> community, I'm not motivated.
>
> NB: I'm not nacking your patch, just please explain it better.
> 

OK, I can understand, and still thank you for your checking.

 
> Thanks
> Laszlo
> 

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Qemu-devel] Cc'ing emails [was: [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()]

[Chen Gang]

On 08/04/2014 03:59 PM, Michael Tokarev wrote:
> Please stop Cc'ing me emails sent to, at least, qemu-trivial@.
> 

OK, next, I shall notice about it: "stop Cc to you and to 'qemu-trivial'
(although for me, I still feel my patch is related with 'qemu-trivial').


> I'm about to filter personal emails which are also sent to
> some mailinglists I receive.  I'd not do that, because this is
> a good practice to Cc someone like that for various really
> important or urgent emails, and if I to apply such a filter,
> these urgent emails will be filtered too, obviously.
> 

For me, can use multiple emails, e.g: one for open mailing list, one for
personal, and one for company wide work.

> I'm not sure how people treat these cases or deal with them.
> We are subscribed to, in particular, qemu-devel@, and active
> maintainers look there too, so receive more than one copy of
> many emails.
> 

For me, I guess so (they will receive redundant emails)

> It is becoming worse.  With get_maintainer.pl pulling addresses
> of people who made changes or commits to files by default,
> contributing to the project becomes a bit dangerous.  Because
> as a result, once you fix something, you're essentially being
> subscribed to a spam list, because other contributors start
> Ccing you for the patches with which you have absolutely nothing
> to do, and if a discussion emerges, you can't opt out of it
> anymore (especially for patches which raise hot discussions).
> So I'd rather think twice before contributing anything...
> 

For some members (e.g. me), may use one email for open mailing list, but
use another email for apply patch, so Cc to them will let them notice
about soon.

By the way, in honest, as a gmail member, I almost do not check my email
in open mailing list, but always check personal mail which for sending
patches.


Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Qemu-devel] Cc'ing emails [

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Please stop Cc'ing me emails sent to, at least, qemu-trivial@.

I'll try to remember, but in general you can't expect everyone to keep
tabs on who wants and who doesn't want to be copied.

> I'm about to filter personal emails which are also sent to
> some mailinglists I receive.  I'd not do that, because this is
> a good practice to Cc someone like that for various really
> important or urgent emails, and if I to apply such a filter,
> these urgent emails will be filtered too, obviously.
>
> I'm not sure how people treat these cases or deal with them.
> We are subscribed to, in particular, qemu-devel@, and active
> maintainers look there too, so receive more than one copy of
> many emails.

I believe fighting the established convention to copy is futile.  I
embrace it instead, and make it help me prioritize my reading.  Copy me,
and I'll at least skim cover letters and other thread-starters to
determine whether I need to follow this thread.  Don't copy me, and I'll
at best glance at the subject in passing.

Automatic filing into folders and marking copies so I don't have to mark
them read twice helps.

The additional traffic is a drop in a bucket.

> It is becoming worse.  With get_maintainer.pl pulling addresses
> of people who made changes or commits to files by default,
> contributing to the project becomes a bit dangerous.  Because
> as a result, once you fix something, you're essentially being
> subscribed to a spam list, because other contributors start
> Ccing you for the patches with which you have absolutely nothing
> to do, and if a discussion emerges, you can't opt out of it
> anymore (especially for patches which raise hot discussions).
> So I'd rather think twice before contributing anything...

That's sad.

Re: [Qemu-devel] Cc'ing emails [

[Chen Gang]

Every members have their own tastes, and one working flow may be not
suitable for all members. I can understand, and hope other members
understand too.

At least for me, next, I shall send patch to the members which I can get
from 'get_maintainers.pl' and only Cc to qemu-devel. And shall skip
qemu-trivial and Michael Tokarev.

If any member feels my patch may related with qemu-trivial, please add
it in replying mailing list during reply, and mark Cc to qemu-trivial.

Welcome any other members' ideas, suggestions or completions.

Thanks.

On 08/04/2014 11:43 PM, Markus Armbruster wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> Please stop Cc'ing me emails sent to, at least, qemu-trivial@.
> 
> I'll try to remember, but in general you can't expect everyone to keep
> tabs on who wants and who doesn't want to be copied.
> 
>> I'm about to filter personal emails which are also sent to
>> some mailinglists I receive.  I'd not do that, because this is
>> a good practice to Cc someone like that for various really
>> important or urgent emails, and if I to apply such a filter,
>> these urgent emails will be filtered too, obviously.
>>
>> I'm not sure how people treat these cases or deal with them.
>> We are subscribed to, in particular, qemu-devel@, and active
>> maintainers look there too, so receive more than one copy of
>> many emails.
> 
> I believe fighting the established convention to copy is futile.  I
> embrace it instead, and make it help me prioritize my reading.  Copy me,
> and I'll at least skim cover letters and other thread-starters to
> determine whether I need to follow this thread.  Don't copy me, and I'll
> at best glance at the subject in passing.
> 
> Automatic filing into folders and marking copies so I don't have to mark
> them read twice helps.
> 
> The additional traffic is a drop in a bucket.
> 
>> It is becoming worse.  With get_maintainer.pl pulling addresses
>> of people who made changes or commits to files by default,
>> contributing to the project becomes a bit dangerous.  Because
>> as a result, once you fix something, you're essentially being
>> subscribed to a spam list, because other contributors start
>> Ccing you for the patches with which you have absolutely nothing
>> to do, and if a discussion emerges, you can't opt out of it
>> anymore (especially for patches which raise hot discussions).
>> So I'd rather think twice before contributing anything...
> 
> That's sad.
> 

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] Cc'ing emails [

[Michael Tokarev]

05.08.2014 08:41, Chen Gang wrote:
> 
> Every members have their own tastes, and one working flow may be not
> suitable for all members. I can understand, and hope other members
> understand too.
> 
> At least for me, next, I shall send patch to the members which I can get
> from 'get_maintainers.pl' and only Cc to qemu-devel. And shall skip
> qemu-trivial and Michael Tokarev.

Why skip both?  It's your call, but I'm curious.

What I _think_ wrong is that get_maintainers.pl lists many random
"patchers" for a given file by default.

Besides, we should probably review role of Anthony Ligory, because
he is returned as a sole contact for manu files, but apparently he
does not reply to emails anymore.

[]
>>> I'm not sure how people treat these cases or deal with them.
>>> We are subscribed to, in particular, qemu-devel@, and active
>>> maintainers look there too, so receive more than one copy of
>>> many emails.
>>
>> I believe fighting the established convention to copy is futile.  I
>> embrace it instead, and make it help me prioritize my reading.  Copy me,
>> and I'll at least skim cover letters and other thread-starters to
>> determine whether I need to follow this thread.  Don't copy me, and I'll
>> at best glance at the subject in passing.

We created some separate mailinglists - for example -trivial@ - especially
to get such attention.  This is what I'm talking about, in most part,
because main qemu mailinglist traffic become quite a bit high to follow
it closely, and it is a good idea indeed to Cc someone when sending mail
to qemu-devel@.  But even there, Cc'ing random "patchers" as get_maintainer.pl
often suggests is _not_ a good idea.  I think.

>> Automatic filing into folders and marking copies so I don't have to mark
>> them read twice helps.
>>
>> The additional traffic is a drop in a bucket.

Which traffic you refer to as "additional"?  The personal emails?

At least in my case it is quite significant because of qemu, and qemu
is _far_ from a single project where I actively contributed.  For example,
I contributed many things to postfix, but I don't have to worry about
it in any way, and I don't receive random personal emails - if something
is being Cc'ed to me it really is something important.  Ditto for linux
kernel and other areas.

In case of qemu, see -- for example, Anthony, who apparently stepped
out from qemu.  Almost every email on qemu-devel@ is being Cc'ed to
him nowadays, so he receives _whole_ qemu-devel@ in his personal
mailbox.  Is it also a drop in (his) bucket?

Thanks,

/mjt

Re: [Qemu-devel] Cc'ing emails [

[Peter Maydell]

n 5 August 2014 08:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 05.08.2014 08:41, Chen Gang wrote:
>>
>> Every members have their own tastes, and one working flow may be not
>> suitable for all members. I can understand, and hope other members
>> understand too.
>>
>> At least for me, next, I shall send patch to the members which I can get
>> from 'get_maintainers.pl' and only Cc to qemu-devel. And shall skip
>> qemu-trivial and Michael Tokarev.
>
> Why skip both?  It's your call, but I'm curious.

I think that is a misunderstanding. You asked not to get mails
cc'd to both you personally and qemu-trivial at the same time,
and Chen misread this to mean not to cc either address.

Trivial patches should still be sent "To: qemu-devel + Cc: qemu-trivial".

> What I _think_ wrong is that get_maintainers.pl lists many random
> "patchers" for a given file by default.

Yes, I think it's probably reasonable to change it to make it not
default to "--git-fallback". Do you want to submit a patch?

(Perhaps we could make it cc qemu-orphan@ if we wanted to
flag up holes in our MAINTAINERS coverage and patches liable
to get lost, but that probably only makes sense if we have people
who would care enough to do that monitoring work.)

thanks
-- PMM

Re: [Qemu-devel] Cc'ing emails [

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 05.08.2014 08:41, Chen Gang wrote:
>> 
>> Every members have their own tastes, and one working flow may be not
>> suitable for all members. I can understand, and hope other members
>> understand too.
>> 
>> At least for me, next, I shall send patch to the members which I can get
>> from 'get_maintainers.pl' and only Cc to qemu-devel. And shall skip
>> qemu-trivial and Michael Tokarev.
>
> Why skip both?  It's your call, but I'm curious.
>
> What I _think_ wrong is that get_maintainers.pl lists many random
> "patchers" for a given file by default.
>
> Besides, we should probably review role of Anthony Ligory, because
> he is returned as a sole contact for manu files, but apparently he
> does not reply to emails anymore.
>
> []
>>>> I'm not sure how people treat these cases or deal with them.
>>>> We are subscribed to, in particular, qemu-devel@, and active
>>>> maintainers look there too, so receive more than one copy of
>>>> many emails.
>>>
>>> I believe fighting the established convention to copy is futile.  I
>>> embrace it instead, and make it help me prioritize my reading.  Copy me,
>>> and I'll at least skim cover letters and other thread-starters to
>>> determine whether I need to follow this thread.  Don't copy me, and I'll
>>> at best glance at the subject in passing.
>
> We created some separate mailinglists - for example -trivial@ - especially
> to get such attention.  This is what I'm talking about, in most part,
> because main qemu mailinglist traffic become quite a bit high to follow
> it closely, and it is a good idea indeed to Cc someone when sending mail
> to qemu-devel@.  But even there, Cc'ing random "patchers" as get_maintainer.pl
> often suggests is _not_ a good idea.  I think.

I don't disagree with you there.  I take get_maintainer.pl as advice,
not gospel.

Note that because --git is *off* by default, you get "random patchers"
only when MAINTAINERS comes up empty.  Which it does far too often,
because its coverage is lousy: some 1300 out of >3600 files.

We could default --git-fallback to off to suppress "random patchers"
unless you ask for them.

>>> Automatic filing into folders and marking copies so I don't have to mark
>>> them read twice helps.
>>>
>>> The additional traffic is a drop in a bucket.
>
> Which traffic you refer to as "additional"?  The personal emails?

The personal copies compared to the full list traffic.

I count some 1200 messages to qemu-devel for the past week.  32 contain
the string "mjt@tls", which I take as a crude upper bound for you
getting a copy.  I don't mean to say that's not annoying or a drain on
your time (who am I to judge?), just that the additional network traffic
over full qemu-devel delivery is negligible.

> At least in my case it is quite significant because of qemu, and qemu
> is _far_ from a single project where I actively contributed.  For example,
> I contributed many things to postfix, but I don't have to worry about
> it in any way, and I don't receive random personal emails - if something
> is being Cc'ed to me it really is something important.  Ditto for linux
> kernel and other areas.

I recommend to set up filters to file away list traffic and copies of
list traffic separately from your private e-mail.

> In case of qemu, see -- for example, Anthony, who apparently stepped
> out from qemu.  Almost every email on qemu-devel@ is being Cc'ed to
> him nowadays, so he receives _whole_ qemu-devel@ in his personal
> mailbox.

I'd be surprised if he received copies in his personal inbox.  I expect
him to file them smartly.

>           Is it also a drop in (his) bucket?

I guess it is: 125 of last week's messages contain "aliguori@".

Re: [Qemu-devel] Cc'ing emails [

[Chen Gang]

On 08/05/2014 04:07 PM, Peter Maydell wrote:
> n 5 August 2014 08:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 05.08.2014 08:41, Chen Gang wrote:
>>>
>>> Every members have their own tastes, and one working flow may be not
>>> suitable for all members. I can understand, and hope other members
>>> understand too.
>>>
>>> At least for me, next, I shall send patch to the members which I can get
>>> from 'get_maintainers.pl' and only Cc to qemu-devel. And shall skip
>>> qemu-trivial and Michael Tokarev.
>>
>> Why skip both?  It's your call, but I'm curious.
> 
> I think that is a misunderstanding. You asked not to get mails
> cc'd to both you personally and qemu-trivial at the same time,
> and Chen misread this to mean not to cc either address.
> 
> Trivial patches should still be sent "To: qemu-devel + Cc: qemu-trivial".
>

OK, thank you for your explanation. Excuse me, my English is not quite
well, originally, I really misunderstood what Michael Tokarev said.

>> What I _think_ wrong is that get_maintainers.pl lists many random
>> "patchers" for a given file by default.
> 
> Yes, I think it's probably reasonable to change it to make it not
> default to "--git-fallback". Do you want to submit a patch?
> 
> (Perhaps we could make it cc qemu-orphan@ if we wanted to
> flag up holes in our MAINTAINERS coverage and patches liable
> to get lost, but that probably only makes sense if we have people
> who would care enough to do that monitoring work.)
> 

Originally, I assumed Michael Tokarev is that role (be the people who
would care enough to do that monitoring work), but sorry, at present,
I know, I misunderstand him (he is not this role).

It is really hard to find a member to act as this role (I guess, for act
as this role, he/she will be a real global maintainer of qemu).


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] Cc'ing emails [

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 4120 bytes --]

On Aug 5, 2014 2:42 AM, "Markus Armbruster" <armbru@redhat.com> wrote:
>
> Michael Tokarev <mjt@tls.msk.ru> writes:
>
> > 05.08.2014 08:41, Chen Gang wrote:
> >>
> >> Every members have their own tastes, and one working flow may be not
> >> suitable for all members. I can understand, and hope other members
> >> understand too.
> >>
> >> At least for me, next, I shall send patch to the members which I can
get
> >> from 'get_maintainers.pl' and only Cc to qemu-devel. And shall skip
> >> qemu-trivial and Michael Tokarev.
> >
> > Why skip both?  It's your call, but I'm curious.
> >
> > What I _think_ wrong is that get_maintainers.pl lists many random
> > "patchers" for a given file by default.
> >
> > Besides, we should probably review role of Anthony Ligory, because
> > he is returned as a sole contact for manu files, but apparently he
> > does not reply to emails anymore.
> >
> > []
> >>>> I'm not sure how people treat these cases or deal with them.
> >>>> We are subscribed to, in particular, qemu-devel@, and active
> >>>> maintainers look there too, so receive more than one copy of
> >>>> many emails.
> >>>
> >>> I believe fighting the established convention to copy is futile.  I
> >>> embrace it instead, and make it help me prioritize my reading.  Copy
me,
> >>> and I'll at least skim cover letters and other thread-starters to
> >>> determine whether I need to follow this thread.  Don't copy me, and
I'll
> >>> at best glance at the subject in passing.
> >
> > We created some separate mailinglists - for example -trivial@ -
especially
> > to get such attention.  This is what I'm talking about, in most part,
> > because main qemu mailinglist traffic become quite a bit high to follow
> > it closely, and it is a good idea indeed to Cc someone when sending mail
> > to qemu-devel@.  But even there, Cc'ing random "patchers" as
get_maintainer.pl
> > often suggests is _not_ a good idea.  I think.
>
> I don't disagree with you there.  I take get_maintainer.pl as advice,
> not gospel.
>
> Note that because --git is *off* by default, you get "random patchers"
> only when MAINTAINERS comes up empty.  Which it does far too often,
> because its coverage is lousy: some 1300 out of >3600 files.
>
> We could default --git-fallback to off to suppress "random patchers"
> unless you ask for them.
>
> >>> Automatic filing into folders and marking copies so I don't have to
mark
> >>> them read twice helps.
> >>>
> >>> The additional traffic is a drop in a bucket.
> >
> > Which traffic you refer to as "additional"?  The personal emails?
>
> The personal copies compared to the full list traffic.
>
> I count some 1200 messages to qemu-devel for the past week.  32 contain
> the string "mjt@tls", which I take as a crude upper bound for you
> getting a copy.  I don't mean to say that's not annoying or a drain on
> your time (who am I to judge?), just that the additional network traffic
> over full qemu-devel delivery is negligible.
>
> > At least in my case it is quite significant because of qemu, and qemu
> > is _far_ from a single project where I actively contributed.  For
example,
> > I contributed many things to postfix, but I don't have to worry about
> > it in any way, and I don't receive random personal emails - if something
> > is being Cc'ed to me it really is something important.  Ditto for linux
> > kernel and other areas.
>
> I recommend to set up filters to file away list traffic and copies of
> list traffic separately from your private e-mail.
>
> > In case of qemu, see -- for example, Anthony, who apparently stepped
> > out from qemu.  Almost every email on qemu-devel@ is being Cc'ed to
> > him nowadays, so he receives _whole_ qemu-devel@ in his personal
> > mailbox.
>
> I'd be surprised if he received copies in his personal inbox.  I expect
> him to file them smartly.
>
> >           Is it also a drop in (his) bucket?
>
> I guess it is: 125 of last week's messages contain "aliguori@".

Good email clients can filter with complex rules.  Just filter
to:your@mail.com and to:qemu-devel into a separate folder.

And yes, 15 emails a day is a drop in the bucket...

[-- Attachment #2: Type: text/html, Size: 5626 bytes --]

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Chen Gang]

If this patch need still be improvement (e.g. need let dump_cleanup
function as a generic one, or other cases), please let me know, and I
shall send patch v2 for it.

Thanks.

On 08/04/2014 09:51 PM, Chen Gang wrote:
> On 08/03/2014 11:56 PM, Laszlo Ersek wrote:
>> comments below
>>
> 
> Excuse me for replying late, firstly.
> 
>> On 08/03/14 17:28, Chen Gang wrote:
>>> In dump_init(), when failure occurs, need notice about 'fd' and memory
>>> mapping. So call dump_cleanup() for it (need let all initializations at
>>> front).
>>>
>>> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
>>> checking.
>>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>> ---
>>>  dump.c | 18 +++++-------------
>>>  1 file changed, 5 insertions(+), 13 deletions(-)
>>
>> Please explain what is leaked and how.
>>
> 
> Oh, sorry for the title misleading, need change to "Fix resource leak"
> instead of "Fix memory leak".
> 
>> The only possibility I can see (without digging very hard) is that
>> qemu_get_guest_memory_mapping() succeeds and lzo_init() fails (which
>> should never happen in practice).
>>
> 
> Yeah, what you said sounds reasonable to me.
>  
>> Regarding s->fd itself, I'm beyond trying to understand its lifecycle.
>> Qemu uses a bad ownership model wherein functions, in case of an
>> internal error, release resources they got from their callers. I'm
>> unable to reason in such a model.
> 
> Yeah, what you said sounds reasonable to me.
> 
>>                                   The only function to close fd *ever*
>> should be qmp_dump_guest_memory() (and that one should close fd with a
>> direct close() call). Currently fd is basically a global variable,
>> because the entire dump function tree has access to it (and closes it if
>> there's an error).
>>
> 
> Although 's->fd' seems a global variable, but it is only have effect
> within this file. It starts from qemu_open() or monitor_get_fd() in
> qmp_dump_guest_memory(), and also end in qmp_dump_guest_memory().
> 
> dump_cleanup() is a static function, and qmp_dump_guest_memory() is
> the only extern function which related with dump_cleanup() (I assume
> 'dump.c' will not be included directly by other C files).
> 
> 
>> Anyway I guess it's OK to call dump_cleanup() to close s->fd just in case.
>>
>> If you have a Coverity report, please share it.
>>
> 
> Excuse me, I only found it by reading source code (vi, grep, find ...), no
> other additional tools.
> 
>> Then,
>>
>>> diff --git a/dump.c b/dump.c
>>> index ce646bc..71d3e94 100644
>>> --- a/dump.c
>>> +++ b/dump.c
>>> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>>>  
>>>  static int dump_cleanup(DumpState *s)
>>>  {
>>> -    int ret = 0;
>>> -
>>
>> I agree with this change.
>>
> 
> Thanks.
> 
>>>      guest_phys_blocks_free(&s->guest_phys_blocks);
>>>      memory_mapping_list_free(&s->list);
>>> -    if (s->fd != -1) {
>>> -        close(s->fd);
>>> -    }
>>> +    close(s->fd);
>>
>> I disagree. It clobbers errno if s->fd is -1. Even though we don't
>> particularly care about errno, it sort of disturbs be. Or can you prove
>> s->fd is never -1 here?
>>
> 
> In our case, s->fd must be valid, or will return with failure in
> qmp_dump_guest_memory().
> 
> For dump_cleanup(), at present, it is only a static function for share
> code which need assume many things (e.g. only can be called once), not
> generic enough.
> 
> But for simplify thinking, for me, it will be OK to let it be a generic
> function, e.g: ('guest_phys_blocks_free' and 'memory_mapping_list_free'
> know about NULL)
> 
> diff --git a/dump.c b/dump.c
> index ce646bc..3f1ec5b 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -71,18 +71,18 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  
>  static int dump_cleanup(DumpState *s)
>  {
> -    int ret = 0;
> -
>      guest_phys_blocks_free(&s->guest_phys_blocks);
>      memory_mapping_list_free(&s->list);
>      if (s->fd != -1) {
>          close(s->fd);
> +        s->fd = -1;
>      }
>      if (s->resume) {
>          vm_start();
> +        s->resume = false;
>      }
>  
> -    return ret;
> +    return 0;
>  }
>  
>  static void dump_error(DumpState *s, const char *reason)
> 
> 
>>>      if (s->resume) {
>>>          vm_start();
>>>      }
>>>  
>>> -    return ret;
>>> +    return 0;
>>>  }
>>>  
>>>  static void dump_error(DumpState *s, const char *reason)
>>> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>>>      s->begin = begin;
>>>      s->length = length;
>>>  
>>> +    memory_mapping_list_init(&s->list);
>>> +
>>>      guest_phys_blocks_init(&s->guest_phys_blocks);
>>>      guest_phys_blocks_append(&s->guest_phys_blocks);
>>>  
>>> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>>>      }
>>>  
>>>      /* get memory mapping */
>>> -    memory_mapping_list_init(&s->list);
>>>      if (paging) {
>>>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>>>          if (err != NULL) {
>>> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>>>      return 0;
>>>  
>>>  cleanup:
>>> -    guest_phys_blocks_free(&s->guest_phys_blocks);
>>> -
>>> -    if (s->resume) {
>>> -        vm_start();
>>> -    }
>>> -
>>> +    dump_cleanup(s);
>>>      return -1;
>>>  }
>>>  
>>>
>>
>> This code is ripe for a generic lifecycle tracking overhaul, but since
>> my view of ownership tracking is marginal in the qemu developer
>> community, I'm not motivated.
>>
>> NB: I'm not nacking your patch, just please explain it better.
>>
> 
> OK, I can understand, and still thank you for your checking.
> 
>  
>> Thanks
>> Laszlo
>>
> 
> Thanks.
> 


-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Laszlo Ersek]

On 08/03/14 17:28, Chen Gang wrote:
> In dump_init(), when failure occurs, need notice about 'fd' and memory
> mapping. So call dump_cleanup() for it (need let all initializations at
> front).
> 
> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
> checking.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  dump.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index ce646bc..71d3e94 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  
>  static int dump_cleanup(DumpState *s)
>  {
> -    int ret = 0;
> -
>      guest_phys_blocks_free(&s->guest_phys_blocks);
>      memory_mapping_list_free(&s->list);
> -    if (s->fd != -1) {
> -        close(s->fd);
> -    }
> +    close(s->fd);
>      if (s->resume) {
>          vm_start();
>      }
>  
> -    return ret;
> +    return 0;
>  }
>  
>  static void dump_error(DumpState *s, const char *reason)
> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      s->begin = begin;
>      s->length = length;
>  
> +    memory_mapping_list_init(&s->list);
> +
>      guest_phys_blocks_init(&s->guest_phys_blocks);
>      guest_phys_blocks_append(&s->guest_phys_blocks);
>  
> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      }
>  
>      /* get memory mapping */
> -    memory_mapping_list_init(&s->list);
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>          if (err != NULL) {
> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      return 0;
>  
>  cleanup:
> -    guest_phys_blocks_free(&s->guest_phys_blocks);
> -
> -    if (s->resume) {
> -        vm_start();
> -    }
> -
> +    dump_cleanup(s);
>      return -1;
>  }
>  
> 

The patch is not trivial at all, because the lifecycles of the affected
resources are not trivial.

The commit message is an abomination. If you want to contribute to open
source, please learn proper English, and make a *serious* effort to
document your changes. Don't expect earlier contributors to have any
working knowledge about a file they have *maybe* touched several months
ago. People have to swap out a whole load of crap from their minds, and
swap in the old crap, to understand your patch. Help them by writing
good commit messages.

That said, no matter how annoying this submission is, my conscience
didn't allow me to let it go ignored, so I spent the time and made an
effort to track the lifetimes of "s->fd" and "s->list" in, and around,
dump_init().

The patch seems correct. That's your only excuse for the loss of gray
matter I've suffered while parsing your commit message.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Chen Gang]

On 08/12/2014 11:43 PM, Laszlo Ersek wrote:
> On 08/03/14 17:28, Chen Gang wrote:
>> In dump_init(), when failure occurs, need notice about 'fd' and memory
>> mapping. So call dump_cleanup() for it (need let all initializations at
>> front).
>>
>> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
>> checking.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>
[...]
> 
> The patch is not trivial at all, because the lifecycles of the affected
> resources are not trivial.
> 

OK, thanks.

For me, next, I shall split it into 2 pieces, one for resource leak, and
the other for improving current code, which may let reviewers easier
(maybe 2 patches belong to 2 different maintainers).

> The commit message is an abomination. If you want to contribute to open
> source, please learn proper English, and make a *serious* effort to
> document your changes. Don't expect earlier contributors to have any
> working knowledge about a file they have *maybe* touched several months
> ago. People have to swap out a whole load of crap from their minds, and
> swap in the old crap, to understand your patch. Help them by writing
> good commit messages.
> 

OK, thanks, And I am just trying to be improving.

 - always communicate with open source (qemu/kvm/kernel/binutils/gcc) in
   English (it must be).

 - I read/listen to Holy Bible in English every day: morning and evening
   in subway (I spend 2 hours from home to office, so 4 hours total),
   so at least, I spend about 1-1.5 hours for reading/listening per day.

> That said, no matter how annoying this submission is, my conscience
> didn't allow me to let it go ignored, so I spent the time and made an
> effort to track the lifetimes of "s->fd" and "s->list" in, and around,
> dump_init().
> 

OK, thanks, next, you can notify me for it, and I shall try to give some
related test (or we can test together for it), although it is not quite
easy for me.

> The patch seems correct. That's your only excuse for the loss of gray
> matter I've suffered while parsing your commit message.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

OK, thank you very much.


Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Luiz Capitulino]

On Sun, 03 Aug 2014 23:28:56 +0800
Chen Gang <gang.chen.5i5j@gmail.com> wrote:

> In dump_init(), when failure occurs, need notice about 'fd' and memory
> mapping. So call dump_cleanup() for it (need let all initializations at
> front).
> 
> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
> checking.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Applied to the qmp branch, thanks.

PS: I still find it hard to track the file-descriptor's life time. IMO,
    the best would be to always release it where it's allocated which is
    qmp_dump_guest_memory().

> ---
>  dump.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index ce646bc..71d3e94 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  
>  static int dump_cleanup(DumpState *s)
>  {
> -    int ret = 0;
> -
>      guest_phys_blocks_free(&s->guest_phys_blocks);
>      memory_mapping_list_free(&s->list);
> -    if (s->fd != -1) {
> -        close(s->fd);
> -    }
> +    close(s->fd);
>      if (s->resume) {
>          vm_start();
>      }
>  
> -    return ret;
> +    return 0;
>  }
>  
>  static void dump_error(DumpState *s, const char *reason)
> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      s->begin = begin;
>      s->length = length;
>  
> +    memory_mapping_list_init(&s->list);
> +
>      guest_phys_blocks_init(&s->guest_phys_blocks);
>      guest_phys_blocks_append(&s->guest_phys_blocks);
>  
> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      }
>  
>      /* get memory mapping */
> -    memory_mapping_list_init(&s->list);
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>          if (err != NULL) {
> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      return 0;
>  
>  cleanup:
> -    guest_phys_blocks_free(&s->guest_phys_blocks);
> -
> -    if (s->resume) {
> -        vm_start();
> -    }
> -
> +    dump_cleanup(s);
>      return -1;
>  }
>  

Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Chen Gang]

On 08/15/2014 04:49 AM, Luiz Capitulino wrote:
> On Sun, 03 Aug 2014 23:28:56 +0800
> Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 
>> > In dump_init(), when failure occurs, need notice about 'fd' and memory
>> > mapping. So call dump_cleanup() for it (need let all initializations at
>> > front).
>> > 
>> > Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
>> > checking.
>> > 
>> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> Applied to the qmp branch, thanks.
> 

Thanks.

> PS: I still find it hard to track the file-descriptor's life time. IMO,
>     the best would be to always release it where it's allocated which is
>     qmp_dump_guest_memory().
> 

If it is really hard to track, for me, I prefer to let dump_cleanup()
common enough, so can simplify every members thinking.

When use genric *_cleanup(), it means the resource manage management is
a little complex, need use a generic cleanup function for it to be sure
that every thing is not missed, and always save enough.


Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed