diff mbox

[RFC,v2,04/22] block/pcache: add pcache debug build

Message ID 20160829171021.4902-5-pbutsykin@virtuozzo.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Butsykin Aug. 29, 2016, 5:10 p.m. UTC 
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
 block/pcache.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Eric Blake Sept. 8, 2016, 3:11 p.m. UTC | #1 
On 08/29/2016 12:10 PM, Pavel Butsykin wrote:
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  block/pcache.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block/pcache.c b/block/pcache.c
> index 74a4bc4..7f221d6 100644
> --- a/block/pcache.c
> +++ b/block/pcache.c
> @@ -28,6 +28,15 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qstring.h"
>  
> +#define PCACHE_DEBUG

Are you sure you want this left enabled?

> +
> +#ifdef PCACHE_DEBUG
> +#define DPRINTF(fmt, ...) \
> +        printf("%s:%s:%d "fmt, __FILE__, __func__, __LINE__, ## __VA_ARGS__)
> +#else
> +#define DPRINTF(fmt, ...) do { } while (0)
> +#endif

NACK.  This leads to bitrot when PCACHE_DEBUG is not defined.  Also, we
typically send debug to stderr, not stdout.  Instead, please follow the
lead of many other debug places, which do something similar to this (off
the top of my head, therefore untested):

#ifdef PCACHE_DEBUG
# define PCACHE_DEBUG_PRINT 1
#else
# define PCACHE_DEBUG_PRINT 0
#endif
#define DPRINTF(fmt, ...) \
    do { \
        if (PCACHE_DEBUG_PRINT) { \
            fprintf(stderr, ... __VA_ARGS__) \
        } \
    } while (0)
Pavel Butsykin Sept. 8, 2016, 3:49 p.m. UTC | #2 
On 08.09.2016 18:11, Eric Blake wrote:
> On 08/29/2016 12:10 PM, Pavel Butsykin wrote:
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>>   block/pcache.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/block/pcache.c b/block/pcache.c
>> index 74a4bc4..7f221d6 100644
>> --- a/block/pcache.c
>> +++ b/block/pcache.c
>> @@ -28,6 +28,15 @@
>>   #include "qapi/error.h"
>>   #include "qapi/qmp/qstring.h"
>>
>> +#define PCACHE_DEBUG
>
> Are you sure you want this left enabled?

No.

>> +
>> +#ifdef PCACHE_DEBUG
>> +#define DPRINTF(fmt, ...) \
>> +        printf("%s:%s:%d "fmt, __FILE__, __func__, __LINE__, ## __VA_ARGS__)
>> +#else
>> +#define DPRINTF(fmt, ...) do { } while (0)
>> +#endif
>
> NACK.  This leads to bitrot when PCACHE_DEBUG is not defined.  Also, we
> typically send debug to stderr, not stdout.  Instead, please follow the
> lead of many other debug places, which do something similar to this (off
> the top of my head, therefore untested):
>
> #ifdef PCACHE_DEBUG
> # define PCACHE_DEBUG_PRINT 1
> #else
> # define PCACHE_DEBUG_PRINT 0
> #endif
> #define DPRINTF(fmt, ...) \
>      do { \
>          if (PCACHE_DEBUG_PRINT) { \
>              fprintf(stderr, ... __VA_ARGS__) \
>          } \
>      } while (0)
>

OK, thanks!
Pavel Butsykin Sept. 8, 2016, 4:05 p.m. UTC | #3 
On 08.09.2016 18:49, Pavel Butsykin wrote:
> On 08.09.2016 18:11, Eric Blake wrote:
>> On 08/29/2016 12:10 PM, Pavel Butsykin wrote:
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> ---
>>>   block/pcache.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/block/pcache.c b/block/pcache.c
>>> index 74a4bc4..7f221d6 100644
>>> --- a/block/pcache.c
>>> +++ b/block/pcache.c
>>> @@ -28,6 +28,15 @@
>>>   #include "qapi/error.h"
>>>   #include "qapi/qmp/qstring.h"
>>>
>>> +#define PCACHE_DEBUG
>>
>> Are you sure you want this left enabled?
>
> No.
>
>>> +
>>> +#ifdef PCACHE_DEBUG
>>> +#define DPRINTF(fmt, ...) \
>>> +        printf("%s:%s:%d "fmt, __FILE__, __func__, __LINE__, ##
>>> __VA_ARGS__)
>>> +#else
>>> +#define DPRINTF(fmt, ...) do { } while (0)
>>> +#endif
>>
>> NACK.  This leads to bitrot when PCACHE_DEBUG is not defined.  Also, we
>> typically send debug to stderr, not stdout.  Instead, please follow the
>> lead of many other debug places, which do something similar to this (off
>> the top of my head, therefore untested):
>>
>> #ifdef PCACHE_DEBUG
>> # define PCACHE_DEBUG_PRINT 1
>> #else
>> # define PCACHE_DEBUG_PRINT 0
>> #endif
>> #define DPRINTF(fmt, ...) \
>>      do { \
>>          if (PCACHE_DEBUG_PRINT) { \
>>              fprintf(stderr, ... __VA_ARGS__) \
>>          } \
>>      } while (0)
>>
>
> OK, thanks!

Can I replace DPRINTFs on tracepoints?
Eric Blake Sept. 8, 2016, 6:42 p.m. UTC | #4 
On 09/08/2016 11:05 AM, Pavel Butsykin wrote:
>>>
>>> #ifdef PCACHE_DEBUG
>>> # define PCACHE_DEBUG_PRINT 1
>>> #else
>>> # define PCACHE_DEBUG_PRINT 0
>>> #endif
>>> #define DPRINTF(fmt, ...) \
>>>      do { \
>>>          if (PCACHE_DEBUG_PRINT) { \
>>>              fprintf(stderr, ... __VA_ARGS__) \
>>>          } \
>>>      } while (0)
>>>
>>
>> OK, thanks!
> 
> Can I replace DPRINTFs on tracepoints?
> 

Yes, tracepoints are even better than conditional printfs. They are a
bit trickier to set up, but more powerful in the end.  And they are
equally immune to the bitrot that I was trying to prevent with your
definition of DPRINTF.
diff mbox

Patch

diff --git a/block/pcache.c b/block/pcache.c
index 74a4bc4..7f221d6 100644
--- a/block/pcache.c
+++ b/block/pcache.c
@@ -28,6 +28,15 @@ 
 #include "qapi/error.h"
 #include "qapi/qmp/qstring.h"
 
+#define PCACHE_DEBUG
+
+#ifdef PCACHE_DEBUG
+#define DPRINTF(fmt, ...) \
+        printf("%s:%s:%d "fmt, __FILE__, __func__, __LINE__, ## __VA_ARGS__)
+#else
+#define DPRINTF(fmt, ...) do { } while (0)
+#endif
+
 typedef struct PrefCacheAIOCB {
     BlockAIOCB common;