From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH 2/2] Allow memory to be specified in kvmctl
Date: Tue, 09 Oct 2007 17:09:19 -0500
Message-ID: <470BFC0F.5040707@us.ibm.com>
References: <11919655141141-git-send-email-aliguori@us.ibm.com>
	<11919655153228-git-send-email-aliguori@us.ibm.com>
	<20071009215449.GM4335@rhun.haifa.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
	Jerone Young <jyoung5-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>, Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
To: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>
Return-path: <kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
In-Reply-To: <20071009215449.GM4335-WD1JZD8MxeCTrf4lBMg6DdBPR1lH4CV8@public.gmane.org>
List-Unsubscribe: <https://lists.sourceforge.net/lists/listinfo/kvm-devel>,
	<mailto:kvm-devel-request-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org?subject=unsubscribe>
List-Archive: <http://sourceforge.net/mailarchive/forum.php?forum_name=kvm-devel>
List-Post: <mailto:kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
List-Help: <mailto:kvm-devel-request-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org?subject=help>
List-Subscribe: <https://lists.sourceforge.net/lists/listinfo/kvm-devel>,
	<mailto:kvm-devel-request-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org?subject=subscribe>
Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
List-Id: kvm.vger.kernel.org

Muli Ben-Yehuda wrote:
>>  	while ((ch = getopt_long(argc, argv, sopts, lopts, &opt_ind)) != -1) {
>>  		switch (ch) {
>> @@ -367,6 +372,24 @@ int main(int argc, char **argv)
>>  		case 'p':
>>  			enter_protected_mode = true;
>>  			break;
>> +		case 'm':
>> +			memory_size = strtoull(optarg, &endptr, 0);
>> +			switch (*endptr) {
>> +			case 'G': case 'g':
>> +				memory_size <<= 10;
>> +			case '\0':
>> +			case 'M': case 'm':
>> +				memory_size <<= 10;
>> +			case 'K': case 'k':
>> +				memory_size <<= 10;
>> +				break;
>>     
>
> Cute trick with the fall-through and shifts... not quite Duff's
> device, but cute. Please consider adding a /* fallthrough */ comment
> to make it obvious.
>   

I'll make it more clear and send out the series again tomorrow when 
others have gotten a chance to review.

>> +			default:
>> +				fprintf(stderr,
>> +					"Unrecongized memory suffix: %c\n",
>> +					*endptr);
>> +				exit(1);
>> +			}
>> +			break;
>>     
>
> How about adding a sanity check that memory_size makes sense here
> rather than having kvm_create() fail obscurely? For example if the
> user got the memory size wrong for some reason we'll end up with
> memory_size = 0 here.
>   

There's an exit(1) and it's using stroull() so the only way that 
memory_size could equal 0 is if the user specified --memory=0.  I'm not 
sure I agree it's worth checking for that sort of circumstance, perhaps 
the user had a reason for doing it?

Regards,

Anthony Liguori

> Cheers,
> Muli
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/