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.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
