Re: [Qemu-devel] [PATCH 3/4] libqos: add a simple first-fit memory allocator

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Thu, Jul 31, 2014 at 05:14:12PM -0400, John Snow wrote:
>> 
>> On 07/31/2014 06:13 AM, Stefan Hajnoczi wrote:
>> >On Wed, Jul 30, 2014 at 06:28:28PM -0400, John Snow wrote:
>> >>-static uint64_t pc_alloc(QGuestAllocator *allocator, size_t size)
>> >>+static inline void mlist_insert(MemList *head, MemBlock *insr)
>> >>  {
>> >>-    PCAlloc *s = container_of(allocator, PCAlloc, alloc);
>> >>-    uint64_t addr;
>> >>+    QTAILQ_INSERT_HEAD(head, insr, MLIST_ENTNAME);
>> >>+}
>> >>+
>> >>+static inline void mlist_append(MemList *head, MemBlock *node)
>> >>+{
>> >>+    QTAILQ_INSERT_TAIL(head, node, MLIST_ENTNAME);
>> >>+}
>> >>+
>> >>+static inline void mlist_unlink(MemList *head, MemBlock *rem)
>> >>+{
>> >>+    QTAILQ_REMOVE(head, rem, MLIST_ENTNAME);
>> >>+}
>> >For the same reasons as my comments about the macros, these trivial 
>> >functions
>> >are boilerplate.  Why not use the QTAILQ macros directly?
>> For /at least/ the append and insert cases, I desired call-by-value
>> semantics.
>> As a matter of taste, I find macros annoying for the reason that you cannot
>> inline things such as:
>> mlist_insert(list, mlist_new(...));
>> 
>> but unlink is certainly superfluous, and just something I did for some
>> consistency.
>> 
>> If there is a matter of style where in-line function call is to be avoided
>> entirely, I'll just nix all of these trivial inlines.
>
> As a reviewer I prefer to see familiar APIs rather than a convenience
> layer because it's extra stuff I need to keep in mind.  Sticking to the
> APIs makes it quicker for other QEMU developers to parse the code.

Seconded.

> That said, feel free to keep the functions if you want.

Can't say, as I haven't studied the patch in detail.