Re: [PATCH v2 17/35] xen/console: introduce consoled_is_enabled()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 04:00, Denis Mukhin wrote:
> On Tuesday, December 10th, 2024 at 6:31 AM, Jan Beulich <jbeulich@xxxxxxxx> 
> wrote:
> 
>>
>>
>> On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
>>
>>> --- a/xen/drivers/char/consoled.c
>>> +++ b/xen/drivers/char/consoled.c
>>> @@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
>>> static char buf[BUF_SZ + 1];
>>>
>>> /* Receives characters from a domain's PV console */
>>> -void consoled_guest_rx(void)
>>> +int consoled_guest_rx(void)
>>> {
>>> size_t idx = 0;
>>> XENCONS_RING_IDX cons, prod;
>>>
>>> if ( !cons_ring )
>>> - return;
>>> + return 0;
>>>
>>> spin_lock(&rx_lock);
>>>
>>> @@ -91,15 +91,17 @@ void consoled_guest_rx(void)
>>>
>>> out:
>>> spin_unlock(&rx_lock);
>>> +
>>> + return 0;
>>> }
>>>
>>> /* Sends a character into a domain's PV console */
>>> -void consoled_guest_tx(char c)
>>> +int consoled_guest_tx(char c)
>>> {
>>> XENCONS_RING_IDX cons, prod;
>>>
>>> if ( !cons_ring )
>>> - return;
>>> + return 0;
>>>
>>> cons = ACCESS_ONCE(cons_ring->in_cons);
>>> prod = cons_ring->in_prod;
>>> @@ -118,6 +120,7 @@ void consoled_guest_tx(char c)
>>>
>>> cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
>>>
>>> +
>>> /* Write to the ring before updating the pointer */
>>
>>
>> No excess blank lines please.
> 
> Fixed.
> 
>>
>>> @@ -125,6 +128,13 @@ void consoled_guest_tx(char c)
>>> notify:
>>> /* Always notify the guest: prevents receive path from getting stuck. */
>>> pv_shim_inject_evtchn(pv_console_evtchn());
>>> +
>>> + return 0;
>>> +}
>>
>>
>> For both of the functions - what use is it to make the functions return
>> a value, when all they'd ever return is zero (and callers don't care)?
> 
> Fixed.
> 
>> I'm also having a hard time seeing how this adjustment is related to ...
>>
>>> +bool consoled_is_enabled(void)
>>> +{
>>> + return pv_shim && pv_console;
>>> }
>>
>>
>> ... the introduction of this function (which by itself is probably fine).
> 
> That will be a cleanup in console driver on the code path I touched wrt 
> console
> focus switch.

Yet then please don't mix entirely independent things in a single patch.

Jan