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
