| Message ID | 1401480140-18653-4-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 2014-05-30 at 17:02 -0300, Eduardo Habkost wrote: > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Cc: Marcel Apfelbaum <marcel.a@redhat.com> > Cc: Andreas Färber <afaerber@suse.de> > --- > hw/core/machine.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index cbba679..df612bb 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->accel); I am not sure if in QMP is not caller's responsibility to free the input string. If I think about it, I ask an object to set "my" string and it deletes it :(... Same for the others. Added Markus and Luiz, maybe they have an opinion on that. Thanks, Marcel > ms->accel = g_strdup(value); > } > > @@ -79,6 +80,7 @@ static void machine_set_kernel(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->kernel_filename); > ms->kernel_filename = g_strdup(value); > } > > @@ -93,6 +95,7 @@ static void machine_set_initrd(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->initrd_filename); > ms->initrd_filename = g_strdup(value); > } > > @@ -107,6 +110,7 @@ static void machine_set_append(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->kernel_cmdline); > ms->kernel_cmdline = g_strdup(value); > } > > @@ -121,6 +125,7 @@ static void machine_set_dtb(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->dtb); > ms->dtb = g_strdup(value); > } > > @@ -135,6 +140,7 @@ static void machine_set_dumpdtb(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->dumpdtb); > ms->dumpdtb = g_strdup(value); > } > > @@ -176,6 +182,7 @@ static void machine_set_dt_compatible(Object *obj, const char *value, Error **er > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->dt_compatible); > ms->dt_compatible = g_strdup(value); > } > > @@ -232,6 +239,7 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) > { > MachineState *ms = MACHINE(obj); > > + g_free(ms->firmware); > ms->firmware = g_strdup(value); > } >
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Fri, 2014-05-30 at 17:02 -0300, Eduardo Habkost wrote: >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> --- >> Cc: Marcel Apfelbaum <marcel.a@redhat.com> >> Cc: Andreas Färber <afaerber@suse.de> >> --- >> hw/core/machine.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index cbba679..df612bb 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) >> { >> MachineState *ms = MACHINE(obj); >> >> + g_free(ms->accel); > I am not sure if in QMP is not caller's responsibility to free the input string. > If I think about it, I ask an object to set "my" string and it deletes it :(... > Same for the others. > > Added Markus and Luiz, maybe they have an opinion on that. > >> ms->accel = g_strdup(value); >> } >> Misunderstanding? Eduardo's patch frees the old value before it overwrites it. It doesn't free "the input string", assuming by "the input string" you mean argument value.
On Mon, 2014-06-02 at 13:51 +0200, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > On Fri, 2014-05-30 at 17:02 -0300, Eduardo Habkost wrote: > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> --- > >> Cc: Marcel Apfelbaum <marcel.a@redhat.com> > >> Cc: Andreas Färber <afaerber@suse.de> > >> --- > >> hw/core/machine.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/hw/core/machine.c b/hw/core/machine.c > >> index cbba679..df612bb 100644 > >> --- a/hw/core/machine.c > >> +++ b/hw/core/machine.c > >> @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) > >> { > >> MachineState *ms = MACHINE(obj); > >> > >> + g_free(ms->accel); > > I am not sure if in QMP is not caller's responsibility to free the input string. > > If I think about it, I ask an object to set "my" string and it deletes it :(... > > Same for the others. > > > > Added Markus and Luiz, maybe they have an opinion on that. > > > >> ms->accel = g_strdup(value); > >> } > >> > > Misunderstanding? Eduardo's patch frees the old value before it > overwrites it. It doesn't free "the input string", assuming by "the > input string" you mean argument value. You are right! My bad, for some reason I saw g_free(value), but it was me not reading it right :(. Thanks, Marcel
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Mon, 2014-06-02 at 13:51 +0200, Markus Armbruster wrote: >> Marcel Apfelbaum <marcel.a@redhat.com> writes: >> >> > On Fri, 2014-05-30 at 17:02 -0300, Eduardo Habkost wrote: >> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> >> --- >> >> Cc: Marcel Apfelbaum <marcel.a@redhat.com> >> >> Cc: Andreas Färber <afaerber@suse.de> >> >> --- >> >> hw/core/machine.c | 8 ++++++++ >> >> 1 file changed, 8 insertions(+) >> >> >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> >> index cbba679..df612bb 100644 >> >> --- a/hw/core/machine.c >> >> +++ b/hw/core/machine.c >> >> @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) >> >> { >> >> MachineState *ms = MACHINE(obj); >> >> >> >> + g_free(ms->accel); >> > I am not sure if in QMP is not caller's responsibility to free the >> > input string. >> > If I think about it, I ask an object to set "my" string and it >> > deletes it :(... >> > Same for the others. >> > >> > Added Markus and Luiz, maybe they have an opinion on that. >> > >> >> ms->accel = g_strdup(value); >> >> } >> >> >> >> Misunderstanding? Eduardo's patch frees the old value before it >> overwrites it. It doesn't free "the input string", assuming by "the >> input string" you mean argument value. > > You are right! My bad, for some reason I saw g_free(value), but it > was me not reading it right :(. Happens :) Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Mon, 2014-06-02 at 14:52 +0200, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > On Mon, 2014-06-02 at 13:51 +0200, Markus Armbruster wrote: > >> Marcel Apfelbaum <marcel.a@redhat.com> writes: > >> > >> > On Fri, 2014-05-30 at 17:02 -0300, Eduardo Habkost wrote: > >> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> >> --- > >> >> Cc: Marcel Apfelbaum <marcel.a@redhat.com> > >> >> Cc: Andreas Färber <afaerber@suse.de> > >> >> --- > >> >> hw/core/machine.c | 8 ++++++++ > >> >> 1 file changed, 8 insertions(+) > >> >> > >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c > >> >> index cbba679..df612bb 100644 > >> >> --- a/hw/core/machine.c > >> >> +++ b/hw/core/machine.c > >> >> @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) > >> >> { > >> >> MachineState *ms = MACHINE(obj); > >> >> > >> >> + g_free(ms->accel); > >> > I am not sure if in QMP is not caller's responsibility to free the > >> > input string. > >> > If I think about it, I ask an object to set "my" string and it > >> > deletes it :(... > >> > Same for the others. > >> > > >> > Added Markus and Luiz, maybe they have an opinion on that. > >> > > >> >> ms->accel = g_strdup(value); > >> >> } > >> >> > >> > >> Misunderstanding? Eduardo's patch frees the old value before it > >> overwrites it. It doesn't free "the input string", assuming by "the > >> input string" you mean argument value. > > > > You are right! My bad, for some reason I saw g_free(value), but it > > was me not reading it right :(. > > Happens :) > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Yes indeed. Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
diff --git a/hw/core/machine.c b/hw/core/machine.c index cbba679..df612bb 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->accel); ms->accel = g_strdup(value); } @@ -79,6 +80,7 @@ static void machine_set_kernel(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->kernel_filename); ms->kernel_filename = g_strdup(value); } @@ -93,6 +95,7 @@ static void machine_set_initrd(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->initrd_filename); ms->initrd_filename = g_strdup(value); } @@ -107,6 +110,7 @@ static void machine_set_append(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->kernel_cmdline); ms->kernel_cmdline = g_strdup(value); } @@ -121,6 +125,7 @@ static void machine_set_dtb(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->dtb); ms->dtb = g_strdup(value); } @@ -135,6 +140,7 @@ static void machine_set_dumpdtb(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->dumpdtb); ms->dumpdtb = g_strdup(value); } @@ -176,6 +182,7 @@ static void machine_set_dt_compatible(Object *obj, const char *value, Error **er { MachineState *ms = MACHINE(obj); + g_free(ms->dt_compatible); ms->dt_compatible = g_strdup(value); } @@ -232,6 +239,7 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->firmware); ms->firmware = g_strdup(value); }
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Marcel Apfelbaum <marcel.a@redhat.com> Cc: Andreas Färber <afaerber@suse.de> --- hw/core/machine.c | 8 ++++++++ 1 file changed, 8 insertions(+)