On Mon, Jun 02, 2014 at 11:56:59AM +0300, Marcel Apfelbaum wrote:
> On Sun, 2014-06-01 at 11:31 +0300, Marcel Apfelbaum wrote:
> > On Fri, 2014-05-30 at 17:41 -0300, Eduardo Habkost wrote:
> > > The kvm-type machine option was left out when MachineState was
> > > introduced, preventing the kvm-type option from being used. Add the
> > > missing property.
> > Very interesting how did I miss that.
> > Thanks!
> > Marcel
> >
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@xxxxxxxxxx>
> > > Cc: Andreas Färber <afaerber@xxxxxxx>
> > > Cc: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> > > Cc: Alexander Graf <agraf@xxxxxxx>
> > > Cc: Marcel Apfelbaum <marcel.a@xxxxxxxxxx>
> > > ---
> > > Tested in a x86 machine only. Help would be welcome to test it on a PPC
> > > machine using -machine spapr and KVM.
> > >
> > > Before this patch:
> > >
> > > $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
> > > qemu-system-x86_64: Property '.kvm-type' not found
> > >
> > > (This means the option won't work even for sPAPR machines.)
> > >
> > > After applying this patch:
> > >
> > > $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
> > > Invalid argument kvm-type=hv
> > >
> > > (This means the x86 KVM init code is seeing (and rejecting) the option,
> > > and the sPAPR code can use it.)
> > >
> > > Note that qemu-system-x86_64 will segfault with the above command-line
> > > unless an additional fix (submitted today) is applied (kvm: Ensure
> > > negative return value on kvm_init() error handling path).
> > > ---
> > > hw/core/machine.c | 17 +++++++++++++++++
> > > include/hw/boards.h | 1 +
> > > 2 files changed, 18 insertions(+)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index cbba679..ed47b3a 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -235,6 +235,21 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> > > ms->firmware = g_strdup(value);
> > > }
> > >
> > > +static char *machine_get_kvm_type(Object *obj, Error **errp)
> > > +{
> > > + MachineState *ms = MACHINE(obj);
> > > +
> > > + return g_strdup(ms->kvm_type);
> > > +}
> > > +
> > > +static void machine_set_kvm_type(Object *obj, const char *value, Error **errp)
> > > +{
> > > + MachineState *ms = MACHINE(obj);
> > > +
> > > + g_free(ms->kvm_type);
> Hi,
>
> Here also, like in my other replies, I think it should be the caller
> responsibility to free the string. A set method should not touch
> its parameter.
We are not freeing the parameter, we are freeing ms->kvm_type, which was
allocated previously by the setter method itself (using g_strdup()). The
caller doesn't even have access to ms->kvm_type directly.
The fact that ms->kvm_type is a strdup()ed string is an internal
implementation detail. We even have setter methods that do _not_ use
strdup(), and just copy the data to a static buffer (see
target-i386/cpu.c).
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
