From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] libxl, hvmloader: Don't relocate memory for MMIO hole Date: Tue, 18 Jun 2013 12:43:24 +0100 Message-ID: <51C047DC.6090609@eu.citrix.com> References: <1371487427-13025-1-git-send-email-george.dunlap@eu.citrix.com> <20927.17485.615792.812108@mariner.uk.xensource.com> <51C02D57.4060601@eu.citrix.com> <20928.15411.508151.115188@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20928.15411.508151.115188@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Hanweidong , Stefano Stabellini , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 06/18/2013 11:53 AM, Ian Jackson wrote: > George Dunlap writes ("Re: [PATCH] libxl,hvmloader: Don't relocate memory for MMIO hole"): >> On 06/17/2013 06:15 PM, Ian Jackson wrote: >>> I think the approach is good. Arguably the two things should be in >>> two patches. >> >> You mean one in libxl to set allow_memory_relocate, one to do something >> about it? > > No. I mean one to do the thing with allow_memory_relocate and one to > change the condition on bar64_relocate. But maybe I have > misunderstood and that doesn't make sense. Anyway, it's OK IMO as one > patch. Yes, I was thinking that if we were breaking down the patches, that might be a candidate for a separate patch. Why don't I do that anyway. > >>>> + s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL); >>>> + if (s) >>>> + allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0); >>> >>> Use strtoul, surely, and bool_t (or _Bool). Then there is no need for >>> the cast. (Also, spaces round `='.) >> >> Remember that hvmloader doesn't actually have a libc; this is a locally >> implemented function, and AFAICT the only one implemented is strtoll. > > _Bool is a compiler feature and available without the use of > stdbool.h. It has better semantics than uintblah_t. We apparently have stdbool.h, but it defines "bool" instead of "bool_t". :-) >>> Use GCSPRINTF not libxl_sprintf. Lacks error check (check return >>> value from libxl__xs_write). >> >> Once again, I disagree with mixing different code styles in the same >> function. Using GCSPRINTF and doing an error check here will make it >> seem like it's doing something different than the line immediately above >> it (which is what I copied to get here), which will confuse people. > > Perhaps we should sweep through libxl sorting out these kind of style > things in 4.4. Clearly now isn't the time for this kind of wholesale > change. > > In the meantime I think we do want to avoid introducing new code with > deprecated style. I'm afraid if that's what you want you're going to have to do it yourself. I think mixing code style is much worse than having a consistent, but deprecated code style, and I'm not going to do it. -George