[Spice-devel] [RFC PATCH qxl-wddm-dod 02/28] Add printer class to dump debug print statements to kernel debugger output

[Frediano Ziglio]

> 
> From: Sandy Stutsman <sstutsma at redhat.com>
> 
> Allows the usage of Kd_IHVVIDEO_Mask to control print level while debugging
> ---
>  qxldod/driver.cpp | 24 +++++++++++++++++++++++-
>  qxldod/driver.h   | 22 ++++++++++++++++++----
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/qxldod/driver.cpp b/qxldod/driver.cpp
> index 4d1913c..2098421 100755
> --- a/qxldod/driver.cpp
> +++ b/qxldod/driver.cpp
> @@ -667,7 +667,29 @@ void DebugPrintFunc(const char *format, ...)
>      va_start(list, format);
>      vDbgPrintEx(DPFLTR_DEFAULT_ID, 9 | DPFLTR_MASK, format, list);
>  }
> +ULONG kd_debug_printer::_xlate [] = { 0, 0, 1, 2, 3 };
> +

This can be const.
Also can be a static inside kd_debug_printer::kd_debug_printer.

> +kd_debug_printer::kd_debug_printer(ULONG level) : _off(FALSE)
> +{
> +    if (!level || level > 5) {
> +        _off = TRUE;
> +        _level = 0xffffffff;
> +
> +    }
> +    else {
> +        _level = _xlate[level - 1];
> +    }
> +}
> +
> +void kd_debug_printer::print(const char * fmt, ...)
> +{
> +    va_list list;
> +    va_start(list, fmt);
> +    if (_off) {
> +        return;
> +    }
> +    vDbgPrintEx(DPFLTR_IHVVIDEO_ID, _level, fmt, list);


If you call va_start you should also call va_end, so perhaps

   if (_off) {
       return;
   }
   va_list list;
   va_start(list, fmt);
   vDbgPrintEx(DPFLTR_IHVVIDEO_ID, _level, fmt, list);
   va_end(list);

> +}
>  #endif
>  
>  #pragma code_seg(pop) // End Non-Paged Code
> -
> diff --git a/qxldod/driver.h b/qxldod/driver.h
> index e64c098..d7f7bf8 100755
> --- a/qxldod/driver.h
> +++ b/qxldod/driver.h
> @@ -208,15 +208,29 @@ DodSystemDisplayWrite(
>      _In_  UINT  PositionY);
>  
>  #if DBG
> +class kd_debug_printer
> +{
> +public:
> +    kd_debug_printer(ULONG level);
> +    void print(const char * fmt, ...);
> +private:
> +    ULONG _level;
> +    BOOLEAN _off;

why not bool?
I think that _off == TRUE it's the same as _level == 0xffffffff.

> +    static ULONG _xlate[6];

_xlate contains just 5 items.

> + };
>  
>  extern int nDebugLevel;
>  void DebugPrintFuncSerial(const char *format, ...);
>  
> -void DebugPrintFunc(const char *format, ...);
> +void DebugPrintFunc(const char *format, ...);
> +
> +#define DbgPrint(level, line)           \
> +    if (level > nDebugLevel) {}         \
> +    else {                              \
> +       DebugPrintFuncSerial line;       \
> +     }                                  \
> +     kd_debug_printer(level).print line
>

I would use the classic do {} while(0) trick instead of the
empty statement but it's just question of style.
  
> -#define DbgPrint(level, line) \
> -    if (level > nDebugLevel) {} \
> -    else DebugPrintFuncSerial line
>  #else
>  #define DbgPrint(level, line)
>  #endif

Why not using variadic macros?
But probably does not fit in this patch anyway.

Frediano