Re: [Xen-devel] [PATCH V4 01/13] xen/mem_event: Cleanup of mem_event structures

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

On Tue, Feb 10, 2015 at 5:17 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx> 02/10/15 2:51 PM >>>
> On Tue, Feb 10, 2015 at 1:52 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 09.02.15 at 19:53, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>>> @@ -598,6 +600,12 @@ int mem_sharing_sharing_resume(struct domain *d)
>>>>      {
>>>>          struct vcpu *v;
>>>>
>>>> +        if ( rsp.version != MEM_EVENT_INTERFACE_VERSION )
>>>> +        {
>>>> +            gdprintk(XENLOG_WARNING, "mem_event interface version 
>>>> mismatch!\n");
>>>
>>> Why gdprintk()?
>>
>>Is that only for debug cases?
>
> I'm intending to propose compiling out alll dprintk() and gdprintk() instance 
> in
> non-debug builds. Right now they're preferable when the message is so terse
> that identifying its origin without file name and line number is difficult. 
> Clearly
> any non-debug messages shouldn't be of such poor quality.

I willwrap it into #ifndef NDEBUG as it is really only for debugging.

>
>>>> @@ -1310,18 +1322,19 @@ void p2m_mem_paging_resume(struct domain *d)
>>>>          /* Fix p2m entry if the page was not dropped */
>>>>          if ( !(rsp.flags & MEM_EVENT_FLAG_DROP_PAGE) )
>>>>          {
>>>> -            gfn_lock(p2m, rsp.gfn, 0);
>>>> -            mfn = p2m->get_entry(p2m, rsp.gfn, &p2mt, &a, 0, NULL);
>>>> +            uint64_t gfn = rsp.u.mem_access.gfn;
>>>> +            gfn_lock(p2m, gfn, 0);
>>>
>>> Blank line between declarations and statements. Also - why uint64_t
>>> instead of just unsigned long?
>>
>>The type of mem_access.gfn is uint64_t so its that for consistency.
>
> And the type most functions taking a gfn expect is unsigned long.
>
> Jan

Well, we need to have it as uint64_t in the shared public struct
definition so that the width of it is explicit. Unsigned long may be
32-bit depending on the compiler so it is going to be casted
somewhere. Whether it is preferred to be casted when assigned to a
local variable or when passed to the function is just a style question
- I'm fine with either.

Tamas

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