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.5...@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
