Re: [Xen-devel] [PATCH] x86/pv: Rename pv/ro-page-fault.c to pv/emul-ro-page-fault.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.18 at 19:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/02/18 13:01, Jan Beulich wrote:
>>>>> On 05.02.18 at 13:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 05/02/18 08:57, Jan Beulich wrote:
>>>>>>> On 02.02.18 at 17:58, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> To match all our other emulation handling.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>>> ---
>>>>>  xen/arch/x86/pv/Makefile                                  | 2 +-
>>>>>  xen/arch/x86/pv/{ro-page-fault.c => emul-ro-page-fault.c} | 2 +-
>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>  rename xen/arch/x86/pv/{ro-page-fault.c => emul-ro-page-fault.c} (99%)
>>>> When this file was introduced, iirc I had specifically asked to drop
>>>> the pointless emul- prefix. If you want to make things consistent
>>>> again, please instead drop the emul- prefixes of the other files.
>>> No.
>>>
>>> First of all, this file is the most recent to come into existence,
>>> around 3 months after the others.
>> Right - it was too late for me to realize the needlessly long names
>> in those earlier code movement patches.
> 
> That is a very subjective point of view which I don't agree with.
> 
> Naming is all to do with conveying meaning, and shorter isn't
> necessarily better.
> 
>>
>>> The point of naming things in a consistent fashion is for the benefit of
>>> humans, and having the emulation related functionality logically grouped
>>> is a benefit, not a detriment.
>> They're all quite well grouped now already by being in pv/.
> 
> That is not the relevant grouping.  Most of our emulation based logic
> has an emul- prefix and this file is an odd one out.
> 
> Naming the files without their emul- prefix leaves them with no context
> as to what they are doing.  "gate-op.c" or "invl-op.c" are far less
> obvious to their purpose than "emul-gate-op.c" and "emul-invl-op.c".

A little less obvious, yes, but far? Furthermore, taking the file you
want to rename into account, "emul-gate-op" indeed stands for
"emulate gate operations" (likewise for the inv and priv infixes),
while "emul-ro-page-fault" does _not_ mean "emulate r/o page
faults", as that would mean we emulate something to _produce_
r/o page faults. Instead we _handle_ r/o page faults here in
order to emulate certain write operations the guest does. In that
sense, the name ought to be e.g. "emul-write-op". That, however,
would break the connection to e.g. the "ptwr" abbreviation we
continue to use, so please don't take this as a suggestion.

IOW - I can live with your objection to rename the existing
emul-*.c files (despite thinking that for name completion they're
badly named; the emul- prefix would better have been an -emul
suffix, and leaving aside mere name length), but I continue to
dislike the new name of the file here.

>> Otherwise do you mean to also change e.g. gpr_switch.S to emul-gpr_switch.S?
> 
> This is extra special, and as soon as I can figure out how it actually
> works, I plan to replace it with something comprehensive.

;-)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel