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

[Stefan Hajnoczi]

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.

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

Stefan

pgpc3gmaWAQYn.pgp
Description: PGP signature