[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Juergen Gross <JGross@xxxxxxxx>
  • From: Jan Beulich <JBeulich@xxxxxxxx>
  • Date: Mon, 29 Jul 2019 14:47:56 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1;spf=pass smtp.mailfrom=suse.com;dmarc=pass action=none header.from=suse.com;dkim=pass header.d=suse.com;arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3rufo48DdoblI1GrXyUK2qmKAwYPBL2MSekAtK2+2dg=; b=gZl2EEYnhO82xEvw+q6ZLdATJaPdbul7kmZiWMxrUxStF4TU9oz1uuEQ3NBCHQlgvcbU9giE3gtHSbvD/0hoLrsAtr1mJETVHGWCUxt383QskI74ocUUFNsz/dbERiQragE8njN60tOLJVRfXTV5+0I53yVwtLhA+GMctT5Kh+Wv50oEhjHX0xnsXTDE/0rM5d5qV0unoKXV8XkQ+ykzKBx/x4ek3+FGCF6bCL1N4a3wqX/SAW8YQoOAxbaz/arvvQmCmeQcrxhr3vbHmtiDoh7p18FF37b2+K/NWs3+b1/5gNygGJySlS9G30AXjcAW1JjMl3zEfMHPw2dsIuud/Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=F76swHluKHKXlcTc3wM70xQhV86MGd/QCRosTDzHdQWPf+Abnh27ZYXoegZRiMg0GqeXtFsBv1ZdCpi4sfouV0kaHfdb3PB0aTRozRBuNZ7s4BKHDOsneZL2v86vJRnj/VpjQY9MlClcA/yhPMPhMu7XJjGOA/XDtj3J9WP1Z+9FWKWBZARZ0c570ZJzgVcnDpBpextvCp8W/+b+WrRh2zhXrnTK4u2zISLFT6VRjD+xNVXGaN3PqqcN/otD7ztG/lKFQMj2YHflDPX7+iBo99C6W6riFsndC2YpsirZkyEQzJwCT0NRNWQv6HaWcAkAqhHKriRkIDUDL2pcDNSpnA==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=JBeulich@xxxxxxxx;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Julien Grall <julien.grall@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 29 Jul 2019 15:09:07 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVRSA2vhgAAF6AnUuE4HJXiRUWH6bhjJWAgAAM8DaAAAgJgIAAB+6RgAAFb4A=
  • Thread-topic: [PATCH 1/3] xen: move debugtrace coding to common/debugtrace.c

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.