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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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.

> 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;
    ...
}

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part