| Message ID | 1398673575-7773-1-git-send-email-marc.mari.barcelo@gmail.com |
|---|---|
| State | New |
| Headers | show |
28.04.2014 12:26, Marc Marí пишет: > From: Marc Marí <5.markmb.5@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.markmb.5@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
Am 28.04.2014 um 10:26 hat Marc Marí geschrieben: > From: Marc Marí <5.markmb.5@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.markmb.5@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 <kwolf@redhat.com>
28.04.2014 12:26, Marc Marí wrote: [] > +#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) > + #define ldebug linfo :) I wonder why they used diff. levels but the same condition. /mjt
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.barcelo@gmail.com> wrote: > From: Marc Marí <5.markmb.5@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.markmb.5@gmail.com>
2014-04-28 11:05 GMT+02:00 Michael Tokarev <mjt@tls.msk.ru>: > > 28.04.2014 12:26, Marc Marí пишет: > > From: Marc Marí <5.markmb.5@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.markmb.5@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.
Hi Marc, Am 28.04.2014 10:26, schrieb Marc Marí: > From: Marc Marí <5.markmb.5@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.markmb.5@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]
On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <afaerber@suse.de> wrote: > Hi Marc, > > Am 28.04.2014 10:26, schrieb Marc Marí: >> From: Marc Marí <5.markmb.5@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.markmb.5@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 >
Am 28.04.2014 14:41, schrieb Peter Crosthwaite: > On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <afaerber@suse.de> wrote: >> Hi Marc, >> >> Am 28.04.2014 10:26, schrieb Marc Marí: >>> From: Marc Marí <5.markmb.5@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.markmb.5@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
2014-04-28 14:25 GMT+02:00 Andreas Färber <afaerber@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
2014-04-28 15:16 GMT+02:00 Andreas Färber <afaerber@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
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.barcelo@gmail.com> wrote: >> From: Marc Marí <5.markmb.5@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 copied&pasted "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
On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 28.04.2014 14:41, schrieb Peter Crosthwaite: >> On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <afaerber@suse.de> wrote: >>> Hi Marc, >>> >>> Am 28.04.2014 10:26, schrieb Marc Marí: >>>> From: Marc Marí <5.markmb.5@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.markmb.5@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 >
Am 28.04.2014 15:35, schrieb Peter Crosthwaite: > On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber <afaerber@suse.de> wrote: >> Am 28.04.2014 14:41, schrieb Peter Crosthwaite: >>> On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <afaerber@suse.de> wrote: >>>> Hi Marc, >>>> >>>> Am 28.04.2014 10:26, schrieb Marc Marí: >>>>> From: Marc Marí <5.markmb.5@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.markmb.5@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
On Mon, Apr 28, 2014 at 11:34 PM, Andreas Färber <afaerber@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.barcelo@gmail.com> wrote: >>> From: Marc Marí <5.markmb.5@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 copied&pasted "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 >
On Mon, Apr 28, 2014 at 11:44 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 28.04.2014 15:35, schrieb Peter Crosthwaite: >> On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber <afaerber@suse.de> wrote: >>> Am 28.04.2014 14:41, schrieb Peter Crosthwaite: >>>> On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <afaerber@suse.de> wrote: >>>>> Hi Marc, >>>>> >>>>> Am 28.04.2014 10:26, schrieb Marc Marí: >>>>>> From: Marc Marí <5.markmb.5@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.markmb.5@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 >
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)