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

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

Hi Michael,

On 4/21/2025 4:27 PM, Easwar Hariharan wrote:
> 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!
> 

Sorry this took so long, commercial work took up all of my focus the past few weeks. The rest
of the patches look good to me. If you could follow up with a v4 for the function naming,
Wei can pick this up for the 6.17 merge window.

Thanks,
Easwar (he/him)