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

[Programmingkid <programmingkidx@gmail.com>]

> On Jan 31, 2018, at 4:43 PM, Programmingkid <programmingkidx@gmail.com> wrote:
> 
>> 
>> 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.

After doing a little more research I found this code in log.c:

/* enable or disable low levels log */
void qemu_set_log(int log_flags)
{
    qemu_loglevel = log_flags;
#ifdef CONFIG_TRACE_LOG
    qemu_loglevel |= LOG_TRACE;
#endif

It looks like no matter what the trace level that is set, LOG_TRACE will always be used. I think this might be a mistake. The configure script will set CONFIG_TRACE_LOG if the backend is log. This appears to be the default behavior. This will lead to LOG_TRACE being set in util/log.c.