From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33133) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkEGr-0004y4-Qu for qemu-devel@nongnu.org; Tue, 13 May 2014 11:05:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkEGm-00052a-Su for qemu-devel@nongnu.org; Tue, 13 May 2014 11:05:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30593) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkEGm-00052V-LJ for qemu-devel@nongnu.org; Tue, 13 May 2014 11:05:28 -0400 Message-ID: <537234B5.5030102@redhat.com> Date: Tue, 13 May 2014 09:05:25 -0600 From: Eric Blake MIME-Version: 1.0 References: <1399964572-5376-1-git-send-email-marc.mari.barcelo@gmail.com> <1399964572-5376-8-git-send-email-marc.mari.barcelo@gmail.com> In-Reply-To: <1399964572-5376-8-git-send-email-marc.mari.barcelo@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uwkXh0dFEGhPKjSloTx6lnAkqwXn1o6K3" Subject: Re: [Qemu-devel] [PATCH v2 07/16] stellaris: Convert conditional compilation of debug printfs to regular ifs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?TWFyYyBNYXLDrQ==?= , qemu-devel@nongnu.org Cc: Stefan Hajnoczi , Peter Crosthwaite , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --uwkXh0dFEGhPKjSloTx6lnAkqwXn1o6K3 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 05/13/2014 01:02 AM, Marc Mar=C3=AD wrote: > Modify debug macros to have the same format through the codebase and us= e regular > ifs instead of ifdef. >=20 > As the debug printf is always put in code, some casting had to be added= to avoid > warnings treated as errors at compile time. Same comments as in 1/16 about long lines and gcc extensions (probably for the entire series). Additionally... >=20 > Signed-off-by: Marc Mar=C3=AD > --- > hw/net/stellaris_enet.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) >=20 > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > index d04e6a4..f6737a9 100644 > --- a/hw/net/stellaris_enet.c > +++ b/hw/net/stellaris_enet.c > @@ -13,16 +13,17 @@ > //#define DEBUG_STELLARIS_ENET 1 > =20 > #ifdef DEBUG_STELLARIS_ENET > -#define DPRINTF(fmt, ...) \ > -do { printf("stellaris_enet: " fmt , ## __VA_ARGS__); } while (0) In this case, the old code was also relying on the gcc extension. But pre-patch, the gcc extension was only encountered if DEBUG_StELLARIS_ENET was set (probably only by a developer using gcc for temporary debugging), while post-patch the gcc extension is ALWAYS encountered, regardless of developer. > -#define BADF(fmt, ...) \ > -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__); = exit(1);} while (0) In the old code, BADF() could be used in a single statement context, such as: if (foo) BADF("blah") else something(); (of course, that violates our coding style, but hear me out). > +#define DEBUG_STELLARIS_ENET_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) do {} while(0) > -#define BADF(fmt, ...) \ > -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);}= while (0) > +#define DEBUG_STELLARIS_ENET_ENABLED 0 > #endif > =20 > +#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_STELLARIS_ENET_ENABLED, "= stellaris_enet", fmt, ## __VA_ARGS__) > + > +#define BADF(fmt, ...) \ > + QEMU_DPRINTF(1, "stellaris_enet error", fmt, ## __VA_ARGS__); \ > + do { if (DEBUG_STELLARIS_ENET_ENABLED) { exit(1); } } while (0) However, in the new code, BADF() is no longer a single statement. You either need to move the "do {" to appear before the QEMU_PRINTF [preferred], or you can just declare that this is no longer a single-statement macro at which point you could just drop the do/while altogether [depends on our coding standard for safety]. The example above, even though it violates coding standards, should demonstrate why your change is wrong - the bare "else" is now a syntax error. > =20 > - DPRINTF("Received packet len=3D%d\n", size); > + DPRINTF("Received packet len=3D%d\n", (int)size); Ah, we FINALLY get to an added cast. What you SHOULD be doing is using len=3D%zu coupled with un-casted size, rather than papering over the type= mismatch. > n =3D s->next_packet + s->np; > if (n >=3D 31) > n -=3D 31; > @@ -212,14 +213,14 @@ static void stellaris_enet_write(void *opaque, hw= addr offset, > switch (offset) { > case 0x00: /* IACK */ > s->ris &=3D ~value; > - DPRINTF("IRQ ack %02x/%02x\n", value, s->ris); > + DPRINTF("IRQ ack %02x/%02x\n", (unsigned)value, s->ris); > stellaris_enet_update(s); > /* Clearing TXER also resets the TX fifo. */ > if (value & SE_INT_TXER) > s->tx_frame_len =3D -1; > break; > case 0x04: /* IM */ > - DPRINTF("IRQ mask %02x/%02x\n", value, s->ris); > + DPRINTF("IRQ mask %02x/%02x\n", (unsigned)value, s->ris); More cases where you should be fixing the % format to use the correct type, rather than adding a cast. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --uwkXh0dFEGhPKjSloTx6lnAkqwXn1o6K3 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTcjS1AAoJEKeha0olJ0Nq4pgH/3bJSSNgonUeHJtKAViSB9XU jNqoz5sh3UiOnuziyQkCu6fByAGVUIHfvunTZjnc9hgUyLCbKo1p9+lI3hZkBI/U PhSKKFokMfHofLqqQd0824Oz0reLmH5IDVAI/1kpnw6J9GyIjG294eQGZ6F7y+Am gdijgalc14NcJSngg75YLXIZfTH6DDi7X8ySD7Z5zEfZotprUVIy8e7gjWSJDOvI qjXVN38smmWU5vREHCCxECLHQ+wjjw4M01ZtzB4wssRQdugyEw8+tpRt6j4xFO34 GEekm7vhC2PxaKYBbeFNmnWiZny0zI1/FB7rL0TPfDQgb11GCMkoA1ilq4E4yGY= =J17d -----END PGP SIGNATURE----- --uwkXh0dFEGhPKjSloTx6lnAkqwXn1o6K3--