Re: [Xen-devel] [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

["Lai, Paul C" <paul.c.lai@xxxxxxxxx>]

[Paul] inlined....

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
Sent: Thursday, June 23, 2016 11:19 PM
To: Lai, Paul C <paul.c.lai@xxxxxxxxx>
Cc: Sahita, Ravi <ravi.sahita@xxxxxxxxx>; xen-devel 
<xen-devel@xxxxxxxxxxxxxxxxxxxx>
Subject: RE: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

>>> On 23.06.16 at 20:23, <paul.c.lai@xxxxxxxxx> wrote:

First of all - please don't top post.
[Paul] Understood

> I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else.  
> That would make reading/understanding of the macros more difficult.  
> This practice is common.  Also, If min & max are defined elsewhere, it 
> will be more likely to lead to mistakes/bugs.

I understand that, but I'm going to nak any patch that introduces clutter 
(which then will need to be retained forever) to the public interface. A 
possible compromise might be to frame these in __XEN__ conditionals, but this 
would need to be outweighed by the resulting benefits, which I'm not convinced 
of.

[Paul] Still waiting for a better idea.

> The use of "_min" and "_max" should be quite clear and is common use 
> in linux code; Yes, I know this is xen code and I see it here too.  If 
> there's a better way, please propose the better.  Maybe you're 
> suggesting the macro names should be all caps:
>   HVMOP_CMD_MIN, HVMOP_CMD_MAX
> ?  I was following the coding style within the file itself.

The problem isn't the use of min and max, but the selected names suggesting 
these are the minimum/maximum HVMOPs, whereas
- afaict - you really mean to frame the altp2m subset. Which btw, together with 
the above, points at another issue here: What if later another altp2m subop 
gets added, and especially when its new value ends up not being adjacent to the 
existing range?

[Paul] Again, waiting for a better idea.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel