From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52093) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsHq5-0002Q9-7a for qemu-devel@nongnu.org; Thu, 27 Jun 2013 15:26:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsHq3-0002Uc-Lf for qemu-devel@nongnu.org; Thu, 27 Jun 2013 15:26:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33125) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsHq3-0002UR-E5 for qemu-devel@nongnu.org; Thu, 27 Jun 2013 15:26:39 -0400 Message-ID: <51CC91EC.4070508@redhat.com> Date: Thu, 27 Jun 2013 13:26:36 -0600 From: Eric Blake MIME-Version: 1.0 References: <1372078125-31085-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1372078125-31085-4-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1372078125-31085-4-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2JBRBMLJXGQQTVOWONUHL" Subject: Re: [Qemu-devel] [PATCH V2 3/7] monitor: discard global variable *info_cmds in help functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: pbonzini@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2JBRBMLJXGQQTVOWONUHL Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/24/2013 06:48 AM, Wenchao Xia wrote: In the subject line, you aren't actually discarding a global variable, so much as avoiding its direct use (the global still exists). Maybe a better subject line would be: monitor: avoid direct use of global *info_cmds in help functions (2/7 has the same wording issue, where you aren't discarding the variable= ). > In help functions info_cmds is treated as sub command group now, not as= > a special case any more. Still help can't show message for single comma= nd > under "info", since command parser reject additional parameter, which > can be improved by change "help" item parameter define later. "log" is > still treated as special help case. compare_cmd() is used instead of st= rcmp() > in command searching. >=20 > To tip better what the patch does, code moving is avoided by declare s/tip better/give better hints about/ s/moving/motion/ s/declare/declaring/ > parse_cmdline() ahead. Rather than avoiding code motion by adding a forward declaration, I would instead split this into two patches - one that does JUST code motion, then the other that takes advantage of the correct motion. However, it's not a show-stopper to review. > static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, > - const char *prefix, const char *name) > + char **args, int nb_args, int arg_index) > { > const mon_cmd_t *cmd; > =20 > + /* Dump all */ > + if (arg_index >=3D nb_args) { > + for (cmd =3D cmds; cmd->name !=3D NULL; cmd++) { > + help_cmd_dump_one(mon, cmd, args, arg_index); > + } > + return; > + } > + > + /* Find one entry to dump */ > for(cmd =3D cmds; cmd->name !=3D NULL; cmd++) { Pre-existing formatting issue, but as long as you are touching both before and after, you might as well add the space after 'for'. > static void help_cmd(Monitor *mon, const char *name) > { > - if (name && !strcmp(name, "info")) { > - help_cmd_dump(mon, info_cmds, "info ", NULL); > - } else { > - help_cmd_dump(mon, mon->cmd_table, "", name); > - if (name && !strcmp(name, "log")) { > + char *args[MAX_ARGS]; > + int nb_args =3D 0, i; > + > + if (name) { > + /* special case for log */ > + if (!strcmp(name, "log")) { > const QEMULogItem *item; > monitor_printf(mon, "Log items (comma separated):\n"); > monitor_printf(mon, "%-10s %s\n", "none", "remove all logs= "); > for (item =3D qemu_log_items; item->mask !=3D 0; item++) {= > monitor_printf(mon, "%-10s %s\n", item->name, item->he= lp); > } > + return; > + } > + > + parse_cmdline(name, &nb_args, args); > + if (nb_args >=3D MAX_ARGS) { > + goto cleanup; [1] > } > } > + > + help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0); > + > +cleanup: > + for (i =3D 0; i < nb_args; i++) { > + g_free(args[i]); If we got here because nb_args > MAX_ARGS at point [1], then this calls g_free() on memory that is beyond the array (bad). Thankfully, I just read parse_cmdline, and it never sets nb_args > MAX_ARGS. But this whole parsing feels rather fragile (not necessarily your fault). Although I still recommend doing proper code motion for topological ordering instead of using a crutch of forward declarations, I'm okay if you fix the commit message and add Reviewed-by: Eric Blake . --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2JBRBMLJXGQQTVOWONUHL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRzJHsAAoJEKeha0olJ0NqYoUH/iSrEs2qeQOThNYlPM1bfgCb 9lEYH+GFAplf3I4BjKhhihTXAiXTx+XxvZqbEuQnYl/Zv5j821z7dfmrDvbq3PRc GSKwcl1CkXQXA4BbPXieTYSkNll6/ESOMA4F0MAePJ1zkhl4/isgjv4k4izAG0L+ d9HszaX+0q1bTvbKIxuJshTO9NLt0K9GXbGGKHZIn8Z27xSsHWJL5Ugnj4BD/30f DUu8eqNW2IHivPNUIk8HQaHgPF4bWkZ2OBE8yIWaEP3BOq2qeBcNCUjiW3E17o5w UNCOwDH0CaN28GwLqS5w2ba0115+8ScJaqhIAUqkRJb2uFR6NTVp0AQzUilT+ag= =ezv+ -----END PGP SIGNATURE----- ------enig2JBRBMLJXGQQTVOWONUHL--