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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx> 02/10/15 5:38 PM >>>
>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.

That'll make the code even uglier, and won't address the question I originally 
asked.

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

That's understood.

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

As much as "gfn" function parameters are usually unsigned long, local
variables are too iirc. So please follow suit.

Jan


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