Xen 
 
Home About Xen.org Xen Xen Summit Wiki Mailing List Bug Tracker Xen Downloads
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 1/9] Add cpu idle pwr mgmt to xen

To: Jan Beulich <jbeulich@xxxxxxxxxx>, Gang Wei <gang.wei@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 1/9] Add cpu idle pwr mgmt to xen
From: Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
Date: Wed, 30 Apr 2008 09:54:15 +0100
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Wed, 30 Apr 2008 01:55:21 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <48183A3F.76E4.0078.0@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: Aciqn8RNAuw9vxaTEd2yrAAX8io7RQ==
Thread-topic: [Xen-devel] [PATCH 1/9] Add cpu idle pwr mgmt to xen
User-agent: Microsoft-Entourage/11.4.0.080122
On 30/4/08 08:22, "Jan Beulich" <jbeulich@xxxxxxxxxx> wrote:

>>>> "Wei, Gang" <gang.wei@xxxxxxxxx> 30.04.08 05:27 >>>
>> Revising done according to Jan's comments. Resend.
> 
> Thanks. Unfortunately you now use a static (but not per-CPU) variable -
> while I understand that it is expected that the call is done just once, I
> don't think this is a good thing to do.

Why is the variable even non-local? Is it just to make the xlat_malloc*()
interfaces simpler? It's a false simplification if so, and I think you'd be
better making the variable an explicit parameter to those functions.

Also I agree with Jan regarding non-ISO C usage of loop-header variable
declarations (don't do it) and also you should check copy_from_guest*()
return values and return -EFAULT where appropriate. His comment regarding
explicit padding or use of uint32_t in your public bitfield also sounds good
to me.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel