| Message ID | 54F194B7.9080603@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
On 2015/2/28 18:13, Michael Tokarev wrote: > 28.02.2015 13:08, arei.gonglei@huawei.com пишет: >> From: Gonglei <arei.gonglei@huawei.com> >> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> v2: fix compilation complaint. (mjt) >> --- >> hw/ppc/e500.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index 7e17d18..c060b50 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -309,8 +309,10 @@ static int ppce500_load_device_tree(MachineState *machine, >> >> fdt = load_device_tree(filename, &fdt_size); >> if (!fdt) { >> + g_free(filename); >> goto out; >> } >> + g_free(filename); >> goto done; >> } > > > How about this? > It's ok on function, but seems oddly, isn't it? Regards, -Gonglei > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -308,6 +308,7 @@ static int ppce500_load_device_tree(MachineState *machine, > } > > fdt = load_device_tree(filename, &fdt_size); > + g_free(filename); > if (!fdt) { > goto out; > } > > Thanks, > > /mjt >
28.02.2015 13:18, Gonglei wrote: > On 2015/2/28 18:13, Michael Tokarev wrote: [] >> How about this? >> > It's ok on function, but seems oddly, isn't it? I don't see anything odd in it. The `filename' variable is only used as an argument for load_device_tree() function and is not used anywhwere else, so it's logical to free it after use. >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -308,6 +308,7 @@ static int ppce500_load_device_tree(MachineState *machine, >> } >> >> fdt = load_device_tree(filename, &fdt_size); >> + g_free(filename); >> if (!fdt) { >> goto out; >> } >> Thanks, /mjt
On 28/02/2015 11:13, Michael Tokarev wrote: > 28.02.2015 13:08, arei.gonglei@huawei.com пишет: >> From: Gonglei <arei.gonglei@huawei.com> >> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> v2: fix compilation complaint. (mjt) >> --- >> hw/ppc/e500.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index 7e17d18..c060b50 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -309,8 +309,10 @@ static int ppce500_load_device_tree(MachineState *machine, >> >> fdt = load_device_tree(filename, &fdt_size); >> if (!fdt) { >> + g_free(filename); >> goto out; >> } >> + g_free(filename); >> goto done; >> } > > > How about this? > > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -308,6 +308,7 @@ static int ppce500_load_device_tree(MachineState *machine, > } > > fdt = load_device_tree(filename, &fdt_size); > + g_free(filename); > if (!fdt) { > goto out; > } Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> See also xilinx_load_device_tree: /* Try the local "ppc.dtb" override. */ fdt = load_device_tree("ppc.dtb", &fdt_size); if (!fdt) { path = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE); if (path) { fdt = load_device_tree(path, &fdt_size); g_free(path); } } Paolo
Am 28.02.2015 um 17:40 schrieb Paolo Bonzini: > On 28/02/2015 11:13, Michael Tokarev wrote: >> 28.02.2015 13:08, arei.gonglei@huawei.com пишет: >>> From: Gonglei <arei.gonglei@huawei.com> >>> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>> --- >>> v2: fix compilation complaint. (mjt) >>> --- >>> hw/ppc/e500.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >>> index 7e17d18..c060b50 100644 >>> --- a/hw/ppc/e500.c >>> +++ b/hw/ppc/e500.c >>> @@ -309,8 +309,10 @@ static int ppce500_load_device_tree(MachineState *machine, >>> >>> fdt = load_device_tree(filename, &fdt_size); >>> if (!fdt) { >>> + g_free(filename); >>> goto out; >>> } >>> + g_free(filename); >>> goto done; >>> } >> >> How about this? >> >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -308,6 +308,7 @@ static int ppce500_load_device_tree(MachineState *machine, >> } >> >> fdt = load_device_tree(filename, &fdt_size); >> + g_free(filename); >> if (!fdt) { >> goto out; >> } > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Which one of the two patch variants did you review? I suggest using Michael's version (only one g_free as early as possible). For that variant, my review may be added: Reviewed-by: Stefan Weil <sw@weilnetz.de>
On 28/02/2015 17:47, Stefan Weil wrote: >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Which one of the two patch variants did you review? > > I suggest using Michael's version (only one g_free as early as > possible). For that variant, my review may be added: > > Reviewed-by: Stefan Weil <sw@weilnetz.de> I also referred to Michael's. Paolo
On 2015/3/1 18:06, Paolo Bonzini wrote: > > > On 28/02/2015 17:47, Stefan Weil wrote: >>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >> >> Which one of the two patch variants did you review? >> >> I suggest using Michael's version (only one g_free as early as >> possible). For that variant, my review may be added: >> >> Reviewed-by: Stefan Weil <sw@weilnetz.de> > > I also referred to Michael's. > OK, thanks. Michael, need I send another version? Or you own handle it directly? Regards, -Gonglei
--- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -308,6 +308,7 @@ static int ppce500_load_device_tree(MachineState *machine, } fdt = load_device_tree(filename, &fdt_size); + g_free(filename); if (!fdt) { goto out; }