Re: [PATCH 3/5] sched/arinc653: Clean up function definitions

[Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>]

On 9/17/2020 10:40 AM, Dario Faggioli wrote:
>On Thu, 2020-09-17 at 10:09 +0200, Jan Beulich wrote:
>> On 16.09.2020 20:18, Jeff Kubascik wrote:
>>> --- a/xen/common/sched/arinc653.c
>>> +++ b/xen/common/sched/arinc653.c
>>> @@ -119,10 +119,9 @@ static int dom_handle_cmp(const
>>> xen_domain_handle_t h1,
>>>      return memcmp(h1, h2, sizeof(xen_domain_handle_t));
>>>  }
>>>
>>> -static struct sched_unit *find_unit(
>>> -    const struct scheduler *ops,
>>> -    xen_domain_handle_t handle,
>>> -    int unit_id)
>>> +static struct sched_unit *find_unit(const struct scheduler *ops,
>>> +                                    xen_domain_handle_t handle,
>>> +                                    int unit_id)
>>>  {
>>
>> Just fyi, afaict we consider both variants legitimate style as far
>> as Xen as a whole is concerned; I'm unaware of scheduler code
>> specific restrictions (but I'll be happy to be corrected if I'm
>> wrong with this).
>>
>No, you're right, there aren't any additional restrictions. And, as
>many other subsystems, scheduling code is not always 100% consistent.
>There's quite a mix of style. E.g., there are both examples of the
>style that this hunk above is changing and of the one that the patch is
>changing it to.
>
>So I also see limited need for doing it. But of course it's Josh's and
>Stweart's call, I guess.

If that's the case, then I'm thinking keeping the previous style would be
preferred. I'll switch it back.

>> Instead what I'm wondering by merely seeing this piece of code is
>> whether unit_id really can go negative. If not (as would be the
>> common case with IDs), it would want converting to unsigned int,
>> which may be more important than the purely typographical
>> adjustment done here.
>>
>Yep, it's defined as `unsigned int` in `struct sched_unit`.
>
>So this indeed would be valuable. And while you're there, this probably
>applies here as well:
>
>/**
> * The sched_entry_t structure holds a single entry of the
> * ARINC 653 schedule.
> */
>typedef struct sched_entry_s
>{
>    /* dom_handle holds the handle ("UUID") for the domain that this
>     * schedule entry refers to. */
>    xen_domain_handle_t dom_handle;
>    /* unit_id holds the UNIT number for the UNIT that this schedule
>     * entry refers to. */
>    int                 unit_id;
>    ...
>}

Agreed. I'll make this change.

-Jeff