dump.c: Fix memory leak issue in cleanup processing for dump_init()
PATCH 1/1 dump.c: Fix memory leak issue in cleanup processing for dump_init()
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.
--- dump.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
dump.c +5 -13
@@ -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 ; }
Signature · 1 line
1.7.11.7
comments below
Quoted 11 lines · Chen Gang
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_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 <[email protected]>--- 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,
dump.c uint64_t cpu_to_dump64(DumpState *s, uint64_t val) · line 75
Quoted 10 lines
dump.c +0
-2@@ -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.
dump.c uint64_t cpu_to_dump64(DumpState *s, uint64_t val) · line 76
Quoted 6 lines
dump.c +1
-3guest_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?
Quoted 31 lines
dump.c +3
-2if (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:
dump.c static int dump_init(DumpState *s, int fd, bool has_format, · line 1625
Quoted 10 lines
dump.c +1
-6-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()]
In reply to Chen GangPlease 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
Quoted 2 lines · Laszlo Ersek
comments below
Excuse me for replying late, firstly.
Quoted 8 lines · Chen Gang
Quoted 7 lines
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_cleanup()for it (need let all initializations at front).Also simplify
dump_cleanup(): remove redundant 'ret' and redundant 'fd' checking.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Chen Gang <[email protected]>--- 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".
Quoted 4 lines
The only possibility I can see (without digging very hard) is that
qemu_get_guest_memory_mapping()succeeds andlzo_init()fails (which should never happen in practice).
Yeah, what you said sounds reasonable to me.
Quoted 4 lines
Regarding
s->fditself, 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.
Quoted 6 lines
The only function to close fd *ever*
should beqmp_dump_guest_memory()(and that one should close fd with a directclose()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).
Quoted 4 lines
Anyway I guess it's OK to call
dump_cleanup()to closes->fdjust 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.
Quoted 8 lines
Then,
Quoted 6 lines
dump.c +0
-0@@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
Quoted 7 lines
dump.c uint64_t cpu_to_dump64(DumpState *s, uint64_t val) · line 75
Quoted 4 lines
dump.c +0
-2static int dump_cleanup (DumpState *s) { -int ret =0 ; -I agree with this change.
Thanks.
Quoted 11 lines
dump.c uint64_t cpu_to_dump64(DumpState *s, uint64_t val) · line 76
Quoted 6 lines
dump.c +1
-3guest_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->fdis -1. Even though we don't particularly care about errno, it sort of disturbs be. Or can you proves->fdis 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)
dump.c +3 -3
@@ -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)
Quoted 46 lines
Quoted 31 lines
dump.c +3
-2if (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:dump.c static int dump_init(DumpState *s, int fd, bool has_format, · line 1625
Quoted 10 lines
dump.c +1
-6-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.
Quoted 2 lines
NB: I'm not nacking your patch, just please explain it better.
OK, I can understand, and still thank you for your checking.
Quoted 3 lines
Thanks
Laszlo
Thanks.
Signature · 3 lines
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()]
In reply to Michael TokarevQuoted 2 lines · Michael Tokarev
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').
Quoted 6 lines
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.
Quoted 5 lines
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)
Quoted 10 lines
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.
Signature · 3 lines
Chen Gang Open share and attitude like air water and life which God blessed
Re: [Qemu-devel] Cc'ing emails [
In reply to Michael TokarevQuoted 1 line · Michael Tokarev
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.
Quoted 10 lines
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.
Quoted 9 lines
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 [
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.
Quoted 41 lines · Markus Armbruster
Quoted 1 line · Michael Tokarev
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.
Quoted 10 lines
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.
Quoted 9 lines
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.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] Cc'ing emails [
Quoted 8 lines · 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.
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.
[]
Quoted 10 lines
Quoted 10 lines
Quoted 4 lines
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.
Quoted 4 lines
Quoted 4 lines
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 [
Quoted 11 lines · Michael Tokarev
Quoted 8 lines · 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.
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".
Quoted 2 lines
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 [
In reply to Michael TokarevQuoted 31 lines · Michael Tokarev
Quoted 8 lines · 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.
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.
[]
Quoted 10 lines
Quoted 10 lines
Quoted 4 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 1 line
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 [
In reply to Peter MaydellQuoted 11 lines · Peter Maydell
Quoted 10 lines · Michael Tokarev
Quoted 8 lines · 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.
Quoted 8 lines
Quoted 1 line
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.
Quoted 11 lines
Quoted 2 lines
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
MAINTAINERScoverage 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.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] Cc'ing emails [
In reply to Markus ArmbrusterQuoted 10 lines · Markus Armbruster
Quoted 7 lines · Michael Tokarev
Quoted 6 lines · 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
Quoted 12 lines
Quoted 12 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 8 lines
[]
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
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,
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
at best glance at the subject in passing.
We created some separate mailinglists - for example -trivial@ -
especially
Quoted 4 lines
Quoted 4 lines
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
Quoted 5 lines
Quoted 1 line
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.
Quoted 8 lines
Note that because --git is *off* by default, you get "random patchers" only when
MAINTAINERScomes 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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Automatic filing into folders and marking copies so I don't have to
mark
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
them read twice helps.
The additional traffic is a drop in a bucket.
Which traffic you refer to as "additional"? The personal emails?
Quoted 10 lines
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.
Quoted 2 lines
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,
Quoted 13 lines
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 6 lines
I'd be surprised if he received copies in his personal inbox. I expect him to file them smartly.
Quoted 1 line
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:[email protected] and to:qemu-devel into a separate folder.
And yes, 15 emails a day is a drop in the bucket...
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.
Quoted 190 lines · Chen Gang
Quoted 2 lines · Laszlo Ersek
comments below
Excuse me for replying late, firstly.
Quoted 8 lines · Chen Gang
Quoted 7 lines
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_cleanup()for it (need let all initializations at front).Also simplify
dump_cleanup(): remove redundant 'ret' and redundant 'fd' checking.Quoted 7 lines
Quoted 4 lines
Signed-off-by Chen Gang <[email protected]>--- 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".
Quoted 4 lines
The only possibility I can see (without digging very hard) is that
qemu_get_guest_memory_mapping()succeeds andlzo_init()fails (which should never happen in practice).Yeah, what you said sounds reasonable to me.
Quoted 4 lines
Regarding
s->fditself, 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.
Quoted 6 lines
The only function to close fd *ever*
should beqmp_dump_guest_memory()(and that one should close fd with a directclose()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 fromqemu_open()ormonitor_get_fd()inqmp_dump_guest_memory(), and also end inqmp_dump_guest_memory().
dump_cleanup()is a static function, andqmp_dump_guest_memory()is the only extern function which related withdump_cleanup()(I assume 'dump.c' will not be included directly by other C files).Quoted 4 lines
Anyway I guess it's OK to call
dump_cleanup()to closes->fdjust 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.
Quoted 8 lines
Then,
Quoted 6 lines
dump.c +0
-0@@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)Quoted 7 lines
dump.c uint64_t cpu_to_dump64(DumpState *s, uint64_t val) · line 75
Quoted 4 lines
dump.c +0
-2static int dump_cleanup (DumpState *s) { -int ret =0 ; -I agree with this change.
Thanks.
Quoted 11 lines
dump.c uint64_t cpu_to_dump64(DumpState *s, uint64_t val) · line 76
Quoted 6 lines
dump.c +1
-3guest_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->fdis -1. Even though we don't particularly care about errno, it sort of disturbs be. Or can you proves->fdis never -1 here?In our case,
s->fdmust be valid, or will return with failure inqmp_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)dump.c +3
-3@@ -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)Quoted 48 lines
Quoted 33 lines
dump.c +3
-2if (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:dump.c static int dump_init(DumpState *s, int fd, bool has_format, · line 1625
Quoted 10 lines
dump.c +1
-6-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.
Quoted 2 lines
NB: I'm not nacking your patch, just please explain it better.
OK, I can understand, and still thank you for your checking.
Quoted 3 lines
Thanks
LaszloThanks.
Signature · 3 lines
Chen Gang Open share and attitude like air water and life which God blessed
Quoted 59 lines · Chen Gang
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_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 <[email protected]>--- dump.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)dump.c +4
-7@@ -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:
dump.c static int dump_init(DumpState *s, int fd, bool has_format, · line 1625
Quoted 10 lines
dump.c +1
-6-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.
Quoted 11 lines · Laszlo Ersek
Quoted 10 lines · Chen Gang
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_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 <[email protected]>---
[...]
Quoted 4 lines
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).
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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 <[email protected]>
OK, thank you very much.
Thanks.
Signature · 3 lines
Chen Gang Open share and attitude like air water and life which God blessed
Luiz Capitulino
AppliedQuoted 8 lines · On Sun, 03 Aug 2014 23:28:56 +0800 Chen Gang <[email protected]> wrote:
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_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 <[email protected]>
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().
Quoted 60 lines
--- dump.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)dump.c +5
-13@@ -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 ; }
Quoted 7 lines · Luiz Capitulino
Quoted 4 lines · On Sun, 03 Aug 2014 23:28:56 +0800 Chen Gang <[email protected]> wrote:
Quoted 4 lines
In
dump_init(), when failure occurs, need notice about 'fd' and memory mapping. So calldump_cleanup()for it (need let all initializations at front).
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Also simplify
dump_cleanup(): remove redundant 'ret' and redundant 'fd' checking.Signed-off-by Chen Gang <[email protected]>Applied to the qmp branch, thanks.
Thanks.
Quoted 4 lines
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 isqmp_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.
Signature · 3 lines
Chen Gang Open share and attitude like air water and life which God blessed