From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rcf5f-0005it-1S for qemu-devel@nongnu.org; Mon, 19 Dec 2011 10:25:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rcf5Z-0008MP-7z for qemu-devel@nongnu.org; Mon, 19 Dec 2011 10:25:23 -0500 Received: from mail-gx0-f173.google.com ([209.85.161.173]:63825) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rcf5Z-0008MI-5M for qemu-devel@nongnu.org; Mon, 19 Dec 2011 10:25:17 -0500 Received: by ggnk1 with SMTP id k1so4664693ggn.4 for ; Mon, 19 Dec 2011 07:25:16 -0800 (PST) Message-ID: <4EEF5757.6040607@codemonkey.ws> Date: Mon, 19 Dec 2011 09:25:11 -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> <4EEF53D1.5040302@codemonkey.ws> <4EEF5591.2010207@redhat.com> In-Reply-To: <4EEF5591.2010207@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: xen-devel@lists.xensource.com, "Michael S. Tsirkin" , qemu-devel@nongnu.org, kvm@vger.kernel.org, Stefano Stabellini On 12/19/2011 09:17 AM, Avi Kivity wrote: > On 12/19/2011 05:10 PM, Anthony Liguori wrote: >> 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". > > I rather like the pattern > > void callback(void *_foo) > { > Foo *foo = _foo; > > ... > } > > If the consensus is that we don't want it, I'll change it, but I do > think it's useful. I dislike enforcing coding style but it's a necessary evil. I greater prefer simple rules to subtle ones as it creates less confusion so I'd prefer you change this. > >>>>> +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. > > It's plain C, I don't see why it's so 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. >> > > How can I return a pointer? If I allocate it, someone has to free it. > If I pass it as a parameter, we end up with a silly looking calling > convention. If I return an error code, the caller has to set up an > additional variable. > > The natural check is section.size which is always meaningful - > memory_region_find() returns a subrange of the input, which may be > zero. You're right that I should document it (and I should use it > consistently). I'm not going to make a fuss over something like this so if you really prefer this style, I'm not going to object strongly. But it should be at least be documented and used consistently. Regards, Anthony Liguori