From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57209) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rcer3-0007fW-12 for qemu-devel@nongnu.org; Mon, 19 Dec 2011 10:10:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rcer1-0004bh-Aa for qemu-devel@nongnu.org; Mon, 19 Dec 2011 10:10:16 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:36562) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rcer1-0004bc-4j for qemu-devel@nongnu.org; Mon, 19 Dec 2011 10:10:15 -0500 Received: by iagj37 with SMTP id j37so9830738iag.4 for ; Mon, 19 Dec 2011 07:10:14 -0800 (PST) Message-ID: <4EEF53D1.5040302@codemonkey.ws> Date: Mon, 19 Dec 2011 09:10:09 -0600 From: Anthony Liguori MIME-Version: 1.0 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> In-Reply-To: <4EEF5263.8070004@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 01/23] memory: introduce memory_region_find() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: Stefano Stabellini , xen-devel@lists.xensource.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, "Michael S. Tsirkin" 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 = _addr; >>> + const FlatRange *fr = _fr; >> >> >> Please don't prefix with an underscore. > > Why not? It's legal according to the standards, if that's your concern > (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 include all external identifiers (global functions and variables) that begin with an underscore (‘_’)" Now that might just mean that you're shadowing a global name, so maybe that's okay, but I think it's easier to just enforce a consistent rule of, "don't start 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 by >>> + * @address_space that overlaps the range given by @addr and @size. >>> + * >>> + * @address_space: a top-level (i.e. parentless) region that contains >>> + * 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 occurs (which isn't documented by the comment btw). Returning a pointer, or taking a pointer and returning a 0/-1 return value makes for a more consistent semantic with the rest of the code base. Regards, Anthony Liguori