| Message ID | 53DF8FCA.1090602@gmail.com |
|---|---|
| State | New |
| Headers | show |
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. >
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)