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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

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). 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. 

> 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.

thanks
Mukesh

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