Re: [Qemu-devel] [PATCH] hw/audio/sb16.c: Convert file to new logging API

[Programmingkid <programmingkidx@gmail.com>]

> On Jan 31, 2018, at 4:22 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> On Tue, Jan 30, 2018 at 10:30:46AM -0500, Programmingkid wrote:
>> 
>>> On Jan 30, 2018, at 4:41 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>> 
>>>> @@ -148,15 +142,16 @@ static int irq_of_magic (int magic)
>>>> #if 0
>>>> static void log_dsp (SB16State *dsp)
>>>> {
>>>> -    ldebug ("%s:%s:%d:%s:dmasize=%d:freq=%d:const=%d:speaker=%d\n",
>>>> -            dsp->fmt_stereo ? "Stereo" : "Mono",
>>>> -            dsp->fmt_signed ? "Signed" : "Unsigned",
>>>> -            dsp->fmt_bits,
>>>> -            dsp->dma_auto ? "Auto" : "Single",
>>>> -            dsp->block_size,
>>>> -            dsp->freq,
>>>> -            dsp->time_const,
>>>> -            dsp->speaker);
>>>> +    qemu_log_mask(LOG_UNIMP, "%s:%s:%d:%s:dmasize=%d:freq=%d:const=%d:"
>>>> +                  "speaker=%d\n",
>>>> +                  dsp->fmt_stereo ? "Stereo" : "Mono",
>>>> +                  dsp->fmt_signed ? "Signed" : "Unsigned",
>>>> +                  dsp->fmt_bits,
>>>> +                  dsp->dma_auto ? "Auto" : "Single",
>>>> +                  dsp->block_size,
>>>> +                  dsp->freq,
>>>> +                  dsp->time_const,
>>>> +                  dsp->speaker);
>>>> }
>>>> #endif
>>> 
>>> Hmm, dead code.  Any places which call log_dsp() ?
>> 
>> There are several places but they are all dead code. Do you want this removed?
> 
> So it is still used but not compiled in by default.
> 
> Looking at all of this again, I think "qemu_log_mask(LOG_UNIMP, ...)"
> should only be used in case there *really* is some soundblaster feature
> unimplemented.  Otherwise converting to a tracepoint is probably more
> useful, so the debug messages can be enabled/disabled individually at
> runtime.
> 
> The "#if 0" seems to be there to limit the flood of debug messages.  So
> once they all are converted to trace points this is not needed any more
> and I think we should just enable all this code.
> 
> dolog() seems to be mostly cases where the code warns about
> unimplemented featues or incomplete emulation, so using LOG_UNIMP looks
> sensible.

Simply replacing dolog() with "qemu_log_mask(LOG_UNIMP," seems like what was desired.

> ldebug() should probably trace points.

I need help with this. I tried replacing ldebug() with "qemu_log_mask(LOG_TRACE,", but that caused a lot of messages to be printed to the terminal. It is odd because I thought I had to use "-d trace" to make it print. Do you have another suggestion on what to replace ldebug() with? 

> Probably makes sense to split this patch up into a small series.

The first patch will be the replacing dolog() with qemu_log_mask(LOG_UNIMP. The second patch will be replacing ldebug() with what you suggest.