|
From: | Anthony Liguori |
Subject: | Re: [Qemu-devel] [PATCH 01/23] memory: introduce memory_region_find() |
Date: | Mon, 19 Dec 2011 09:10:09 -0600 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |