From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39039) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOZpg-000052-D6 for qemu-devel@nongnu.org; Tue, 24 Sep 2013 17:07:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOZpY-0001yP-Kb for qemu-devel@nongnu.org; Tue, 24 Sep 2013 17:07:44 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:60124) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOZpY-0001y9-Gd for qemu-devel@nongnu.org; Tue, 24 Sep 2013 17:07:36 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 24 Sep 2013 17:07:35 -0400 Received: from b01cxnp23032.gho.pok.ibm.com (b01cxnp23032.gho.pok.ibm.com [9.57.198.27]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 7F4B6C90044 for ; Tue, 24 Sep 2013 17:07:31 -0400 (EDT) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by b01cxnp23032.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r8OL7VlP62062788 for ; Tue, 24 Sep 2013 21:07:31 GMT Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r8OL7Vi3013795 for ; Tue, 24 Sep 2013 17:07:31 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <5241E444.4040606@redhat.com> References: <1379832654-21722-1-git-send-email-wudxw@linux.vnet.ibm.com> <20130924190032.7651.74662@loki> <5241E444.4040606@redhat.com> Message-ID: <20130924210729.20300.29270@loki> Date: Tue, 24 Sep 2013 16:07:29 -0500 Subject: Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Mark Wu , Qemu-devel@nongnu.org, Luiz Capitulino Quoting Eric Blake (2013-09-24 14:13:08) > On 09/24/2013 01:00 PM, Michael Roth wrote: > > Quoting Mark Wu (2013-09-22 01:50:54) > >> Now we have several qemu-ga commands not returning response on success. > >> It has been documented in qga/qapi-schema.json already. This patch exp= oses > >> the 'success-response' flag by extending 'guest-info' command. With th= is > >> change, the clients can handle the command response more flexibly. > >> > >> Changes: > >> v2: add the notation 'since 1.7' to the option 'success-respon= se' > >> (per Eric Blake's comments) > >> > >> Signed-off-by: Mark Wu > > = > > Reviewed-by: Michael Roth > > = > > Eric, do we have your reviewed-by other than the changes you mentioned?= If so I > > can fix those up in my tree. > = > Aha - force me to do a FULL review, rather than just an interface > review. I found more issues, so this probably deserves a v2: Haha, you saw through my plan! :) > = > >> +bool qmp_command_has_success_response(const char *name) > >> +{ > >> + QmpCommand *cmd; > >> + > >> + QTAILQ_FOREACH(cmd, &qmp_commands, node) { > >> + if (strcmp(cmd->name, name) =3D=3D 0) { > >> + return cmd->options !=3D QCO_NO_SUCCESS_RESP; > = > cmd->options is a bitmask - it is feasible that we may add more QCO_NO_* > flags in the future, at which point inequality is NOT correct. Rather, > you want: > = > return !(cmd->options & QCO_NO_SUCCESS_RESP); > = > >> +++ b/qga/commands.c > >> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) > >> cmd_info =3D g_malloc0(sizeof(GuestAgentCommandInfo)); > >> cmd_info->name =3D g_strdup(*cmd_list); > >> cmd_info->enabled =3D qmp_command_is_enabled(cmd_info->name); > >> + cmd_info->success_response =3D > >> + qmp_command_has_success_response(cmd_info->name); > = > This feels wasteful. Why are we doing an O(n) lookup for BOTH > qmp_command_is_enabled AND qmp_command_has_success_response, in an O(n) > loop over command names? That's O(n^2) in the number of commands. > Better would be getting a list of QmpCommand* instead of a list of > char*, and looking directly in each object, for O(n) computation of the > results. Agreed, modifying qmp_get_command_list to return a list of QmpCommand would be nicer. Rather than looking directly at the fields though I think we should just fix up qmp_command_is_enabled() and friends to take a QmpCommand arg instead of a char*. We already have qmp_find_command to map char*->QmpCommand to support any cases where we rely on cmd names. > = > -- = > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org