Message ID | 53DE5538.1020701@gmail.com |
---|---|
State | New |
Headers | show |
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
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
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.
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.
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. >
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
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
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@".
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.
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...
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>
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.
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; > } >
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.
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; }
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(-)