Re: [Xen-devel] [PATCH 1/18 V2]: PVH xen: turn gdb_frames/gdt_ents into union

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.13 at 01:04, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 18 Mar 2013 11:21:31 +0000
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 16.03.13 at 01:14, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > --- a/xen/include/public/arch-x86/xen.h
>> > +++ b/xen/include/public/arch-x86/xen.h
>> > @@ -170,7 +170,20 @@ struct vcpu_guest_context {
>> >      struct cpu_user_regs user_regs;         /* User-level CPU
>> > registers     */ struct trap_info trap_ctxt[256];        /* Virtual
>> > IDT                  */ unsigned long ldt_base, ldt_ents;       /*
>> > LDT (linear address, # ents) */ +#if __XEN_INTERFACE_VERSION__ <
>> > 0x00040300 unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
>> > frames, # ents) */ +#else
>> > +    union {
>> > +        struct {
>> > +            /* GDT (machine frames, # ents) */
>> > +            unsigned long gdt_frames[16], gdt_ents;
>> > +        } pv;
>> > +        struct {
>> > +            /* PVH: GDTR addr and size */   
>> > +            unsigned long gdtaddr, gdtsz;
>> > +        } pvh;
>> > +    } u;
>> 
>> Leaving aside the line wrapping issue already pointed out by
>> others, I can only repeat that I don't see why you would name
>> the union as badly as "u" when the obvious name would be "gdt".
>> 
>> With that, I can further more only repeat that dropping the
>> "gdt_" and "gdt" prefixes on the names would be much preferred.
> 
> Ok, I thought we had resolved that in V1 when I said it was recommended
> during linux review to have it that way (linux patches were posted on
> xen-devel).

I know you thought so, but I repeatedly said that posting the
Linux side first was a mistake.

> But I guess it has to be your way now. So I'll change it as
> much as I dislike making code harder to read by naming variables that
> can't be easily grep'd/cscope'd.
> 
> Since gdt_frames/gdt_ents is pre-existing code, I'll just change gdtaddr
> to addr, and gdtsz to sz or call it limit. 

In the old interface version code that obviously has to be that
way. But in the new interface version code, even the gdt_
prefixes should go away.

>> And finally I question the usefulness of having what is currently
>> named "gdtsz" be an "unsigned long" when this can't exceed a
>> 16-bit quantity (the more if you used a limit value here rather
>> than a size, just like hardware does).
> 
> Ok, I will change it to unsigned short.

uint16_t please. Let's try to use types we can be certain we know
the width of now and forever (and at once not make things harder
for consumers of the headers we might not even be aware of).

Jan


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