Re: [Xen-devel] [PATCH 6/18 V2]: PVH xen: Introduce PVH guest type

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.03.13 at 02:13, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Tue, 19 Mar 2013 08:48:53 +0000
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 19.03.13 at 01:21, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > On Mon, 18 Mar 2013 11:54:29 +0000
>> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>>And as pointed out elsewhere - your whole series is in need of
>>making it conform to coding conventions.
> 
> For the most part I looked at existing code in the same or different 
> module for coding conventions.

Which is the right approach, except that you should ignore bad
examples (namely "real" Xen sources not conforming to the
conventions, where "real" means not cloned from another source,
which most frequently would have been Linux).

>> >> >  #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
>> >> >  
>> >> > -#define is_hvm_domain(d) ((d)->is_hvm)
>> >> > +#define is_hvm_domain(d) ((d)->guest_type == hvm_guest)
>> >> >  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
>> >> > +#define is_pvh_domain(d) ((d)->guest_type == pvh_guest)
>> >> > +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
>> >> >  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>> >> >                             cpumask_weight((v)->cpu_affinity) ==
>> >> > 1) +#define is_hvm_or_pvh_domain(d) (is_hvm_domain(d) ||
>> >> > is_pvh_domain(d)) +#define is_hvm_or_pvh_vcpu(v)
>> >> > (is_hvm_or_pvh_domain(v->domain))
>> >> 
>> >> These surely can have better names, if they're needed at all:
>> >> Wouldn't !is_pv_domain() do what you need?
>> > 
>> > Nop, that's more confusing, since PVH is a PV domain. So, I suggest
>> > we leave it as is. is_hvm_or_pvh_domain is nicely readable, what
>> > name do you suggest?
>> 
>> No. The three kinds should be fully distinct, such that when
>> meaning one you can use is_xyz_domain() and when meaning
>> two, you can use !is_abc_domain().
> 
>> is_hvm_or_pvh_domain() isn't nicely readable to me, in particular
>> because this kind of naming doesn't scale. And it's certainly more
>> typing than !is_pv_domain().
> 
> Since, pvh is a pv domain, I don't like using pv_guest for non PVH PV.
> But perhaps I could use the name pv_mmu and have something like
> following:
> 
> enum guest_type { is_mmu_pv, is_pvh_pv, is_hvm } guest_type;
> 
> Then:  is_hvm_or_pvh_domain()  becomes:  !is_mmu_pv().
> 
> Alternative to is_mmu_pv: is_pure_pv, is_orig_pv, ....

These are all ugly, and I don't see why the triplet I suggested
(is_pv, is_pvh, and is_hvm), including their intended use, wouldn't
be acceptable.

Jan


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