From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43719) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOoF4-0007Te-Mh for qemu-devel@nongnu.org; Wed, 25 Sep 2013 08:31:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOoEv-0000Br-LV for qemu-devel@nongnu.org; Wed, 25 Sep 2013 08:30:54 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:60845) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOoEu-0000B5-Un for qemu-devel@nongnu.org; Wed, 25 Sep 2013 08:30:45 -0400 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 25 Sep 2013 22:30:32 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 227303578050 for ; Wed, 25 Sep 2013 22:30:30 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r8PCUHCH65339492 for ; Wed, 25 Sep 2013 22:30:19 +1000 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r8PCUSI2025648 for ; Wed, 25 Sep 2013 22:30:28 +1000 Message-ID: <5242D760.1070401@linux.vnet.ibm.com> Date: Wed, 25 Sep 2013 20:30:24 +0800 From: Mark Wu MIME-Version: 1.0 References: <1379832654-21722-1-git-send-email-wudxw@linux.vnet.ibm.com> <20130924190032.7651.74662@loki> <5241E444.4040606@redhat.com> <20130924210729.20300.29270@loki> <20130924203454.2901afc3@redhat.com> In-Reply-To: <20130924203454.2901afc3@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Luiz Capitulino Cc: Michael Roth , Qemu-devel@nongnu.org Hi guys, Thanks a lot for all your insightful comments. I will post a patch to resolve the O(n2) problem according to Michael's comments soon and rebase the 'success-response' on it. Mark. On 09/25/2013 08:34 AM, Luiz Capitulino wrote: > On Tue, 24 Sep 2013 16:07:29 -0500 > Michael Roth wrote: > >>>>> +bool qmp_command_has_success_response(const char *name) >>>>> +{ >>>>> + QmpCommand *cmd; >>>>> + >>>>> + QTAILQ_FOREACH(cmd, &qmp_commands, node) { >>>>> + if (strcmp(cmd->name, name) == 0) { >>>>> + return cmd->options != 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); > Good catch! IIRC I added cmd->options myself and didn't catch this... > >>>>> +++ b/qga/commands.c >>>>> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) >>>>> cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); >>>>> cmd_info->name = g_strdup(*cmd_list); >>>>> cmd_info->enabled = qmp_command_is_enabled(cmd_info->name); >>>>> + cmd_info->success_response = >>>>> + 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. > I agree and I thought the same thing when I reviewed the patch, but > I didn't mind as Mark is just using what's already there. >