On 4/21/2025 2:24 PM, Michael Kelley wrote:
> From: Easwar Hariharan <eahar...@linux.microsoft.com> Sent: Monday, April 21,
> 2025 1:41 PM
>>>
<snip>
>>>
>>
>> This is very cool, thanks for taking the time! I think the function naming
>> could be more intuitive, e.g. hv_setup_*_args(). I'd not block it for that
>> reason,
>> but would be super happy if you would update it. What do you think?
>>
>
> I'm not particularly enamored with my naming scheme, but it was the
> best I could come up with. My criteria were:
>
> * Keep the length reasonably short to not make line length problems
> any worse
> * Distinguish the input args only, input & output args, and array versions
I think the in/inout/array scheme you have does this nicely
> * Use the standard "hv_" prefix for Hyper-V related code
>
> Using "setup" instead of "hvcall" seems like an improvement to me, and
> it is 1 character shorter. The "hv" prefix would be there, but they wouldn't
> refer specifically to hypercalls. I would not add "_args" on the end because
> that's another 5 characters in length. So we would have:
>
> * hv_setup_in()
> * hv_setup_inout()
> * hv_setup_in_array()
> * hv_setup_inout_array()
> * hv_setup_in_batch_size() [??]
>
> Or maybe, something like this, or similar, which picks up the "args" string,
> but not "setup":
>
> * hv_hcargs_in()
> * hv_hcargs_inout()
> * hv_hcargs_in_array()
> * hv_hcargs_inout_array()
> * hv_hcargs_in_batch_size() [??]
>
> I'm very open to any other ideas because I'm not particularly
> happy with the hv_hvcall_* approach.
Between the two presented here, I prefer option 1, with the "setup" verb
because it tells you
inline what the function will do. I agree that the "args" is unnecessary
because most
hypercall args are named hv_{input, output}_* and are clearly arguments to
hv_do_hypercall()
and friends.
Since hv_setup*() will normally be followed shortly after by hv_do_hypercall(),
I don't
see a problem with not referring specifically to hypercalls, it should be clear
in context.
For hv_hvcall_in_batch_size(), I think it serves a fundamentally different
function than the
other wrappers and doesn't need to follow the "setup" pattern. Instead it could
be named
hv_get_input_batch_size() for the same length and similarly tell you its
purpose inline.
I am continuing to review the rest of the series, sorry for the delay, and
thank you for your
patience!
Thanks,
Easwar (he/him)
