This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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