From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17131 invoked by alias); 4 Nov 2008 18:41:26 -0000 Received: (qmail 17032 invoked by uid 22791); 4 Nov 2008 18:41:25 -0000 X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 04 Nov 2008 18:40:49 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id mA4Iejpe002795; Tue, 4 Nov 2008 13:40:45 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id mA4IeiDc027181; Tue, 4 Nov 2008 13:40:44 -0500 Received: from opsy.redhat.com (vpn-14-82.rdu.redhat.com [10.11.14.82]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id mA4IehV9020873; Tue, 4 Nov 2008 13:40:44 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 440CA508089; Tue, 4 Nov 2008 11:40:43 -0700 (MST) To: =?utf-8?Q?S=C3=A9rgio?= Durigan =?utf-8?Q?J=C3=BAnior?= Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/4] 'catch syscall' feature -- XML support part References: <1225773086.24532.55.camel@miki> From: Tom Tromey Reply-To: tromey@redhat.com X-Attribution: Tom Date: Tue, 04 Nov 2008 18:41:00 -0000 In-Reply-To: <1225773086.24532.55.camel@miki> (=?utf-8?Q?=22S=C3=A9rgio?= Durigan =?utf-8?Q?J=C3=BAnior=22's?= message of "Tue\, 04 Nov 2008 02\:31\:26 -0200") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-11/txt/msg00042.txt.bz2 >>>>> "S=C3=A9rgio" =3D=3D S=C3=A9rgio Durigan J=C3=BAnior writes: S=C3=A9rgio> This is the part which adds XML support for the "catch S=C3=A9rgio> syscall" feature. I decided to put the architecture's XML S=C3=A9rgio> files here too, so that the architecture-independent part does S=C3=A9rgio> not grow too much. Just a few nits from me... S=C3=A9rgio> +struct syscalls_info S=C3=A9rgio> +{ S=C3=A9rgio> + /* The number of syscalls in this system. */ S=C3=A9rgio> + S=C3=A9rgio> + int number_of_syscalls; S=C3=A9rgio> + S=C3=A9rgio> + /* The syscalls. */ S=C3=A9rgio> + S=C3=A9rgio> + VEC(syscall_desc_p) *syscalls; S=C3=A9rgio> +}; I think number_of_syscalls is redundant here. The VEC knows how many elements it contains, so why not just use that? S=C3=A9rgio> +static void S=C3=A9rgio> +do_cleanup_fclose (void *file) S=C3=A9rgio> +{ S=C3=A9rgio> + fclose (file); S=C3=A9rgio> +} There's a make_cleanup_fclose now. S=C3=A9rgio> + [...] S=C3=A9rgio> + for (i =3D 0; i < len; i++) S=C3=A9rgio> + { S=C3=A9rgio> + if (strcmp (attrs[i].name, "name") =3D=3D 0) S=C3=A9rgio> + name =3D attrs[i].value; S=C3=A9rgio> + else if (strcmp (attrs[i].name, "number") =3D=3D 0) S=C3=A9rgio> + number =3D * (ULONGEST *) attrs[i].value; S=C3=A9rgio> + else S=C3=A9rgio> + internal_error (__FILE__, __LINE__, S=C3=A9rgio> + _("Unknown attribute name '%s'."), at= trs[i].name); S=C3=A9rgio> + } S=C3=A9rgio> + S=C3=A9rgio> + syscall_create_syscall_desc (data->sysinfo, name, number); This will make an entry with an empty name given the above XML snippet. But that doesn't seem helpful... if this triggers, won't it print as an unnamed syscall? It seems to me that it would be preferable to skip nameless ones and just report them numerically, if they occur. S=C3=A9rgio> +static const struct gdb_xml_attribute syscall_attr[] =3D { S=C3=A9rgio> + { "number", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulonges= t, NULL }, The indentation is wrong here and in other definitions like this. It should be just 2 spaces. S=C3=A9rgio> +static struct syscalls_info * S=C3=A9rgio> +syscall_parse_xml (const char *document, xml_fetch_another fe= tcher, S=C3=A9rgio> + void *fetcher_baton) S=C3=A9rgio> +{ [...] S=C3=A9rgio> + back_to =3D make_cleanup (null_cleanup, NULL); I don't think you need to make a null cleanup here... S=C3=A9rgio> + if (gdb_xml_parse (parser, document) =3D=3D 0) S=C3=A9rgio> + { S=C3=A9rgio> + /* Parsed successfully. */ S=C3=A9rgio> + discard_cleanups (result_cleanup); S=C3=A9rgio> + do_cleanups (back_to); S=C3=A9rgio> + return data.sysinfo; S=C3=A9rgio> + } S=C3=A9rgio> + else S=C3=A9rgio> + { S=C3=A9rgio> + warning (_("Could not load XML syscalls info; ignoring"= )); S=C3=A9rgio> + do_cleanups (back_to); S=C3=A9rgio> + return NULL; ... you can either just do_ or discard_ result_cleanup instead. Tom