Re: [Qemu-trivial] [PATCH] ivshmem: allow the sharing of hugepages

[Michael Tokarev <mjt@tls.msk.ru>]

12.09.2013 22:23, Damien Millescamps wrote:
> According to shm_open specifications:
>
>   A shared memory object should be identified by a name of the form /somename;
>   that is, a null-terminated string of up to NAME_MAX (i.e., 255) characters
>   consisting of an initial slash, followed by one or more characters, none of
>   which are slashes.
>
> This patch permits to share memory areas that do not specifically belong to
> /dev/shmem.
>
> A use case for this patch is sharing huge pages available through a
> hugetlbfs mountpoint.
>
> Signed-off-by: Damien Millescamps <damien.millescamps@6wind.com>
> ---
>   hw/misc/ivshmem.c |   16 +++++++++++++++-
>   1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 2838866..9020bb2 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -751,9 +751,23 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
>           IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
>
> +        /*
> +         * A shared memory object should be identified by a name of the form /somename;
> +         * that is, a null-terminated string of up to NAME_MAX (i.e., 255) characters
> +         * consisting of an initial slash, followed by one or more characters, none of
> +         * which are slashes.

It'd be nice to give a hint about where this definition comes from.

And once you look at shm_open(3), you'll notice that this paragraph
is prefixed with "For portable use", and reads:

  For portable use, a shared memory object should be identified by a name of the
  form  /somename; that is, a null-terminated string of up to NAME_MAX (i.e., 255)
  characters consisting of an initial slash, followed by one or more characters,
  none of which are slashes.

That to say, this is not a _definition_ of a shared memory object, it is just
a suggested name syntax, suggested purely for portability.  In other words,
there may be other acceptable syntaxes for it.

So as the result, I'm not sure this approach is valid.  Maybe we should
always try shared first and create-new second?  I dunno.

Note that whole thing - using shared memory object like this - may lead
to surprizes at least, -- users who previously expected one behavour now
see different behavour.  Most likely the old behavour wasn't correct.

At least this should be documented somewhere in user-visible part of
ivshmem, so users will have an ides when objects will be shared and
when truncated.

> +         */
> +        if (s->shmobj && s->shmobj[0] == '/' && strstr(&s->shmobj[1], "/")) {

Here you're testing for s->shmobj, but before, the code were referencing
it directly as an argument for shm_open().  Can it be NULL in this place?

It is a somewhat minor nitpick, but it'd be not nice to spread such tests
(for NULLness) where the object can't be NULL and to confuse readers.

> +            /* This can't be a shared memory object. */
> +            fd = open(s->shmobj, O_RDWR);
> +            if (fd < 0) {
> +                perror("ivshmem - open");
> +                exit(-1);
> +            }
> +        }
>           /* try opening with O_EXCL and if it succeeds zero the memory
>            * by truncating to 0 */
> -        if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
> +        else if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
>                           S_IRWXU|S_IRWXG|S_IRWXO)) > 0) {
>              /* truncate file to length PCI device's memory */
>               if (ftruncate(fd, s->ivshmem_size) != 0) {
>

Thanks,

/mjt