Re: [Xen-devel] [PATCH 1/3] xen: move debugtrace coding to common/debugtrace.c

[Jan Beulich <JBeulich@xxxxxxxx>]

On 29.07.2019 16:28, Juergen Gross wrote:
> On 29.07.19 16:00, Jan Beulich wrote:
>> On 29.07.2019 15:30, Juergen Gross wrote:
>>> On 29.07.19 14:45, Jan Beulich wrote:
>>>> On 28.07.2019 10:40, Juergen Gross wrote:
>>>>> -#endif /* !CONFIG_DEBUG_TRACE */
>>>>> -
>>>>> -
>>>>>     /*
>>>>>      * **************************************************************
>>>>>      * *************** Debugging/tracing/error-report ***************
>>>>
>>>> ... what about this one? There's only panic() between it and the next
>>>> such comment, and I don't think the "Debugging/tracing" part of it
>>>> are applicable (anymore).
>>>
>>> True. I'll remove the "Debugging/tracing" part.
>>>
>>>>
>>>>> --- a/xen/include/xen/console.h
>>>>> +++ b/xen/include/xen/console.h
>>>>> @@ -48,4 +48,8 @@ int console_resume(void);
>>>>>     extern int8_t opt_console_xen;
>>>>> +/* Issue string via serial line. */
>>>>> +extern int sercon_handle;
>>>>> +void sercon_puts(const char *s);
>>>>
>>>> I guess avoiding their exposure was one of the reasons the debug trace
>>>> code lived in the place you move it from. I'm unconvinced non-console
>>>> code is actually supposed to make use of either, but I'm not opposed
>>>> enough to nak the change. I don't think though the comment fits well
>>>> with the variable declaration.
>>>
>>> sercon_handle is used for calling serial_puts(), so maybe instead of
>>> directly using serial_puts() with sercon_handle I should add a wrapper
>>> to console.c (e.g. console_serial_puts())? It should be noted that
>>> serial_puts() is called only in case of debugtrace output toggled to go
>>> to the console. I guess using serial_puts() in that case is meant to
>>> avoid too many software layers when doing the output.
>>
>> Hmm, I'd rather expect this to be used to avoid doing anything else
>> sercon_puts() does besides calling serial_puts(). These other steps
>> are also why I think this is to remain a console internal interface.
> 
> To me it seems a little bit strange to have the buffer dumping using
> sercon_puts() while issuing the actual trace entries to console isn't
> using it.

I guess I agree.

>>> It would be
>>> possible to use sercon_puts() for that case, too, resulting in the
>>> inability to use debugtrace_printk() in the then additionally needed
>>> paths (or better: to use it with output redirected to console).
>>>
>>> sercon_puts() could use another wrapper, e.g. console_debug_puts().
>>>
>>> Would you like that better?
>>
>> Probably not. I wonder whether splitting out this code is really a
>> good step.
> 
> I'm not fighting for it, I just thought it would better be put into a
> file of its own.
> 
> In case you disagree and others are not pushing for separation I'm fine
> to drop this patch.

Well, I don't mind the separation as long as it's indeed properly
separated in the end.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel