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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux