[pulseaudio-discuss] [PATCH 05/11] pulsecore: Provide an abstract interface for pa_shm and pa_privatemem

[Tanu Kaskinen]

On Sun, 2015-09-20 at 23:31 +0200, Ahmed S. Darwish wrote:
> Given this new interface, we can now remove the pool's pointer and
> size fields: they can be obtained through polymorphic access.
> 
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
>  src/Makefile.am            |  1 +
>  src/pulsecore/mem.h        | 43 +++++++++++++++++++++++++++++++++++++++++++
>  src/pulsecore/memblock.c   | 33 ++++++++++++++-------------------
>  src/pulsecore/privatemem.h |  5 +++--
>  src/pulsecore/shm.h        |  7 ++++---
>  5 files changed, 65 insertions(+), 24 deletions(-)
>  create mode 100644 src/pulsecore/mem.h
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 19b335c..d4a6b5d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -697,6 +697,7 @@ libpulsecommon_ at PA_MAJORMINOR@_la_SOURCES = \
>  > 	> 	> pulsecore/refcnt.h \
>  > 	> 	> pulsecore/srbchannel.c pulsecore/srbchannel.h \
>  > 	> 	> pulsecore/sample-util.c pulsecore/sample-util.h \
> +> 	> 	> pulsecore/mem.h \
>  > 	> 	> pulsecore/shm.c pulsecore/shm.h \
>  > 	> 	> pulsecore/privatemem.c pulsecore/privatemem.h \
>  > 	> 	> pulsecore/bitset.c pulsecore/bitset.h \
> diff --git a/src/pulsecore/mem.h b/src/pulsecore/mem.h
> new file mode 100644
> index 0000000..7390e47
> --- /dev/null
> +++ b/src/pulsecore/mem.h
> @@ -0,0 +1,43 @@
> +#ifndef SRC_PULSECORE_MEM_H_
> +#define SRC_PULSECORE_MEM_H_

#define foomemhfoo would be more consistent with other headers.

> +
> +/***
> +  This file is part of PulseAudio.
> +
> +  Copyright 2015 Ahmed S. Darwish <darwish.07 at gmail.com>
> +
> +  PulseAudio is free software; you can redistribute it and/or modify
> +  it under the terms of the GNU Lesser General Public License as
> +  published by the Free Software Foundation; either version 2.1 of the
> +  License, or (at your option) any later version.
> +
> +  PulseAudio is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  Lesser General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public
> +  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include 
> +
> +/* Parent anonymous structure representing a plain memory area and its

We haven't used anonymous structures so far, since they've been non
-standard in the past. They're supported since C11, however, and I
think we could start using them. I'll start another thread about that,
in case not everyone reads review mails carefully...

> + * size. Different structures inherit from this, representing different
> + * memory area types (e.g. Posix SHM, Linux memfds, or private).
> + *
> + * To inherit from this object, just include __PA_MEM_STRUCT__ as the
> + * very first element of your structure. Check "pa_mempool", "pa_shm",
> + * and "pa_memfd" for a concrete inheritance relationships example. */
> +#define __PA_PARENT_MEM_STRUCT__ struct {   \

The comment talks about __PA_MEM_STRUCT__, but here you define
__PA_PARENT_MEM_STRUCT__. I guess they're supposed to be the same. I
prefer the shorter version.

Another thing: the C standard reserves all identifiers that start with
two underscores, so it's not good style to use them in application
code. "PA_MEM_STRUCT" without any extra underscores looks good enough
to me.

> +    void *ptr;                              \
> +    size_t size;                            \
> +} PA_GCC_PACKED

What's the rationale for using PA_GCC_PACKED?

-- 
Tanu