From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blyeY-0000LY-U2 for qemu-devel@nongnu.org; Mon, 19 Sep 2016 09:30:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1blyeT-000654-QW for qemu-devel@nongnu.org; Mon, 19 Sep 2016 09:30:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32898) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blyeT-00064j-HB for qemu-devel@nongnu.org; Mon, 19 Sep 2016 09:30:29 -0400 From: Markus Armbruster References: <1472117833-10236-1-git-send-email-dgilbert@redhat.com> <1472117833-10236-3-git-send-email-dgilbert@redhat.com> Date: Mon, 19 Sep 2016 15:30:26 +0200 In-Reply-To: <1472117833-10236-3-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Thu, 25 Aug 2016 10:37:13 +0100") Message-ID: <87poo0f24t.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] qom: Implement qom-get HMP command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: qemu-devel@nongnu.org, lcapitulino@redhat.com, afaerber@suse.de, kwolf@redhat.com, pbonzini@redhat.com, arei.gonglei@huawei.com "Dr. David Alan Gilbert (git)" writes: > From: "Dr. David Alan Gilbert" > > Reimplement it based on qmp_qom_get() to avoid converting QObjects back > to strings. It's not using qmp_qom_get(), though. That's a sign our abstractions aren't quite right. HMP command handlers should be implemented as mostly trivial wrappers around the QMP handlers, or the two handlers should be mostly trivial wrappers around a common helper function. For a more detailed explanation, see below. Hmm, existing hmp_qom_set() has the same issue. I'm not demanding you clean this up. > Inspired-by: Paolo Bonzini > Signed-off-by: Andreas F=C3=A4rber > > Slight fix for bit-rot: > Signed-off-by: Dr. David Alan Gilbert > --- > hmp-commands.hx | 13 +++++++++++++ > hmp.c | 23 +++++++++++++++++++++++ > hmp.h | 1 + > 3 files changed, 37 insertions(+) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 848efee..73f0372 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1736,6 +1736,19 @@ Print QOM properties of object at location @var{pa= th} > ETEXI >=20=20 > { > + .name =3D "qom-get", > + .args_type =3D "path:s,property:s", > + .params =3D "path property", > + .help =3D "print QOM property", > + .mhandler.cmd =3D hmp_qom_get, > + }, > + > +STEXI > +@item qom-get @var{path} @var{property} > +Print QOM property @var{property} of object at location @var{path} This is a bit terse, but existing qom-list and qom-set are no less terse. The QAPI schema has more detail, perhaps we can adapt it for the manual. Not a blocker for me. > +ETEXI > + > + { > .name =3D "qom-set", > .args_type =3D "path:s,property:s,value:s", > .params =3D "path property value", > diff --git a/hmp.c b/hmp.c > index cc2056e..17e6b06 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -2064,6 +2064,29 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict) > hmp_handle_error(mon, &err); > } >=20=20 > +void hmp_qom_get(Monitor *mon, const QDict *qdict) > +{ > + const char *path =3D qdict_get_str(qdict, "path"); > + const char *property =3D qdict_get_str(qdict, "property"); > + Error *err =3D NULL; > + Object *obj; > + char *value; > + > + obj =3D object_resolve_path(path, NULL); > + if (obj =3D=3D NULL) { > + error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", path); > + hmp_handle_error(mon, &err); > + return; > + } > + value =3D object_property_print(obj, property, true, &err); > + if (err =3D=3D NULL) { > + monitor_printf(mon, "%s\n", value); > + g_free(value); > + } > + hmp_handle_error(mon, &err); > +} > + Compare: QObject *qmp_qom_get(const char *path, const char *property, Error **err= p) { Object *obj; obj =3D object_resolve_path(path, NULL); if (!obj) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", path); return NULL; } return object_property_get_qobject(obj, property, errp); } The missing piece seems to be a function that prints the result of object_property_get_qobject(). Since converting to QObject just for printing is stupid, a common helper function seems more appropriate. qmp_qom_get() would pass it the QMP output visitor to get a QObject, and hmp_qom_get() would pass it the string output visitor to get a string. > void hmp_qom_set(Monitor *mon, const QDict *qdict) > { > const char *path =3D qdict_get_str(qdict, "path"); > diff --git a/hmp.h b/hmp.h > index 0876ec0..882f339 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -103,6 +103,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict); > void hmp_info_memdev(Monitor *mon, const QDict *qdict); > void hmp_info_memory_devices(Monitor *mon, const QDict *qdict); > void hmp_qom_list(Monitor *mon, const QDict *qdict); > +void hmp_qom_get(Monitor *mon, const QDict *qdict); > void hmp_qom_set(Monitor *mon, const QDict *qdict); > void object_add_completion(ReadLineState *rs, int nb_args, const char *s= tr); > void object_del_completion(ReadLineState *rs, int nb_args, const char *s= tr);