Re: [Xen-devel] [Qemu-devel] [PATCH 01/23] memory: introduce memory_region_find()

[Anthony Liguori <anthony@xxxxxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel