Re: [Xen-devel] [RFC PATCH V3 01/12] xen/mem_event: Cleanup of mem_event structures

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Feb 2, 2015 at 6:19 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2015-01-29 at 22:46 +0100, Tamas K Lengyel wrote:
>> From: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>
>> The public mem_event structures used to communicate with helper applications 
>> via
>> shared rings have been used in different settings. However, the variable 
>> names
>> within this structure have not reflected this fact, resulting in the reuse of
>> variables to mean different things under different scenarios.
>>
>> This patch remedies the issue by clearly defining the structure members 
>> based on
>> the actual context within which the structure is used.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>> ---
>> v3: Add padding to mem_event structures.
>>     Add version field to mem_event structures and checks for it.
>
> The addition of versioning is worth mentioning in the commit message
> IMHO.

OK, will do!

>
> I'm not going to review the tools changes in detail, since I suppose
> they are very much mechanical.

Indeed, no logic changes in this patch.

>
>> -            if ( req.gfn > paging->max_pages )
>> +            if ( req.data.mem_paging.gfn > paging->max_pages )
>>              {
>> -                ERROR("Requested gfn %"PRIx64" higher than max_pages %x\n", 
>> req.gfn, paging->max_pages);
>> +                ERROR("Requested gfn %"PRIx64" higher than max_pages %x\n", 
>> req.data.mem_paging.gfn, paging->max_pages);
>
> If you could wrap some of these lines which have become even more overly
> long here as you change them then that would be much appreciated.

OK!

>
>> +    union {
>> +        struct mem_event_paging_data                mem_paging;
>> +        struct mem_event_sharing_data               mem_sharing;
>> +        struct mem_event_mem_access_data            mem_access;
>> +        struct mem_event_mov_to_cr_data             mov_to_cr;
>> +        struct mem_event_mov_to_msr_data            mov_to_msr;
>> +        struct mem_event_software_breakpoint_data   software_breakpoint;
>> +        struct mem_event_singlestep_data            singlestep;
>> +    } data;
>
> We typically call these unions "u" but that's up to the hypervisor guys
> really.
>
> Ian.
>

Thanks,
Tamas

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