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 6:39 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> 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.

I just reused the function since I've seen it being used for printing
warnings. What would be the preffered print function to be used here?

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

Ack.

>
> Jan
>

Thanks,
Tamas

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