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

[Damien Millescamps <damien.millescamps@6wind.com>]

On 09/14/2013 11:36 AM, Michael Tokarev wrote:
> 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.

You are right, the definition can be found in shm_open(P), and reads:
 
  If name does not begin with the slash character, the effect is
implementation-defined.
  The interpretation of slash characters other  than  the leading slash
character in name
  is implementation-defined.

The "implementation-defined" found in glibc is as follow:

The leading '/', if present, is removed. and '/dev/shm/' is prepended to
the resulting name before calling open().
(found in sysdeps/posix/shm_open.c around line 57 depending on the version).

Note that a workaround in my case is to give a name in the form:
/../../path/to/file" ...

> 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.

This is probably a better approach, yes. cf next paragraph.

> 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.

By first trying to open with shm_open, and only when it fails with open,
the behavior should stay the same. Because according to glibc
implementation, if "folder" exists in /dev/shm, a name like
"/folder/shared_mem" should work, but will trigger a false positive with
the checks I added.
Note that I am not sure anyone uses this syntax, but still, this is a
false positive. I'll change that.

> 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.

I will add a paragraph in docs/specs/ivshmem_device_spec.txt for that.
Thanks for mentioning it.

> 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.

agreed.

> Thanks,

Thanks for your review, that was helpful. I'll send a reworked patch,
probably on Monday.

-- 
Damien Millescamps