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:25:11 -0600 Message-ID: <4EEF5757.6040607@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> <4EEF53D1.5040302@codemonkey.ws> <4EEF5591.2010207@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Stefano Stabellini , xen-devel@lists.xensource.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, "Michael S. Tsirkin" To: Avi Kivity Return-path: Received: from mail-gx0-f174.google.com ([209.85.161.174]:47144 "EHLO mail-gx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752462Ab1LSPZR (ORCPT ); Mon, 19 Dec 2011 10:25:17 -0500 Received: by ggdk6 with SMTP id k6so4055314ggd.19 for ; Mon, 19 Dec 2011 07:25:16 -0800 (PST) In-Reply-To: <4EEF5591.2010207@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 =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 con= cern >>> (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) th= at >> begin with an underscore (=91_=92)" >> >> Now that might just mean that you're shadowing a global name, so may= be >> that's okay, but I think it's easier to just enforce a consistent ru= le >> of, "don't start identifiers with an underscore". > > I rather like the pattern > > void callback(void *_foo) > { > Foo *foo =3D _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 p= refer=20 simple rules to subtle ones as it creates less confusion so I'd prefer = you=20 change this. > >>>>> +MemoryRegionSection memory_region_find(MemoryRegion *address_spa= ce, >>>>> + 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= =2E > 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=20 this style, I'm not going to object strongly. But it should be at least be documented and used consistently. Regards, Anthony Liguori