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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/01/2023 4:14 pm, Jan Beulich wrote:
> On 14.01.2023 00:08, Andrew Cooper wrote:
>> struct xen_build_id and struct xen_varbuf are identical from an ABI point of
>> view, so XENVER_build_id can reuse xenver_varbuf_op() rather than having it's
>> own almost identical copy of the logic.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

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


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.

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

>> +        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.

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.

~Andrew