Re: [PATCH v3 1/7] Drivers: hv: Introduce hv_hvcall_*() functions for hypercall arguments

[Easwar Hariharan <eahariha@linux.microsoft.com>]

On 4/21/2025 2:24 PM, Michael Kelley wrote:
> From: Easwar Hariharan <eahariha@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)