Re: [PATCH v2 4/5] xen/version: Fold build_id handling into xenver_varbuf_op()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2023 23:14, Andrew Cooper wrote:
> On 16/01/2023 4:14 pm, Jan Beulich wrote:
>> On 14.01.2023 00:08, Andrew Cooper wrote:
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -476,9 +476,22 @@ static long xenver_varbuf_op(int cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>      struct xen_varbuf user_str;
>>>      const char *str = NULL;
>>>      size_t sz;
>>> +    int rc;
>> Why is this declared here, yet ...
>>
>>>      switch ( cmd )
>>>      {
>>> +    case XENVER_build_id:
>>> +    {
>>> +        unsigned int local_sz;
>> ... this declared here?
> 
> Because rc is more likely to be used outside in the future, and...
> 
>>  Both could live in switch()'s scope,
> 
> ... this would have to be reverted tree-wide to use
> trivial-initialisation hardening, which we absolutely should be doing by
> default already.

Interesting; thanks for giving some background.

> I was sorely tempted to correct xen_build_id() to use size_t, but I
> don't have time to correct everything which is wrong here.  That can
> wait until later clean-up.
> 
> Alternatively, this is a pattern we have in quite a few places,
> returning a {ptr, sz} pair.  All architectures we compile for (and even
> x86 32bit given a suitable code-gen flag) are capable of returning at
> least 2 GPRs worth of data (ARM can do 4), so switching to some kind of
> 
> struct pair {
>     void *ptr;
>     size_t sz;
> };
> 
> return value would improve the code generation (and performance for that
> matter) across the board by avoiding unnecessary spills of
> pointers/sizes/secondary error information to the stack.

Sounds like a possible plan (ideally we'd check with RISC-V and PPC as
well in this regard, as these look to be the two upcoming new ports).

> The wins for hvm get/set_segment_register() modest bug absolutely
> worthwhile (and I notice I still haven't got those patches published. 
> /sigh).

Did I ever post my 128-bit retval extension for altcall?

>>> +        rc = xen_build_id((const void **)&str, &local_sz);
>>> +        if ( rc )
>>> +            return rc;
>>> +
>>> +        sz = local_sz;
>>> +        goto have_len;
>> Personally I certainly dislike "goto" in general, and I thought the
>> common grounds were to permit its use in error handling (only).
> 
> That's not written in CODING_STYLE, nor has it (to my knowledge) ever
> been an expressed view on xen-devel.

It has been, but that was years ago.

> I don't use goto's gratuitously, and this one isn't.  Just try and write
> this patch without a goto and then judge which version is cleaner/easier
> to follow.

I wouldn't have given my R-b if I didn't realize that.

Jan