Re: [PATCH 2/2] Allow memory to be specified in kvmctl

[Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.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/