From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2 5/5] libxl, hvmloader: Don't relocate memory for MMIO hole Date: Thu, 20 Jun 2013 12:01:58 +0100 Message-ID: <51C2E126.4090304@eu.citrix.com> References: <1371573984-28514-1-git-send-email-george.dunlap@eu.citrix.com> <1371573984-28514-5-git-send-email-george.dunlap@eu.citrix.com> <51C2C9E0.6060006@eu.citrix.com> <51C2F1A502000078000DF4B7@nat28.tlf.novell.com> <51C2D77D.90103@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Ian Campbell , Hanweidong , xen-devel@lists.xen.org, Stefano Stabellini , Jan Beulich , Ian Jackson List-Id: xen-devel@lists.xenproject.org On 20/06/13 11:29, Stefano Stabellini wrote: > On Thu, 20 Jun 2013, George Dunlap wrote: >> On 20/06/13 11:12, Jan Beulich wrote: >>>>>> On 20.06.13 at 11:22, George Dunlap >>>>>> wrote: >>>> On 19/06/13 18:18, Stefano Stabellini wrote: >>>>> On Tue, 18 Jun 2013, George Dunlap wrote: >>>>>> + s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL); >>>>>> + if ( s ) >>>>>> + allow_memory_relocate = (bool)strtoll(s, NULL, 0); >>>>>> + printf("Relocating guest memory for lowmem MMIO space %s\n", >>>>>> + allow_memory_relocate?"enabled":"disabled"); >>>>> It doesn't take a strtoll to parse a boolean. >>>> As discussed in v1, strtoll is the only "XtoY" function available in >>>> hvmloader. :-) The only other option would be to explicitly compare for >>>> "1" or "0" (or do some half-baked *s-'0' thing). >>>> >>>> This does make me think though -- what is the semantics of casting to a >>>> bool? Is it !!, or will it essentially clip off the high bits? (e.g., >>>> would "2" become "1", or "0"?) >>> If bool is a typedef or #define of _Bool, and _Bool is a complier >>> supplied type, then the cast will do the right thing. But doing the >>> assignment without the cast would too, i.e. the cast is pointless >>> (as I think IanJ had already pointed out). >> Thanks for the info. >> >> It may be pointless from a functionality perspective, but it's also harmless. >> It won't add a single byte to the compiled code, but the 6 characters will >> remind a developer reading the source that there is a cast being done here, >> just in case it should ever become important. Not super important, but I'd >> rather leave it in. >> >>> However, if we want to be on the safe side and also make the >>> code work with a compiler that doesn't have a built-in _Bool, I'd >>> think >>> >>> allow_memory_relocate = !s || strtoll(s, NULL, 0); >>> >>> would be the better statement (without any if() surrounding it, >>> and without the variable declaration having an initializer. >> Doing this would effectively hide the "default" value. This is bad because 1) >> it's not clear what the default is to someone just scanning the code, 2) it's >> hard to change. (Consider how you'd modify the above statement if you wanted >> to default to 0 instead.) > I would avoid the strtoll altogether: > > if (s != NULL && s[0] != '0') > allow_memory_relocate = 1; > else > allow_memory_relocate = 0; I think I'd be more inclined to do allow_memory_relocate = strcmp(s, "0"); That will have more predictable results (e.g., 0 is false, anything else at all is true). -George