From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH 04/13] tools/libxl: detect and avoid conflicts with RDM Date: Fri, 08 May 2015 09:14:56 +0800 Message-ID: <554C0E10.5000401@intel.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-5-git-send-email-tiejun.chen@intel.com> <5534FB700200007800073B6C@mail.emea.novell.com> <554A2CA1.9030907@intel.com> <554A509402000078000773F6@mail.emea.novell.com> <554ACC5C.2040300@intel.com> <554B0E8802000078000CEA2D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <554B0E8802000078000CEA2D@mail.emea.novell.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: Jan Beulich Cc: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2015/5/7 14:04, Jan Beulich wrote: >>>> "Chen, Tiejun" 05/07/15 4:22 AM >>> >> On 2015/5/6 23:34, Jan Beulich wrote: >>>>>> On 06.05.15 at 17:00, wrote: >>>> On 2015/4/20 19:13, Jan Beulich wrote: >>>>>>>> On 10.04.15 at 11:21, wrote: >>>>>> + PERROR("Could not allocate memory."); >>>>> >>>>> Now that's exactly the kind of error message that makes no sense: >>>>> As errno will already cause PERROR() to print something along the >>>>> lines of the message you provide here, you're just creating >>>>> redundancy. Indicating the purpose of the allocation, otoh, would >>>>> add helpful context for the one inspecting the resulting log. >>>> >>>> What about this? >>>> >>>> PERROR("Could not allocate memory buffers to store reserved device >>>> memory entries."); >>> >>> You kind of go from one extreme to the other - the message >>> doesn't need to be overly long, but it should be distinct from >>> all other messages (so that when seen one can identify what >>> went wrong). >> >> I originally refer to some existing examples like this, >> >> int >> xc_core_arch_memory_map_get(xc_interface *xch, struct >> xc_core_arch_context *unused, >> xc_dominfo_t *info, shared_info_any_t >> *live_shinfo, >> xc_core_memory_map_t **mapp, >> unsigned int *nr_entries) >> { >> ... >> map = malloc(sizeof(*map)); >> if ( map == NULL ) >> { >> PERROR("Could not allocate memory"); >> return -1; >> } >> >> Maybe this is wrong to my case. Could I change this? > > Yeah, I realize there are bad examples. But we try to avoid spreading those. Sure. > >> PERROR("Could not allocate memory for XENMEM_reserved_device_memory_map >> hypercall"); >> >> Or just give me your line. > > How about > > PERROR("Could not allocate RDM buffer"); > > It's brief but specific enough to uniquely identify it. Looks good. > >>>>> and hence don't have the final say on stylistic issues, I don't see >>>>> why the above couldn't be expressed with a single return statement. >>>> >>>> Are you saying something like this? Note this was showed by yourself >>>> long time ago. >>> >>> I know, and hence I was puzzled to still see you use the more >>> convoluted form. >>> >>>> static bool check_mmio_hole_conflict(uint64_t start, uint64_t memsize, >>>> uint64_t mmio_start, uint64_t mmio_size) >>>> { >>>> return start + memsize > mmio_start && start < mmio_start + mmio_size; >>>> } >>>> >>>> But I don't think this really can't work out our case. >>> >>> It's equivalent to the original you had, so I don't see what you >>> mean with "this really can't work out our case". >> >> Let me make this point clear. >> >> The original implementation, >> >> +static int check_rdm_hole(uint64_t start, uint64_t memsize, >> + uint64_t rdm_start, uint64_t rdm_size) >> +{ >> + if (start + memsize <= rdm_start || start >= rdm_start + rdm_size) >> + return 0; >> + else >> + return 1; >> +} >> >> means it returns 'false' in two cases: >> >> #1. end = start + memsize; end <= rdm_start; >> >> This region [start, end] is below of rdm entry. >> >> #2. rdm_end = rdm_start + rdm_size; stat >= rdm_end; >> >> This region [start, end] is above of rdm entry. >> >> So others conditions should indicate that rdm entry is overlapping with >> this region. Actually this has three cases: >> >> #1. This region just conflicts with the first half of rdm entry; >> #2. This region just conflicts with the second half of rdm entry; >> #3. This whole region falls inside of rdm entry; >> >> Then it should return 'true', right? >> >> But with this single line, >> >> return start + memsize > rdm_start && start < rdm_start + rdm_size; >> >> => >> >> return end > rdm_start && start < rdm_end; >> >> This just guarantee it return 'true' *only* if #3 above occurs. > > I don't even need to look at all the explanations you give. It is a simple matter > of expression re-writing to see that > > if (a <= b || c >= d) > return 0; > else > return 1; > > is equivalent to > > return !(a <= b || c >= d); > > and a simple matter of formal logic to see that this is equivalent to > > return a > b && c < d; Right now I think you're right. And I can't recall exactly what's my problem while using this simple line, maybe others was misleading me to treat this change as a cause so sorry to this confusion. Thanks Tiejun