| Message ID | 1404032955-2591-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jun 29, 2014 at 12:09:15PM +0300, Marcel Apfelbaum wrote: > Replaced '_' with '-' to comply with QOM guidelines. > Made the conversion from HMP to QMP in vl.c > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> Nothing to do with me, pls merge through Andrea's or Paolo's tree. FWIW Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/core/machine.c | 8 ++++---- > vl.c | 12 +++++++++++- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index cbba679..7a66c57 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -239,11 +239,11 @@ static void machine_initfn(Object *obj) > { > object_property_add_str(obj, "accel", > machine_get_accel, machine_set_accel, NULL); > - object_property_add_bool(obj, "kernel_irqchip", > + object_property_add_bool(obj, "kernel-irqchip", > machine_get_kernel_irqchip, > machine_set_kernel_irqchip, > NULL); > - object_property_add(obj, "kvm_shadow_mem", "int", > + object_property_add(obj, "kvm-shadow-mem", "int", > machine_get_kvm_shadow_mem, > machine_set_kvm_shadow_mem, > NULL, NULL, NULL); > @@ -257,11 +257,11 @@ static void machine_initfn(Object *obj) > machine_get_dtb, machine_set_dtb, NULL); > object_property_add_str(obj, "dumpdtb", > machine_get_dumpdtb, machine_set_dumpdtb, NULL); > - object_property_add(obj, "phandle_start", "int", > + object_property_add(obj, "phandle-start", "int", > machine_get_phandle_start, > machine_set_phandle_start, > NULL, NULL, NULL); > - object_property_add_str(obj, "dt_compatible", > + object_property_add_str(obj, "dt-compatible", > machine_get_dt_compatible, > machine_set_dt_compatible, > NULL); > diff --git a/vl.c b/vl.c > index a1686ef..7587c97 100644 > --- a/vl.c > +++ b/vl.c > @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > Object *obj = OBJECT(opaque); > StringInputVisitor *siv; > Error *local_err = NULL; > + char *c, *qom_name; > > if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > strcmp(name, "type") == 0) { > return 0; > } > > + qom_name = g_strdup(name); > + c = qom_name; > + while (*c++) { > + if (*c == '_') { > + *c = '-'; > + } > + } > + > siv = string_input_visitor_new(value); > - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); > + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); > string_input_visitor_cleanup(siv); > + free(qom_name); > > if (local_err) { > qerror_report_err(local_err); > -- > 1.8.3.1
On Sun, 2014-06-29 at 14:37 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 29, 2014 at 12:09:15PM +0300, Marcel Apfelbaum wrote: > > Replaced '_' with '-' to comply with QOM guidelines. > > Made the conversion from HMP to QMP in vl.c > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Nothing to do with me, pls merge through Andrea's or Paolo's tree. > FWIW Ping. I thought we want this in 2.1 Thanks, Marcel > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > hw/core/machine.c | 8 ++++---- > > vl.c | 12 +++++++++++- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index cbba679..7a66c57 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -239,11 +239,11 @@ static void machine_initfn(Object *obj) > > { > > object_property_add_str(obj, "accel", > > machine_get_accel, machine_set_accel, NULL); > > - object_property_add_bool(obj, "kernel_irqchip", > > + object_property_add_bool(obj, "kernel-irqchip", > > machine_get_kernel_irqchip, > > machine_set_kernel_irqchip, > > NULL); > > - object_property_add(obj, "kvm_shadow_mem", "int", > > + object_property_add(obj, "kvm-shadow-mem", "int", > > machine_get_kvm_shadow_mem, > > machine_set_kvm_shadow_mem, > > NULL, NULL, NULL); > > @@ -257,11 +257,11 @@ static void machine_initfn(Object *obj) > > machine_get_dtb, machine_set_dtb, NULL); > > object_property_add_str(obj, "dumpdtb", > > machine_get_dumpdtb, machine_set_dumpdtb, NULL); > > - object_property_add(obj, "phandle_start", "int", > > + object_property_add(obj, "phandle-start", "int", > > machine_get_phandle_start, > > machine_set_phandle_start, > > NULL, NULL, NULL); > > - object_property_add_str(obj, "dt_compatible", > > + object_property_add_str(obj, "dt-compatible", > > machine_get_dt_compatible, > > machine_set_dt_compatible, > > NULL); > > diff --git a/vl.c b/vl.c > > index a1686ef..7587c97 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > > Object *obj = OBJECT(opaque); > > StringInputVisitor *siv; > > Error *local_err = NULL; > > + char *c, *qom_name; > > > > if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > > strcmp(name, "type") == 0) { > > return 0; > > } > > > > + qom_name = g_strdup(name); > > + c = qom_name; > > + while (*c++) { > > + if (*c == '_') { > > + *c = '-'; > > + } > > + } > > + > > siv = string_input_visitor_new(value); > > - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); > > + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); > > string_input_visitor_cleanup(siv); > > + free(qom_name); > > > > if (local_err) { > > qerror_report_err(local_err); > > -- > > 1.8.3.1 >
Il 17/07/2014 16:15, Marcel Apfelbaum ha scritto: > On Sun, 2014-06-29 at 14:37 +0300, Michael S. Tsirkin wrote: >> On Sun, Jun 29, 2014 at 12:09:15PM +0300, Marcel Apfelbaum wrote: >>> Replaced '_' with '-' to comply with QOM guidelines. >>> Made the conversion from HMP to QMP in vl.c >>> >>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> >> >> Nothing to do with me, pls merge through Andrea's or Paolo's tree. >> FWIW > Ping. > I thought we want this in 2.1 Renaming properties is fine according to the QOM guidelines, so I think it can be left for 2.2. Sorry for the delay, this patch escaped me completely. Paolo
Am 17.07.2014 16:20, schrieb Paolo Bonzini: > Il 17/07/2014 16:15, Marcel Apfelbaum ha scritto: >> On Sun, 2014-06-29 at 14:37 +0300, Michael S. Tsirkin wrote: >>> On Sun, Jun 29, 2014 at 12:09:15PM +0300, Marcel Apfelbaum wrote: >>>> Replaced '_' with '-' to comply with QOM guidelines. >>>> Made the conversion from HMP to QMP in vl.c >>>> >>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> >>> >>> Nothing to do with me, pls merge through Andrea's or Paolo's tree. >>> FWIW >> Ping. >> I thought we want this in 2.1 > > Renaming properties is fine according to the QOM guidelines, so I think > it can be left for 2.2. > > Sorry for the delay, this patch escaped me completely. Sorry, just seeing this patch now, too. My argument for getting this into 2.1 had been to avoid tools picking up these to-be-renamed property names from the start. At this point, I'm not so sure whether it's worse to break management tools or potentially some rarely used/tested option - if we decide for 2.2, is backporting to 2.1.1 an option if we document it in the release notes? Regards, Andreas
Quoting Andreas Färber (2014-07-17 10:48:59) > Am 17.07.2014 16:20, schrieb Paolo Bonzini: > > Il 17/07/2014 16:15, Marcel Apfelbaum ha scritto: > >> On Sun, 2014-06-29 at 14:37 +0300, Michael S. Tsirkin wrote: > >>> On Sun, Jun 29, 2014 at 12:09:15PM +0300, Marcel Apfelbaum wrote: > >>>> Replaced '_' with '-' to comply with QOM guidelines. > >>>> Made the conversion from HMP to QMP in vl.c > >>>> > >>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > >>> > >>> Nothing to do with me, pls merge through Andrea's or Paolo's tree. > >>> FWIW > >> Ping. > >> I thought we want this in 2.1 > > > > Renaming properties is fine according to the QOM guidelines, so I think > > it can be left for 2.2. > > > > Sorry for the delay, this patch escaped me completely. > > Sorry, just seeing this patch now, too. > > My argument for getting this into 2.1 had been to avoid tools picking up > these to-be-renamed property names from the start. At this point, I'm > not so sure whether it's worse to break management tools or potentially > some rarely used/tested option - if we decide for 2.2, is backporting to > 2.1.1 an option if we document it in the release notes? IMO, if there's some risk to breaking management or other tools, I'd rather it be left to major releases. And if these values are already misnamed for 2.1.0 and prior, I don't think we stop it from poliferating much more by pushing the fix up by a few months. I do think it makes sense to pull it in for 2.1.0-rc3, but I think that's a priority call and this seems fairly low risk. > > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On 29 June 2014 10:09, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > Replaced '_' with '-' to comply with QOM guidelines. > Made the conversion from HMP to QMP in vl.c > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > index a1686ef..7587c97 100644 > --- a/vl.c > +++ b/vl.c > @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > Object *obj = OBJECT(opaque); > StringInputVisitor *siv; > Error *local_err = NULL; > + char *c, *qom_name; > > if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > strcmp(name, "type") == 0) { > return 0; > } > > + qom_name = g_strdup(name); Memory allocated with g_strdup... > + c = qom_name; > + while (*c++) { > + if (*c == '_') { > + *c = '-'; > + } > + } > + > siv = string_input_visitor_new(value); > - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); > + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); > string_input_visitor_cleanup(siv); > + free(qom_name); ...but freed with free rather than g_free. > > if (local_err) { > qerror_report_err(local_err); > -- > 1.8.3.1 thanks -- PMM
On Thu, 2014-07-17 at 17:55 +0100, Peter Maydell wrote: > On 29 June 2014 10:09, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > Replaced '_' with '-' to comply with QOM guidelines. > > Made the conversion from HMP to QMP in vl.c > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > index a1686ef..7587c97 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > > Object *obj = OBJECT(opaque); > > StringInputVisitor *siv; > > Error *local_err = NULL; > > + char *c, *qom_name; > > > > if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > > strcmp(name, "type") == 0) { > > return 0; > > } > > > > + qom_name = g_strdup(name); > > Memory allocated with g_strdup... > > > + c = qom_name; > > + while (*c++) { > > + if (*c == '_') { > > + *c = '-'; > > + } > > + } > > + > > siv = string_input_visitor_new(value); > > - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); > > + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); > > string_input_visitor_cleanup(siv); > > + free(qom_name); > > ...but freed with free rather than g_free. Thanks, I'll take care of that. Marcel > > > > > if (local_err) { > > qerror_report_err(local_err); > > -- > > 1.8.3.1 > > thanks > -- PMM
Il 17/07/2014 18:47, Michael Roth ha scritto: >> > My argument for getting this into 2.1 had been to avoid tools picking up >> > these to-be-renamed property names from the start. At this point, I'm >> > not so sure whether it's worse to break management tools or potentially >> > some rarely used/tested option - if we decide for 2.2, is backporting to >> > 2.1.1 an option if we document it in the release notes? > IMO, if there's some risk to breaking management or other tools, I'd > rather it be left to major releases. And if these values are already misnamed > for 2.1.0 and prior, I don't think we stop it from poliferating much more by > pushing the fix up by a few months. I'm not sure in which case management could break (except for qom-get). Andreas, can you explain? Paolo
Am 17.07.2014 19:00, schrieb Paolo Bonzini: > Il 17/07/2014 18:47, Michael Roth ha scritto: >>> > My argument for getting this into 2.1 had been to avoid tools >>> picking up >>> > these to-be-renamed property names from the start. At this point, I'm >>> > not so sure whether it's worse to break management tools or >>> potentially >>> > some rarely used/tested option - if we decide for 2.2, is >>> backporting to >>> > 2.1.1 an option if we document it in the release notes? >> IMO, if there's some risk to breaking management or other tools, I'd >> rather it be left to major releases. And if these values are already >> misnamed >> for 2.1.0 and prior, I don't think we stop it from poliferating much >> more by >> pushing the fix up by a few months. > > I'm not sure in which case management could break (except for qom-get). > Andreas, can you explain? I was mainly concerned about qom-set, but same goes for qom-get. The breakage would be in 2.2, if in 2.1 we introduce properties with foo_bar and rename them to foo-bar in 2.2. Since they're not in 2.0, I had asked Marcel to rename them for 2.1 on a KVM call. I checked that sPAPR is not affected, so the only issue is the trivial g_free(). Since apart from sPAPR we have a compact snippet of properties being added, grep'ing for occurrences of the old strings and verifying that the patch changes all properties should be safe for -rc3 if Peter would be willing to take a pull. Andreas
Il 18/07/2014 16:15, Andreas Färber ha scritto: > I was mainly concerned about qom-set, but same goes for qom-get. The > breakage would be in 2.2, if in 2.1 we introduce properties with foo_bar > and rename them to foo-bar in 2.2. Since they're not in 2.0, I had asked > Marcel to rename them for 2.1 on a KVM call. > > I checked that sPAPR is not affected, so the only issue is the trivial > g_free(). Since apart from sPAPR we have a compact snippet of properties > being added, grep'ing for occurrences of the old strings and verifying > that the patch changes all properties should be safe for -rc3 if Peter > would be willing to take a pull. Okay, I agree. Paolo
Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: > Replaced '_' with '-' to comply with QOM guidelines. > Made the conversion from HMP to QMP in vl.c > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > hw/core/machine.c | 8 ++++---- > vl.c | 12 +++++++++++- > 2 files changed, 15 insertions(+), 5 deletions(-) [snip] > diff --git a/vl.c b/vl.c > index a1686ef..7587c97 100644 > --- a/vl.c > +++ b/vl.c > @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > Object *obj = OBJECT(opaque); > StringInputVisitor *siv; > Error *local_err = NULL; > + char *c, *qom_name; > > if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > strcmp(name, "type") == 0) { > return 0; > } > > + qom_name = g_strdup(name); > + c = qom_name; > + while (*c++) { > + if (*c == '_') { > + *c = '-'; > + } > + } Actually, is this really safe? By my reading, this function handles -object as well, which in turn allows - in theory - to instantiate any device, where some will still have underscores in their property names. Not sure if all non-device objects such as virtio-rng backends have been checked? Since it's really late, I would be more comfortable to copy this function with a large TODO and only apply this fixup for -machine, where we are more easily able to review and test. Opinions? Regards, Andreas > + > siv = string_input_visitor_new(value); > - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); > + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); > string_input_visitor_cleanup(siv); > + free(qom_name); > > if (local_err) { > qerror_report_err(local_err); >
Il 18/07/2014 16:25, Andreas Färber ha scritto: > Actually, is this really safe? By my reading, this function handles > -object as well, which in turn allows - in theory - to instantiate any > device, where some will still have underscores in their property names. > Not sure if all non-device objects such as virtio-rng backends have been > checked? I guess you would have to change devices from _ to - before adding UserCreatable to devices. -object backends are safe, they only use - (virtio-rng doesn't even have dashes). Paolo
On Fri, 2014-07-18 at 16:25 +0200, Andreas Färber wrote: > Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: > > Replaced '_' with '-' to comply with QOM guidelines. > > Made the conversion from HMP to QMP in vl.c > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > hw/core/machine.c | 8 ++++---- > > vl.c | 12 +++++++++++- > > 2 files changed, 15 insertions(+), 5 deletions(-) > [snip] > > diff --git a/vl.c b/vl.c > > index a1686ef..7587c97 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > > Object *obj = OBJECT(opaque); > > StringInputVisitor *siv; > > Error *local_err = NULL; > > + char *c, *qom_name; > > > > if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > > strcmp(name, "type") == 0) { > > return 0; > > } > > > > + qom_name = g_strdup(name); > > + c = qom_name; > > + while (*c++) { > > + if (*c == '_') { > > + *c = '-'; > > + } > > + } > > Actually, is this really safe? By my reading, this function handles > -object as well, which in turn allows - in theory - to instantiate any > device, where some will still have underscores in their property names. > Not sure if all non-device objects such as virtio-rng backends have been > checked? Hi Andreas, I checked and object_set_property is used only be machine right now, so no problem here. Thanks, Marcel > > Since it's really late, I would be more comfortable to copy this > function with a large TODO and only apply this fixup for -machine, where > we are more easily able to review and test. Opinions? > > Regards, > Andreas > > > + > > siv = string_input_visitor_new(value); > > - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); > > + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); > > string_input_visitor_cleanup(siv); > > + free(qom_name); > > > > if (local_err) { > > qerror_report_err(local_err); > > > >
On Fri, 2014-07-18 at 16:15 +0200, Andreas Färber wrote: > Am 17.07.2014 19:00, schrieb Paolo Bonzini: > > Il 17/07/2014 18:47, Michael Roth ha scritto: > >>> > My argument for getting this into 2.1 had been to avoid tools > >>> picking up > >>> > these to-be-renamed property names from the start. At this point, I'm > >>> > not so sure whether it's worse to break management tools or > >>> potentially > >>> > some rarely used/tested option - if we decide for 2.2, is > >>> backporting to > >>> > 2.1.1 an option if we document it in the release notes? > >> IMO, if there's some risk to breaking management or other tools, I'd > >> rather it be left to major releases. And if these values are already > >> misnamed > >> for 2.1.0 and prior, I don't think we stop it from poliferating much > >> more by > >> pushing the fix up by a few months. > > > > I'm not sure in which case management could break (except for qom-get). > > Andreas, can you explain? > > I was mainly concerned about qom-set, but same goes for qom-get. The > breakage would be in 2.2, if in 2.1 we introduce properties with foo_bar > and rename them to foo-bar in 2.2. Since they're not in 2.0, I had asked > Marcel to rename them for 2.1 on a KVM call. > > I checked that sPAPR is not affected, so the only issue is the trivial > g_free(). Since apart from sPAPR we have a compact snippet of properties > being added, grep'ing for occurrences of the old strings and verifying > that the patch changes all properties should be safe for -rc3 if Peter > would be willing to take a pull. Hi, The patch only affects machine properties. The patch will be upstream in a few minutes. Sorry for the delay. Thanks, Marcel > > Andreas >
Hi, Am 18.07.2014 17:59, schrieb Marcel Apfelbaum: > On Fri, 2014-07-18 at 16:25 +0200, Andreas Färber wrote: >> Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: >>> Replaced '_' with '-' to comply with QOM guidelines. >>> Made the conversion from HMP to QMP in vl.c >>> >>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> >>> --- >>> hw/core/machine.c | 8 ++++---- >>> vl.c | 12 +++++++++++- >>> 2 files changed, 15 insertions(+), 5 deletions(-) >> [snip] >>> diff --git a/vl.c b/vl.c >>> index a1686ef..7587c97 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque >>> Object *obj = OBJECT(opaque); >>> StringInputVisitor *siv; >>> Error *local_err = NULL; >>> + char *c, *qom_name; >>> >>> if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || >>> strcmp(name, "type") == 0) { >>> return 0; >>> } >>> >>> + qom_name = g_strdup(name); >>> + c = qom_name; >>> + while (*c++) { >>> + if (*c == '_') { >>> + *c = '-'; >>> + } >>> + } >> >> Actually, is this really safe? By my reading, this function handles >> -object as well, which in turn allows - in theory - to instantiate any >> device, where some will still have underscores in their property names. >> Not sure if all non-device objects such as virtio-rng backends have been >> checked? > Hi Andreas, > > I checked and object_set_property is used only be machine right now, so > no problem here. Indeed you are right. If -object is no longer using it, can we drop qom-type handling? What changed there? Regards, Andreas
On Fri, 2014-07-18 at 18:10 +0200, Andreas Färber wrote: > Hi, > > Am 18.07.2014 17:59, schrieb Marcel Apfelbaum: > > On Fri, 2014-07-18 at 16:25 +0200, Andreas Färber wrote: > >> Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: > >>> Replaced '_' with '-' to comply with QOM guidelines. > >>> Made the conversion from HMP to QMP in vl.c > >>> > >>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > >>> --- > >>> hw/core/machine.c | 8 ++++---- > >>> vl.c | 12 +++++++++++- > >>> 2 files changed, 15 insertions(+), 5 deletions(-) > >> [snip] > >>> diff --git a/vl.c b/vl.c > >>> index a1686ef..7587c97 100644 > >>> --- a/vl.c > >>> +++ b/vl.c > >>> @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > >>> Object *obj = OBJECT(opaque); > >>> StringInputVisitor *siv; > >>> Error *local_err = NULL; > >>> + char *c, *qom_name; > >>> > >>> if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > >>> strcmp(name, "type") == 0) { > >>> return 0; > >>> } > >>> > >>> + qom_name = g_strdup(name); > >>> + c = qom_name; > >>> + while (*c++) { > >>> + if (*c == '_') { > >>> + *c = '-'; > >>> + } > >>> + } > >> > >> Actually, is this really safe? By my reading, this function handles > >> -object as well, which in turn allows - in theory - to instantiate any > >> device, where some will still have underscores in their property names. > >> Not sure if all non-device objects such as virtio-rng backends have been > >> checked? > > Hi Andreas, > > > > I checked and object_set_property is used only be machine right now, so > > no problem here. > > Indeed you are right. If -object is no longer using it, can we drop > qom-type handling? What changed there? Hi Andreas, The check was originally placed there by Paolo for -object handling. We need to find out where the "qom-type" property is coming from. (What code is adding it) If is added automatically at parse/init time we can't get rid of it. If is object specific, it is ok. Paolo, can you please advise? Thanks, Marcel > Regards, > Andreas >
Am 18.07.2014 18:23, schrieb Marcel Apfelbaum: > On Fri, 2014-07-18 at 18:10 +0200, Andreas Färber wrote: >> Hi, >> >> Am 18.07.2014 17:59, schrieb Marcel Apfelbaum: >>> On Fri, 2014-07-18 at 16:25 +0200, Andreas Färber wrote: >>>> Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: >>>>> Replaced '_' with '-' to comply with QOM guidelines. >>>>> Made the conversion from HMP to QMP in vl.c >>>>> >>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> >>>>> --- >>>>> hw/core/machine.c | 8 ++++---- >>>>> vl.c | 12 +++++++++++- >>>>> 2 files changed, 15 insertions(+), 5 deletions(-) >>>> [snip] >>>>> diff --git a/vl.c b/vl.c >>>>> index a1686ef..7587c97 100644 >>>>> --- a/vl.c >>>>> +++ b/vl.c >>>>> @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque >>>>> Object *obj = OBJECT(opaque); >>>>> StringInputVisitor *siv; >>>>> Error *local_err = NULL; >>>>> + char *c, *qom_name; >>>>> >>>>> if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || >>>>> strcmp(name, "type") == 0) { >>>>> return 0; >>>>> } >>>>> >>>>> + qom_name = g_strdup(name); >>>>> + c = qom_name; >>>>> + while (*c++) { >>>>> + if (*c == '_') { >>>>> + *c = '-'; >>>>> + } >>>>> + } >>>> >>>> Actually, is this really safe? By my reading, this function handles >>>> -object as well, which in turn allows - in theory - to instantiate any >>>> device, where some will still have underscores in their property names. >>>> Not sure if all non-device objects such as virtio-rng backends have been >>>> checked? >>> Hi Andreas, >>> >>> I checked and object_set_property is used only be machine right now, so >>> no problem here. >> >> Indeed you are right. If -object is no longer using it, can we drop >> qom-type handling? What changed there? > Hi Andreas, > > The check was originally placed there by Paolo for -object handling. > We need to find out where the "qom-type" property is coming from. (What code is adding it) > If is added automatically at parse/init time we can't get rid of it. > If is object specific, it is ok. It was not a QOM property, it was a QemuOpt parameter for -object and therefore excluded from the handling like your type property is. Paolo had dicussed to rename qom-type to type for simplicity and consistency, but what I don't know is why this function is no longer used. Andreas > Paolo, can you please advise? > > Thanks, > Marcel
On Fri, 2014-07-18 at 18:32 +0200, Andreas Färber wrote: > Am 18.07.2014 18:23, schrieb Marcel Apfelbaum: > > On Fri, 2014-07-18 at 18:10 +0200, Andreas Färber wrote: > >> Hi, > >> > >> Am 18.07.2014 17:59, schrieb Marcel Apfelbaum: > >>> On Fri, 2014-07-18 at 16:25 +0200, Andreas Färber wrote: > >>>> Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: > >>>>> Replaced '_' with '-' to comply with QOM guidelines. > >>>>> Made the conversion from HMP to QMP in vl.c > >>>>> > >>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > >>>>> --- > >>>>> hw/core/machine.c | 8 ++++---- > >>>>> vl.c | 12 +++++++++++- > >>>>> 2 files changed, 15 insertions(+), 5 deletions(-) > >>>> [snip] > >>>>> diff --git a/vl.c b/vl.c > >>>>> index a1686ef..7587c97 100644 > >>>>> --- a/vl.c > >>>>> +++ b/vl.c > >>>>> @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque > >>>>> Object *obj = OBJECT(opaque); > >>>>> StringInputVisitor *siv; > >>>>> Error *local_err = NULL; > >>>>> + char *c, *qom_name; > >>>>> > >>>>> if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || > >>>>> strcmp(name, "type") == 0) { > >>>>> return 0; > >>>>> } > >>>>> > >>>>> + qom_name = g_strdup(name); > >>>>> + c = qom_name; > >>>>> + while (*c++) { > >>>>> + if (*c == '_') { > >>>>> + *c = '-'; > >>>>> + } > >>>>> + } > >>>> > >>>> Actually, is this really safe? By my reading, this function handles > >>>> -object as well, which in turn allows - in theory - to instantiate any > >>>> device, where some will still have underscores in their property names. > >>>> Not sure if all non-device objects such as virtio-rng backends have been > >>>> checked? > >>> Hi Andreas, > >>> > >>> I checked and object_set_property is used only be machine right now, so > >>> no problem here. > >> > >> Indeed you are right. If -object is no longer using it, can we drop > >> qom-type handling? What changed there? > > Hi Andreas, > > > > The check was originally placed there by Paolo for -object handling. > > We need to find out where the "qom-type" property is coming from. (What code is adding it) > > If is added automatically at parse/init time we can't get rid of it. > > If is object specific, it is ok. > > It was not a QOM property, it was a QemuOpt parameter for -object and > therefore excluded from the handling like your type property is. Paolo > had dicussed to rename qom-type to type for simplicity and consistency, > but what I don't know is why this function is no longer used. Paolo, can you help? I posted a V2, but I can resend in case it is needed, of course. Thanks, Marcel > > Andreas > > > Paolo, can you please advise? > > > > Thanks, > > Marcel >
Il 18/07/2014 18:32, Andreas Färber ha scritto: > Am 18.07.2014 18:23, schrieb Marcel Apfelbaum: >> On Fri, 2014-07-18 at 18:10 +0200, Andreas Färber wrote: >>> Hi, >>> >>> Am 18.07.2014 17:59, schrieb Marcel Apfelbaum: >>>> On Fri, 2014-07-18 at 16:25 +0200, Andreas Färber wrote: >>>>> Am 29.06.2014 11:09, schrieb Marcel Apfelbaum: >>>>>> Replaced '_' with '-' to comply with QOM guidelines. >>>>>> Made the conversion from HMP to QMP in vl.c >>>>>> >>>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> >>>>>> --- >>>>>> hw/core/machine.c | 8 ++++---- >>>>>> vl.c | 12 +++++++++++- >>>>>> 2 files changed, 15 insertions(+), 5 deletions(-) >>>>> [snip] >>>>>> diff --git a/vl.c b/vl.c >>>>>> index a1686ef..7587c97 100644 >>>>>> --- a/vl.c >>>>>> +++ b/vl.c >>>>>> @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque >>>>>> Object *obj = OBJECT(opaque); >>>>>> StringInputVisitor *siv; >>>>>> Error *local_err = NULL; >>>>>> + char *c, *qom_name; >>>>>> >>>>>> if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || >>>>>> strcmp(name, "type") == 0) { >>>>>> return 0; >>>>>> } >>>>>> >>>>>> + qom_name = g_strdup(name); >>>>>> + c = qom_name; >>>>>> + while (*c++) { >>>>>> + if (*c == '_') { >>>>>> + *c = '-'; >>>>>> + } >>>>>> + } >>>>> >>>>> Actually, is this really safe? By my reading, this function handles >>>>> -object as well, which in turn allows - in theory - to instantiate any >>>>> device, where some will still have underscores in their property names. >>>>> Not sure if all non-device objects such as virtio-rng backends have been >>>>> checked? >>>> Hi Andreas, >>>> >>>> I checked and object_set_property is used only be machine right now, so >>>> no problem here. >>> >>> Indeed you are right. If -object is no longer using it, can we drop >>> qom-type handling? What changed there? >> Hi Andreas, >> >> The check was originally placed there by Paolo for -object handling. >> We need to find out where the "qom-type" property is coming from. (What code is adding it) >> If is added automatically at parse/init time we can't get rid of it. >> If is object specific, it is ok. > > It was not a QOM property, it was a QemuOpt parameter for -object and > therefore excluded from the handling like your type property is. Paolo > had dicussed to rename qom-type to type for simplicity and consistency, > but what I don't know is why this function is no longer used. It is not used anymore because -object is now parsed using OptsVisitor; see function object_create. Paolo
diff --git a/hw/core/machine.c b/hw/core/machine.c index cbba679..7a66c57 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -239,11 +239,11 @@ static void machine_initfn(Object *obj) { object_property_add_str(obj, "accel", machine_get_accel, machine_set_accel, NULL); - object_property_add_bool(obj, "kernel_irqchip", + object_property_add_bool(obj, "kernel-irqchip", machine_get_kernel_irqchip, machine_set_kernel_irqchip, NULL); - object_property_add(obj, "kvm_shadow_mem", "int", + object_property_add(obj, "kvm-shadow-mem", "int", machine_get_kvm_shadow_mem, machine_set_kvm_shadow_mem, NULL, NULL, NULL); @@ -257,11 +257,11 @@ static void machine_initfn(Object *obj) machine_get_dtb, machine_set_dtb, NULL); object_property_add_str(obj, "dumpdtb", machine_get_dumpdtb, machine_set_dumpdtb, NULL); - object_property_add(obj, "phandle_start", "int", + object_property_add(obj, "phandle-start", "int", machine_get_phandle_start, machine_set_phandle_start, NULL, NULL, NULL); - object_property_add_str(obj, "dt_compatible", + object_property_add_str(obj, "dt-compatible", machine_get_dt_compatible, machine_set_dt_compatible, NULL); diff --git a/vl.c b/vl.c index a1686ef..7587c97 100644 --- a/vl.c +++ b/vl.c @@ -2820,15 +2820,25 @@ static int object_set_property(const char *name, const char *value, void *opaque Object *obj = OBJECT(opaque); StringInputVisitor *siv; Error *local_err = NULL; + char *c, *qom_name; if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 || strcmp(name, "type") == 0) { return 0; } + qom_name = g_strdup(name); + c = qom_name; + while (*c++) { + if (*c == '_') { + *c = '-'; + } + } + siv = string_input_visitor_new(value); - object_property_set(obj, string_input_get_visitor(siv), name, &local_err); + object_property_set(obj, string_input_get_visitor(siv), qom_name, &local_err); string_input_visitor_cleanup(siv); + free(qom_name); if (local_err) { qerror_report_err(local_err);
Replaced '_' with '-' to comply with QOM guidelines. Made the conversion from HMP to QMP in vl.c Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- hw/core/machine.c | 8 ++++---- vl.c | 12 +++++++++++- 2 files changed, 15 insertions(+), 5 deletions(-)