Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
On Mon, Apr 28, 2014 at 11:34 PM, Andreas Färber afaer...@suse.de wrote: Am 28.04.2014 13:21, schrieb Peter Crosthwaite: Hi Marc, On such a long series, it's usual to include a cover letter summarising the entire series. Its subject is PATCH 00/NN and can be generated by adding the --cover-letter switch to git send-email. Hand edit the file them send along with the autogenerated patches. On Mon, Apr 28, 2014 at 6:26 PM, Marc Marí marc.mari.barc...@gmail.com wrote: From: Marc Marí 5.markm...@gmail.com Modify debug macros as explained in https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html Wrap the body of the commit message to 72 chars, it's ok to blow the limit for a long URL IMO but you should at least line break between in https. Or better drop that reference at all. The archives are not kept forever, so any essential explanation should go into the commit message proper. In this case, a copiedpasted This avoids code bitrotting. or so would be sufficient as the pattern is obvious from looking at the diff and mentioned in the subject. On that matter, there is no dma subsystem. It would be helpful if there were Maintainerships that directly matched the file org. After all hw/dma/* is very real and exists. To that end perhaps this maintainership should exist? Regards, Peter There's i82374, which is part of PReP. i8257 is a core PC thing, not sure if that means mst? Don't know about rc4030. Anyway, this series is clearly not divided by maintenance areas and not CC'ing the appropriate maintainers - Marc, please use: git config sendemail.cccmd scripts/get_maintainer.pl --nogit-fallback to CC the maintainers documented in MAINTAINERS file. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
On Mon, Apr 28, 2014 at 11:44 PM, Andreas Färber afaer...@suse.de wrote:
Am 28.04.2014 15:35, schrieb Peter Crosthwaite:
On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber afaer...@suse.de wrote:
Am 28.04.2014 14:41, schrieb Peter Crosthwaite:
On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber afaer...@suse.de wrote:
Hi Marc,
Am 28.04.2014 10:26, schrieb Marc Marí:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
This is exactly how I told you not to do it in response to Peter C.'s
proposal. I had done so in my v1 [1] and it was rejected.
Instead it was concluded that we need:
//#define DEBUG_FOO
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED 1
#else
#define DEBUG_FOO_ENABLED 2
Oops, obviously this what meant to read 0, not 2, i.e. translating
absence of a constant to another constant that is assured to always
carry a value.
true/false may be an alternative for boolean logic.
#endif
if you are going to go this way you probably want:
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED DEBUG_FOO
#else
#define DEBUG_FOO_ENABLED 0
#endif
Is it guaranteed that #define DEBUG_FOO = DEBUG_FOO == 1? I assume so.
well -DDEBUG_FOO as a CFLAG comes with this == 1 guarantee which is
the prescribed usage. If you want to hack source with #define then
it's local to the code and your compile failure will quickly
straighten you out.
Otherwise this may be just applicable to code where you do this kind of
level-based distinction rather than presence/absence.
I prefer always level-capable - It's nice to be able to quickly add a
higher level of debug without applying a framework change.
The real question to ask is, does the code have any #ifdef DEBUG_FOO, or
does the respective maintainer intend to use it that way? If not, then
your if (DEBUG_FOO) {...} is perfectly valid and makes more sense than
having ..._ENABLED be anything but a boolean. It's just totally unsafe
to assume this to work everywhere in one huge cross-maintainer
refactoring series, as my earlier series showed (which did locate and
update such #ifdef DEBUG_FOO code). The series becomes non-trivial then.
Your change is a good idea perhaps even universally. Just I think
levels are a valuable feature.
I wouldn't mind, but I suggest DEBUG_FOO or DEBUG_FOO_LEVEL naming then,
not DEBUG_FOO_ENABLED with numeric values please.
Sounds good. I see your point. _LEVEL universally is my recommendation.
Regards,
Peter
Something else to consider as well will be converting these #define
DEBUG_FOOs further down the file to regular if() too where possible
for the same reasons as this series.
Agreed, but either as follow-up patch/series for better review, or by
re-dividing this series along maintenance lines.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
+
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_I82374) { \
+fprintf(stderr, I82374: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
#define BADF(fmt, ...) \
do { fprintf(stderr, i82374 ERROR: fmt , ## __VA_ARGS__); } while (0)
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index 4490372..ac38d7b 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -25,17 +25,27 @@
#include hw/isa/isa.h
#include qemu/main-loop.h
-/* #define DEBUG_DMA */
+/* #define DEBUG_DMA 1*/
#define dolog(...) fprintf (stderr, dma: __VA_ARGS__)
-#ifdef DEBUG_DMA
-#define linfo(...) fprintf (stderr, dma: __VA_ARGS__)
-#define ldebug(...) fprintf (stderr, dma: __VA_ARGS__)
-#else
-#define linfo(...)
-#define ldebug(...)
+#ifndef DEBUG_DMA
+#define DEBUG_DMA 0
#endif
+#define linfo(...) \
+do { \
+if(DEBUG_DMA) { \
+fprintf(stderr, dma: __VA_ARGS__); \
+} \
+} while (0)
+
+#define ldebug(...) \
+do { \
+if(DEBUG_DMA) { \
+fprintf(stderr, dma: __VA_ARGS__); \
+} \
+} while (0)
+
struct dma_regs {
int now[2];
uint16_t base[2];
diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index af26632..217b3f3 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -29,18 +29,23 @@
//
/* debug rc4030 */
-//#define DEBUG_RC4030
+//#define DEBUG_RC4030 1
//#define DEBUG_RC4030_DMA
#ifdef DEBUG_RC4030
-#define DPRINTF(fmt, ...) \
-do { printf(rc4030: fmt , ## __VA_ARGS__); } while (0)
static const char* irq_names[] = { parallel, floppy, sound, video,
network, scsi, keyboard, mouse, serial0, serial1 };
#else
-#define DPRINTF(fmt, ...)
+#define DEBUG_RC4030 0
#endif
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_RC4030) { \
+printf(rc4030: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
+
#define RC4030_ERROR(fmt, ...) \
do { fprintf(stderr, rc4030 ERROR: %s: fmt, __func__ , ## __VA_ARGS__); }
while (0)
--
1.7.10.4
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
28.04.2014 12:26, Marc Marí пишет:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
+
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_I82374) { \
+fprintf(stderr, I82374: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
Since this is the same pattern in all files touched, it can be generalized:
qemu-common.h:
#define QEMU_DPRINTF(cond,pfx,fmt,...) \
do { \
if (cond) {
fprintf(stderr, pfx # : fmt, ## __VA_ARGS__); \
} \
while(0)
This file (and other):
#ifndef DEBUG_I82374
#define DEBUG_I82374 0
#endif
#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_I82374, I82374, fmt, ##
__VA_ARGS__)
which is just 4 lines per file.
FWIW.
Thanks,
/mjt
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
Am 28.04.2014 um 10:26 hat Marc Marí geschrieben:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
+
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_I82374) { \
Coding style: There should be a space in if (
This seems to be pretty consistent across the whole series. In patch 2,
I also saw a while(0) without space. I won't comment on each instance
of this, just check your whole series for these things when you prepare
a v2.
+fprintf(stderr, I82374: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
#define BADF(fmt, ...) \
do { fprintf(stderr, i82374 ERROR: fmt , ## __VA_ARGS__); } while (0)
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index 4490372..ac38d7b 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -25,17 +25,27 @@
#include hw/isa/isa.h
#include qemu/main-loop.h
-/* #define DEBUG_DMA */
+/* #define DEBUG_DMA 1*/
#define dolog(...) fprintf (stderr, dma: __VA_ARGS__)
-#ifdef DEBUG_DMA
-#define linfo(...) fprintf (stderr, dma: __VA_ARGS__)
-#define ldebug(...) fprintf (stderr, dma: __VA_ARGS__)
-#else
-#define linfo(...)
-#define ldebug(...)
+#ifndef DEBUG_DMA
+#define DEBUG_DMA 0
#endif
+#define linfo(...) \
+do { \
+if(DEBUG_DMA) { \
+fprintf(stderr, dma: __VA_ARGS__); \
+} \
+} while (0)
+
Trailing whitespace.
+#define ldebug(...) \
+do { \
+if(DEBUG_DMA) { \
+fprintf(stderr, dma: __VA_ARGS__); \
+} \
+} while (0)
+
struct dma_regs {
int now[2];
uint16_t base[2];
diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index af26632..217b3f3 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -29,18 +29,23 @@
//
/* debug rc4030 */
-//#define DEBUG_RC4030
+//#define DEBUG_RC4030 1
//#define DEBUG_RC4030_DMA
Should DEBUG_RC4030_DMA be converted as well? It would probably be even
more useful than the other #defines because it enables a whole block of
code and not just a single printf.
Could be a separate patch (series), though.
#ifdef DEBUG_RC4030
-#define DPRINTF(fmt, ...) \
-do { printf(rc4030: fmt , ## __VA_ARGS__); } while (0)
static const char* irq_names[] = { parallel, floppy, sound, video,
network, scsi, keyboard, mouse, serial0, serial1 };
#else
-#define DPRINTF(fmt, ...)
+#define DEBUG_RC4030 0
#endif
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_RC4030) { \
+printf(rc4030: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
+
#define RC4030_ERROR(fmt, ...) \
do { fprintf(stderr, rc4030 ERROR: %s: fmt, __func__ , ## __VA_ARGS__); }
while (0)
Apart from the style issues and potential extension mentioned above,
the logic looks fine.
Reviewed-by: Kevin Wolf kw...@redhat.com
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
Hi Marc, On such a long series, it's usual to include a cover letter summarising the entire series. Its subject is PATCH 00/NN and can be generated by adding the --cover-letter switch to git send-email. Hand edit the file them send along with the autogenerated patches. On Mon, Apr 28, 2014 at 6:26 PM, Marc Marí marc.mari.barc...@gmail.com wrote: From: Marc Marí 5.markm...@gmail.com Modify debug macros as explained in https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html Wrap the body of the commit message to 72 chars, it's ok to blow the limit for a long URL IMO but you should at least line break between in https. Regards, Peter Signed-off-by: Marc Marí 5.markm...@gmail.com
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
2014-04-28 11:05 GMT+02:00 Michael Tokarev m...@tls.msk.ru:
28.04.2014 12:26, Marc Marí пишет:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
+
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_I82374) { \
+fprintf(stderr, I82374: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
Since this is the same pattern in all files touched, it can be
generalized:
qemu-common.h:
#define QEMU_DPRINTF(cond,pfx,fmt,...) \
do { \
if (cond) {
fprintf(stderr, pfx # : fmt, ## __VA_ARGS__); \
} \
while(0)
This file (and other):
#ifndef DEBUG_I82374
#define DEBUG_I82374 0
#endif
#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_I82374, I82374, fmt, ##
__VA_ARGS__)
which is just 4 lines per file.
FWIW.
Thanks,
/mjt
Approximately 1/3 cannot be forced into the same pattern (do not use
fprintf(stderr), mainly). I'll change the ones that are possible for v2.
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
Hi Marc,
Am 28.04.2014 10:26, schrieb Marc Marí:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
This is exactly how I told you not to do it in response to Peter C.'s
proposal. I had done so in my v1 [1] and it was rejected.
Instead it was concluded that we need:
//#define DEBUG_FOO
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED 1
#else
#define DEBUG_FOO_ENABLED 2
#endif
or something like that, so that #ifdef DEBUG_FOO still works as expected
further down the file.
Otherwise the patch looks good, except for the missing space after if.
Please add a cover letter 00/14 next time, so that general comments can
be placed there and 01/14 with its replies gets shown alongside 02/14.
Regards,
Andreas
[1] https://github.com/afaerber/qemu-cpu/commits/dprintf.v1
+
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_I82374) { \
+fprintf(stderr, I82374: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
#define BADF(fmt, ...) \
do { fprintf(stderr, i82374 ERROR: fmt , ## __VA_ARGS__); } while (0)
[snip]
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber afaer...@suse.de wrote:
Hi Marc,
Am 28.04.2014 10:26, schrieb Marc Marí:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
This is exactly how I told you not to do it in response to Peter C.'s
proposal. I had done so in my v1 [1] and it was rejected.
Instead it was concluded that we need:
//#define DEBUG_FOO
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED 1
#else
#define DEBUG_FOO_ENABLED 2
#endif
if you are going to go this way you probably want:
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED DEBUG_FOO
#else
#define DEBUG_FOO_ENABLED 0
#endif
So you can implement leveled debugging with just the one symbol. See
m25p80.c for an example of two level debugging using the if() system.
Regards,
Peter
or something like that, so that #ifdef DEBUG_FOO still works as expected
further down the file.
Otherwise the patch looks good, except for the missing space after if.
Please add a cover letter 00/14 next time, so that general comments can
be placed there and 01/14 with its replies gets shown alongside 02/14.
Regards,
Andreas
[1] https://github.com/afaerber/qemu-cpu/commits/dprintf.v1
+
+#define DPRINTF(fmt, ...) \
+do { \
+if(DEBUG_I82374) { \
+fprintf(stderr, I82374: fmt, ## __VA_ARGS__); \
+} \
+} while (0)
#define BADF(fmt, ...) \
do { fprintf(stderr, i82374 ERROR: fmt , ## __VA_ARGS__); } while (0)
[snip]
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
Am 28.04.2014 14:41, schrieb Peter Crosthwaite:
On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber afaer...@suse.de wrote:
Hi Marc,
Am 28.04.2014 10:26, schrieb Marc Marí:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
This is exactly how I told you not to do it in response to Peter C.'s
proposal. I had done so in my v1 [1] and it was rejected.
Instead it was concluded that we need:
//#define DEBUG_FOO
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED 1
#else
#define DEBUG_FOO_ENABLED 2
Oops, obviously this what meant to read 0, not 2, i.e. translating
absence of a constant to another constant that is assured to always
carry a value.
true/false may be an alternative for boolean logic.
#endif
if you are going to go this way you probably want:
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED DEBUG_FOO
#else
#define DEBUG_FOO_ENABLED 0
#endif
Is it guaranteed that #define DEBUG_FOO = DEBUG_FOO == 1? I assume so.
Otherwise this may be just applicable to code where you do this kind of
level-based distinction rather than presence/absence.
The real question to ask is, does the code have any #ifdef DEBUG_FOO, or
does the respective maintainer intend to use it that way? If not, then
your if (DEBUG_FOO) {...} is perfectly valid and makes more sense than
having ..._ENABLED be anything but a boolean. It's just totally unsafe
to assume this to work everywhere in one huge cross-maintainer
refactoring series, as my earlier series showed (which did locate and
update such #ifdef DEBUG_FOO code). The series becomes non-trivial then.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
2014-04-28 14:25 GMT+02:00 Andreas Färber afaer...@suse.de: This is exactly how I told you not to do it in response to Peter C.'s proposal. I had done so in my v1 [1] and it was rejected. In your response to the proposal, you sent me the link to your dprintf branch, which uses functions, no macros, so it left me a bit puzzled. Now it makes sense. For v2, I'll just create and use a pattern, as Michael said. Please add a cover letter 00/14 next time, so that general comments can be placed there and 01/14 with its replies gets shown alongside 02/14. Third time I've read that :D. I'll do next time Regards, Andreas Marc
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
Am 28.04.2014 13:21, schrieb Peter Crosthwaite: Hi Marc, On such a long series, it's usual to include a cover letter summarising the entire series. Its subject is PATCH 00/NN and can be generated by adding the --cover-letter switch to git send-email. Hand edit the file them send along with the autogenerated patches. On Mon, Apr 28, 2014 at 6:26 PM, Marc Marí marc.mari.barc...@gmail.com wrote: From: Marc Marí 5.markm...@gmail.com Modify debug macros as explained in https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html Wrap the body of the commit message to 72 chars, it's ok to blow the limit for a long URL IMO but you should at least line break between in https. Or better drop that reference at all. The archives are not kept forever, so any essential explanation should go into the commit message proper. In this case, a copiedpasted This avoids code bitrotting. or so would be sufficient as the pattern is obvious from looking at the diff and mentioned in the subject. On that matter, there is no dma subsystem. There's i82374, which is part of PReP. i8257 is a core PC thing, not sure if that means mst? Don't know about rc4030. Anyway, this series is clearly not divided by maintenance areas and not CC'ing the appropriate maintainers - Marc, please use: git config sendemail.cccmd scripts/get_maintainer.pl --nogit-fallback to CC the maintainers documented in MAINTAINERS file. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
2014-04-28 15:16 GMT+02:00 Andreas Färber afaer...@suse.de:
The real question to ask is, does the code have any #ifdef DEBUG_FOO, or
does the respective maintainer intend to use it that way? If not, then
your if (DEBUG_FOO) {...} is perfectly valid and makes more sense than
having ..._ENABLED be anything but a boolean. It's just totally unsafe
to assume this to work everywhere in one huge cross-maintainer
refactoring series, as my earlier series showed (which did locate and
update such #ifdef DEBUG_FOO code). The series becomes non-trivial then.
I just checked, and #ifdef is also used in some (not all), so this is not
good. Probably the best approach is adding _ENABLED
Marc
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber afaer...@suse.de wrote:
Am 28.04.2014 14:41, schrieb Peter Crosthwaite:
On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber afaer...@suse.de wrote:
Hi Marc,
Am 28.04.2014 10:26, schrieb Marc Marí:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
This is exactly how I told you not to do it in response to Peter C.'s
proposal. I had done so in my v1 [1] and it was rejected.
Instead it was concluded that we need:
//#define DEBUG_FOO
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED 1
#else
#define DEBUG_FOO_ENABLED 2
Oops, obviously this what meant to read 0, not 2, i.e. translating
absence of a constant to another constant that is assured to always
carry a value.
true/false may be an alternative for boolean logic.
#endif
if you are going to go this way you probably want:
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED DEBUG_FOO
#else
#define DEBUG_FOO_ENABLED 0
#endif
Is it guaranteed that #define DEBUG_FOO = DEBUG_FOO == 1? I assume so.
well -DDEBUG_FOO as a CFLAG comes with this == 1 guarantee which is
the prescribed usage. If you want to hack source with #define then
it's local to the code and your compile failure will quickly
straighten you out.
Otherwise this may be just applicable to code where you do this kind of
level-based distinction rather than presence/absence.
I prefer always level-capable - It's nice to be able to quickly add a
higher level of debug without applying a framework change.
The real question to ask is, does the code have any #ifdef DEBUG_FOO, or
does the respective maintainer intend to use it that way? If not, then
your if (DEBUG_FOO) {...} is perfectly valid and makes more sense than
having ..._ENABLED be anything but a boolean. It's just totally unsafe
to assume this to work everywhere in one huge cross-maintainer
refactoring series, as my earlier series showed (which did locate and
update such #ifdef DEBUG_FOO code). The series becomes non-trivial then.
Your change is a good idea perhaps even universally. Just I think
levels are a valuable feature.
Something else to consider as well will be converting these #define
DEBUG_FOOs further down the file to regular if() too where possible
for the same reasons as this series.
Regards,
Peter
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs
Am 28.04.2014 15:35, schrieb Peter Crosthwaite:
On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber afaer...@suse.de wrote:
Am 28.04.2014 14:41, schrieb Peter Crosthwaite:
On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber afaer...@suse.de wrote:
Hi Marc,
Am 28.04.2014 10:26, schrieb Marc Marí:
From: Marc Marí 5.markm...@gmail.com
Modify debug macros as explained in
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
Signed-off-by: Marc Marí 5.markm...@gmail.com
---
hw/dma/i82374.c | 17 ++---
hw/dma/i8257.c | 24 +---
hw/dma/rc4030.c | 13 +
3 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index dc7a767..fff4e6f 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,15 +24,18 @@
#include hw/isa/isa.h
-//#define DEBUG_I82374
+//#define DEBUG_I82374 1
-#ifdef DEBUG_I82374
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, i82374: fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
+#ifndef DEBUG_I82374
+#define DEBUG_I82374 0
#endif
This is exactly how I told you not to do it in response to Peter C.'s
proposal. I had done so in my v1 [1] and it was rejected.
Instead it was concluded that we need:
//#define DEBUG_FOO
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED 1
#else
#define DEBUG_FOO_ENABLED 2
Oops, obviously this what meant to read 0, not 2, i.e. translating
absence of a constant to another constant that is assured to always
carry a value.
true/false may be an alternative for boolean logic.
#endif
if you are going to go this way you probably want:
#ifdef DEBUG_FOO
#define DEBUG_FOO_ENABLED DEBUG_FOO
#else
#define DEBUG_FOO_ENABLED 0
#endif
Is it guaranteed that #define DEBUG_FOO = DEBUG_FOO == 1? I assume so.
well -DDEBUG_FOO as a CFLAG comes with this == 1 guarantee which is
the prescribed usage. If you want to hack source with #define then
it's local to the code and your compile failure will quickly
straighten you out.
Otherwise this may be just applicable to code where you do this kind of
level-based distinction rather than presence/absence.
I prefer always level-capable - It's nice to be able to quickly add a
higher level of debug without applying a framework change.
The real question to ask is, does the code have any #ifdef DEBUG_FOO, or
does the respective maintainer intend to use it that way? If not, then
your if (DEBUG_FOO) {...} is perfectly valid and makes more sense than
having ..._ENABLED be anything but a boolean. It's just totally unsafe
to assume this to work everywhere in one huge cross-maintainer
refactoring series, as my earlier series showed (which did locate and
update such #ifdef DEBUG_FOO code). The series becomes non-trivial then.
Your change is a good idea perhaps even universally. Just I think
levels are a valuable feature.
I wouldn't mind, but I suggest DEBUG_FOO or DEBUG_FOO_LEVEL naming then,
not DEBUG_FOO_ENABLED with numeric values please.
Something else to consider as well will be converting these #define
DEBUG_FOOs further down the file to regular if() too where possible
for the same reasons as this series.
Agreed, but either as follow-up patch/series for better review, or by
re-dividing this series along maintenance lines.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
