Re: [PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.04.2021 20:24, Julien Grall wrote:
> Hi Jan,
> 
> On 06/04/2021 09:07, Jan Beulich wrote:
>> On 05.04.2021 17:57, Julien Grall wrote:
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> Both name and opt_name are pointing to literal string. So mark both of
>>> the fields as const.
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> albeit ...
>>
>>> --- a/xen/common/sched/private.h
>>> +++ b/xen/common/sched/private.h
>>> @@ -272,8 +272,8 @@ static inline spinlock_t 
>>> *pcpu_schedule_trylock(unsigned int cpu)
>>>   }
>>>   
>>>   struct scheduler {
>>> -    char *name;             /* full name for this scheduler      */
>>> -    char *opt_name;         /* option name for this scheduler    */
>>> +    const char *name;       /* full name for this scheduler      */
>>> +    const char *opt_name;   /* option name for this scheduler    */
>>
>> ... I'd like to suggest considering at least the latter to become
>> an array instead of a pointer - there's little point wasting 8
>> bytes of storage for the pointer when the strings pointed to are
>> all at most 9 bytes long (right now; I don't expect much longer
>> ones to appear).
> 
> I have tried this simple/dumb change on top of my patch:
> 
> diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
> index a870320146ef..ab2236874217 100644
> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -273,7 +273,7 @@ static inline spinlock_t 
> *pcpu_schedule_trylock(unsigned int cpu)
> 
>   struct scheduler {
>       const char *name;       /* full name for this scheduler      */
> -    const char *opt_name;   /* option name for this scheduler    */
> +    const char opt_name[9]; /* option name for this scheduler    */
>       unsigned int sched_id;  /* ID for this scheduler             */
>       void *sched_data;       /* global data pointer               */
>       struct cpupool *cpupool;/* points to this scheduler's pool   */
> 
> GCC will throw an error:
> 
> core.c: In function ‘scheduler_init’:
> core.c:2987:17: error: assignment of read-only variable ‘ops’
>               ops = *schedulers[i];
>                   ^
> core.c:2997:21: error: assignment of read-only variable ‘ops’
>                   ops = *schedulers[i];
>                       ^
> 
> I don't particularly want to drop the const. So the code would probably 
> need some rework.

What's wrong with not having the const when the field is an array?
The more that all original (build-time, i.e. contain in the binary)
instances of the struct are already const as a whole?

Jan