From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [Qemu-devel] [PATCH 01/23] memory: introduce memory_region_find() Date: Mon, 19 Dec 2011 09:10:09 -0600 Message-ID: <4EEF53D1.5040302@codemonkey.ws> References: <1324304024-11220-1-git-send-email-avi@redhat.com> <1324304024-11220-2-git-send-email-avi@redhat.com> <4EEF4F1E.9080104@codemonkey.ws> <4EEF5263.8070004@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: xen-devel@lists.xensource.com, "Michael S. Tsirkin" , qemu-devel@nongnu.org, kvm@vger.kernel.org, Stefano Stabellini To: Avi Kivity Return-path: Received: from mail-iy0-f174.google.com ([209.85.210.174]:33919 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750880Ab1LSPKP (ORCPT ); Mon, 19 Dec 2011 10:10:15 -0500 Received: by iaeh11 with SMTP id h11so8664336iae.19 for ; Mon, 19 Dec 2011 07:10:14 -0800 (PST) In-Reply-To: <4EEF5263.8070004@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/19/2011 09:04 AM, Avi Kivity wrote: > On 12/19/2011 04:50 PM, Anthony Liguori wrote: >>> +static int cmp_flatrange_addr(const void *_addr, const void *_fr) >>> +{ >>> + const AddrRange *addr =3D _addr; >>> + const FlatRange *fr =3D _fr; >> >> >> Please don't prefix with an underscore. > > Why not? It's legal according to the standards, if that's your conce= rn > (only _[_A-Z]+ are reserved). http://www.gnu.org/s/hello/manual/libc/Reserved-Names.html "In addition to the names documented in this manual, reserved names inc= lude all=20 external identifiers (global functions and variables) that begin with a= n=20 underscore (=91_=92)" Now that might just mean that you're shadowing a global name, so maybe = that's=20 okay, but I think it's easier to just enforce a consistent rule of, "do= n't start=20 identifiers with an underscore". > >>> @@ -502,6 +520,20 @@ void memory_region_del_subregion(MemoryRegion = *mr, >>> MemoryRegion *subregion); >>> >>> /** >>> + * memory_region_find: locate a MemoryRegion in an address space >>> + * >>> + * Locates the first #MemoryRegion within an address space given b= y >>> + * @address_space that overlaps the range given by @addr and @size= =2E >>> + * >>> + * @address_space: a top-level (i.e. parentless) region that conta= ins >>> + * the region to be found >>> + * @addr: start of the area within @address_space to be searched >>> + * @size: size of the area to be searched >>> + */ >>> +MemoryRegionSection memory_region_find(MemoryRegion *address_space= , >>> + target_phys_addr_t addr, >>> uint64_t size); >> >> >> Returning structs by value is a bit unexpected. > > It's just prejudice, here's the call sequence: It's not about whether it's fast or slow. It's just unexpected. Instead of returning a NULL pointer on error, you set .mr to NULL if an= error=20 occurs (which isn't documented by the comment btw). Returning a pointe= r, or=20 taking a pointer and returning a 0/-1 return value makes for a more con= sistent=20 semantic with the rest of the code base. Regards, Anthony Liguori