Re: [Qemu-devel] [PATCH v3 34/46] ivshmem-server: fix hugetlbfs support

[Vladimir Sementsov-Ogievskiy]

On 16.09.2015 19:14, Marc-André Lureau wrote:
> ----- Original Message -----
> > On 15.09.2015 19:07, address@hidden wrote:
> > > From: Marc-André Lureau <address@hidden>
> > >
> > > As pointed out on the ML by Andrew Jones, glibc no longer permits
> > > creating POSIX shm on hugetlbfs directly. When given a hugetlbfs path,
> > > create a shareable file there.
> > >
> > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > ---
> > >    contrib/ivshmem-server/ivshmem-server.c | 40
> > >    ++++++++++++++++++++++++++++++++-
> > >    contrib/ivshmem-server/ivshmem-server.h |  3 +--
> > >    contrib/ivshmem-server/main.c           |  5 ++---
> > >    3 files changed, 42 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/contrib/ivshmem-server/ivshmem-server.c
> > > b/contrib/ivshmem-server/ivshmem-server.c
> > > index 972fda2..51264b4 100644
> > > --- a/contrib/ivshmem-server/ivshmem-server.c
> > > +++ b/contrib/ivshmem-server/ivshmem-server.c
> > > @@ -11,6 +11,7 @@
> > >    #include <sys/socket.h>
> > >    #include <sys/un.h>
> > >    #include <sys/eventfd.h>
> > > +#include <sys/vfs.h>
> > >    
> > >    #include "qemu-common.h"
> > >    #include "qemu/queue.h"
> > > @@ -271,15 +272,52 @@ ivshmem_server_init(IvshmemServer *server, const char
> > > *unix_sock_path,
> > >        return 0;
> > >    }
> > >    
> > > +#define HUGETLBFS_MAGIC       0x958458f6
> > > +
> > > +static long gethugepagesize(const char *path)
> > > +{
> > > +    struct statfs fs;
> > > +    int ret;
> > > +
> > > +    do {
> > > +        ret = statfs(path, &fs);
> > > +    } while (ret != 0 && errno == EINTR);
> > > +
> > > +    if (ret != 0) {
> > > +        if (errno != ENOENT) {
> > > +            fprintf(stderr, "cannot stat shm file %s: %s\n", path,
> > > +                    strerror(errno));
> > > +        }
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (fs.f_type != HUGETLBFS_MAGIC) {
> > should it be silent?
> Well, the same given path may be shm name or a hugefs. I guess we could use a 
> different argument instead and make sure that it is hugefs. At the minimum, I 
> understand we may want a warning here.
>
> > > +        return -1;
> > > +    }
> > > +
> > > +    return fs.f_bsize;
> > > +}
> > > +
> > >    /* open shm, create and bind to the unix socket */
> > >    int
> > >    ivshmem_server_start(IvshmemServer *server)
> > >    {
> > >        struct sockaddr_un sun;
> > >        int shm_fd, sock_fd, ret;
> > > +    long hpagesize;
> > > +    gchar *filename;
> > >    
> > >        /* open shm file */
> > > -    shm_fd = shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU);
> > > +    hpagesize = gethugepagesize(server->shm_path);
> > > +    if (hpagesize > 0) {
> > filename is used only in this block, it may be defined here.
> ok
>
> > > +        filename = g_strdup_printf("%s/ivshmem.XXXXXX", server->shm_path);
> > > +        shm_fd = mkstemp(filename);
> > > +        unlink(filename);
> > > +        g_free(filename);
> > > +    } else {
> > > +        shm_fd = shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU);
> > > +    }
> > > +
> > >        if (shm_fd < 0) {
> > >            fprintf(stderr, "cannot open shm file %s: %s\n",
> > >            server->shm_path,
> > >                    strerror(errno));
> > > diff --git a/contrib/ivshmem-server/ivshmem-server.h
> > > b/contrib/ivshmem-server/ivshmem-server.h
> > > index 2176d5e..e9b0e7a 100644
> > > --- a/contrib/ivshmem-server/ivshmem-server.h
> > > +++ b/contrib/ivshmem-server/ivshmem-server.h
> > > @@ -81,8 +81,7 @@ typedef struct IvshmemServer {
> > >     * @server:         A pointer to an uninitialized IvshmemServer structure
> > >     * @unix_sock_path: The pointer to the unix socket file name
> > >     * @shm_path:       Path to the shared memory. The path corresponds to a
> > >     POSIX
> > > - *                  shm name. To use a real file, for instance in a
> > > hugetlbfs,
> > > - *                  it is possible to use /../../abspath/to/file.
> > > + *                  shm name or a hugetlbfs mount point.
> > >     * @shm_size:       Size of shared memory
> > >     * @n_vectors:      Number of interrupt vectors per client
> > >     * @verbose:        True to enable verbose mode
> > > diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
> > > index 84ffc4d..cd8d9ed 100644
> > > --- a/contrib/ivshmem-server/main.c
> > > +++ b/contrib/ivshmem-server/main.c
> > > @@ -47,9 +47,8 @@ ivshmem_server_usage(const char *name, int code)
> > >                        "     to listen to.\n"
> > >                        "     Default=%s\n",
> > >                        IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH);
> > >        fprintf(stderr, "  -m <shm_path>: path to the shared memory.\n"
> > > -                    "     The path corresponds to a POSIX shm name. To use
> > > a\n"
> > > -                    "     real file, for instance in a hugetlbfs, use\n"
> > > -                    "     /../../abspath/to/file.\n"
> > > +                    "     The path corresponds to a POSIX shm name or a\n"
> > > +                    "     hugetlbfs mount point.\n"
> > >                        "     default=%s\n",
> > >                        IVSHMEM_SERVER_DEFAULT_SHM_PATH);
> > >        fprintf(stderr, "  -l <size>: size of shared memory in bytes. The
> > >        suffix\n"
> > >                        "     K, M and G can be used (ex: 1K means 1024).\n"
> >
> > --
> > Best regards,
> > Vladimir
> > * now, @virtuozzo.com instead of @parallels.com. Sorry for this
> > inconvenience.
with these tiny fixes:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>



--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.