Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-03 Thread Igor Mammedov 
On Mon,  2 Nov 2015 17:13:11 +0800
Xiao Guangrong  wrote:

> Currently, file_ram_alloc() only works on directory - it creates a file
> under @path and do mmap on it
> 
> This patch tries to allow it to work on file directly, if @path is a
> directory it works as before, otherwise it treats @path as the target
> file then directly allocate memory from it
Paolo has just queued
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06513.html
perhaps that's what you can reuse here.
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  exec.c | 80 
> ++
>  1 file changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 9075f4d..db0fdaf 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
>  }
>  
>  #ifdef __linux__
> +static bool path_is_dir(const char *path)
> +{
> +struct stat fs;
> +
> +return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
> +}
> +
> +static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
> +{
> +char *filename;
> +char *sanitized_name;
> +char *c;
> +int fd;
> +
> +if (!path_is_dir(path)) {
> +int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
> +
> +flags |= O_EXCL;
> +return open(path, flags);
> +}
> +
> +/* Make name safe to use with mkstemp by replacing '/' with '_'. */
> +sanitized_name = g_strdup(memory_region_name(block->mr));
> +for (c = sanitized_name; *c != '\0'; c++) {
> +if (*c == '/') {
> +*c = '_';
> +}
> +}
> +filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
> +   sanitized_name);
> +g_free(sanitized_name);
> +fd = mkstemp(filename);
> +if (fd >= 0) {
> +unlink(filename);
> +/*
> + * ftruncate is not supported by hugetlbfs in older
> + * hosts, so don't bother bailing out on errors.
> + * If anything goes wrong with it under other filesystems,
> + * mmap will fail.
> + */
> +if (ftruncate(fd, size)) {
> +perror("ftruncate");
> +}
> +}
> +g_free(filename);
> +
> +return fd;
> +}
> +
>  static void *file_ram_alloc(RAMBlock *block,
>  ram_addr_t memory,
>  const char *path,
>  Error **errp)
>  {
> -char *filename;
> -char *sanitized_name;
> -char *c;
>  void *area;
>  int fd;
>  uint64_t pagesize;
> @@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
>  goto error;
>  }
>  
> -/* Make name safe to use with mkstemp by replacing '/' with '_'. */
> -sanitized_name = g_strdup(memory_region_name(block->mr));
> -for (c = sanitized_name; *c != '\0'; c++) {
> -if (*c == '/')
> -*c = '_';
> -}
> -
> -filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
> -   sanitized_name);
> -g_free(sanitized_name);
> +memory = ROUND_UP(memory, pagesize);
>  
> -fd = mkstemp(filename);
> +fd = open_ram_file_path(block, path, memory);
>  if (fd < 0) {
>  error_setg_errno(errp, errno,
>   "unable to create backing store for path %s", path);
> -g_free(filename);
>  goto error;
>  }
> -unlink(filename);
> -g_free(filename);
> -
> -memory = ROUND_UP(memory, pagesize);
> -
> -/*
> - * ftruncate is not supported by hugetlbfs in older
> - * hosts, so don't bother bailing out on errors.
> - * If anything goes wrong with it under other filesystems,
> - * mmap will fail.
> - */
> -if (ftruncate(fd, memory)) {
> -perror("ftruncate");
> -}
>  
>  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);
>  if (area == MAP_FAILED) {

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-03 Thread Xiao Guangrong 



On 11/03/2015 08:34 PM, Igor Mammedov wrote:

On Mon,  2 Nov 2015 17:13:11 +0800
Xiao Guangrong  wrote:


Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a
directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Paolo has just queued
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06513.html
perhaps that's what you can reuse here.


Yep, Paolo has told me about that, i will update this patchset after his
pull request.

BTW, which tree should this patchset be based on in future development?
Paolo's or Michael's or even upstream qemu tree?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-03 Thread Paolo Bonzini 


On 03/11/2015 04:56, Xiao Guangrong wrote:
> 
> 
> On 11/03/2015 05:12 AM, Paolo Bonzini wrote:
>>
>>
>> On 02/11/2015 10:13, Xiao Guangrong wrote:
>>> Currently, file_ram_alloc() only works on directory - it creates a file
>>> under @path and do mmap on it
>>>
>>> This patch tries to allow it to work on file directly, if @path is a
>>> directory it works as before, otherwise it treats @path as the target
>>> file then directly allocate memory from it
>>>
>>> Signed-off-by: Xiao Guangrong 
>>> ---
>>>   exec.c | 80
>>> ++
>>>   1 file changed, 51 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 9075f4d..db0fdaf 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
>>>   }
>>>
>>>   #ifdef __linux__
>>> +static bool path_is_dir(const char *path)
>>> +{
>>> +struct stat fs;
>>> +
>>> +return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
>>> +}
>>> +
>>> +static int open_ram_file_path(RAMBlock *block, const char *path,
>>> size_t size)
>>> +{
>>> +char *filename;
>>> +char *sanitized_name;
>>> +char *c;
>>> +int fd;
>>> +
>>> +if (!path_is_dir(path)) {
>>> +int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
>>> +
>>> +flags |= O_EXCL;
>>> +return open(path, flags);
>>> +}
>>> +
>>> +/* Make name safe to use with mkstemp by replacing '/' with '_'. */
>>> +sanitized_name = g_strdup(memory_region_name(block->mr));
>>> +for (c = sanitized_name; *c != '\0'; c++) {
>>> +if (*c == '/') {
>>> +*c = '_';
>>> +}
>>> +}
>>> +filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
>>> +   sanitized_name);
>>> +g_free(sanitized_name);
>>> +fd = mkstemp(filename);
>>> +if (fd >= 0) {
>>> +unlink(filename);
>>> +/*
>>> + * ftruncate is not supported by hugetlbfs in older
>>> + * hosts, so don't bother bailing out on errors.
>>> + * If anything goes wrong with it under other filesystems,
>>> + * mmap will fail.
>>> + */
>>> +if (ftruncate(fd, size)) {
>>> +perror("ftruncate");
>>> +}
>>> +}
>>> +g_free(filename);
>>> +
>>> +return fd;
>>> +}
>>> +
>>>   static void *file_ram_alloc(RAMBlock *block,
>>>   ram_addr_t memory,
>>>   const char *path,
>>>   Error **errp)
>>>   {
>>> -char *filename;
>>> -char *sanitized_name;
>>> -char *c;
>>>   void *area;
>>>   int fd;
>>>   uint64_t pagesize;
>>> @@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
>>>   goto error;
>>>   }
>>>
>>> -/* Make name safe to use with mkstemp by replacing '/' with '_'. */
>>> -sanitized_name = g_strdup(memory_region_name(block->mr));
>>> -for (c = sanitized_name; *c != '\0'; c++) {
>>> -if (*c == '/')
>>> -*c = '_';
>>> -}
>>> -
>>> -filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
>>> -   sanitized_name);
>>> -g_free(sanitized_name);
>>> +memory = ROUND_UP(memory, pagesize);
>>>
>>> -fd = mkstemp(filename);
>>> +fd = open_ram_file_path(block, path, memory);
>>>   if (fd < 0) {
>>>   error_setg_errno(errp, errno,
>>>"unable to create backing store for path
>>> %s", path);
>>> -g_free(filename);
>>>   goto error;
>>>   }
>>> -unlink(filename);
>>> -g_free(filename);
>>> -
>>> -memory = ROUND_UP(memory, pagesize);
>>> -
>>> -/*
>>> - * ftruncate is not supported by hugetlbfs in older
>>> - * hosts, so don't bother bailing out on errors.
>>> - * If anything goes wrong with it under other filesystems,
>>> - * mmap will fail.
>>> - */
>>> -if (ftruncate(fd, memory)) {
>>> -perror("ftruncate");
>>> -}
>>>
>>>   area = qemu_ram_mmap(fd, memory, pagesize, block->flags &
>>> RAM_SHARED);
>>>   if (area == MAP_FAILED) {
>>>
>>
>> I was going to send tomorrow a pull request for a similar patch,
>> "backends/hostmem-file: Allow to specify full pathname for backing file".
>>
>> The main difference seems to be your usage of O_EXCL.  Can you explain
>> why you added it?
> 
> It' used if we pass a block device as a NVDIMM backend memory:
>  O_EXCL can be used without O_CREAT if pathname refers to a block
> device.  If the block device
>  is in use by the system (e.g., mounted), open() fails with the error EBUSY

That makes sense, but I think it's better to be consistent with the
handling of block devices.  Block devices do not use O_EXCL when QEMU
opens them; I guess in principle it would also be possible to share a
single pmem backend between multiple guests.

Paolo
--
To unsubscribe from this

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-03 Thread Xiao Guangrong 



On 11/03/2015 09:55 PM, Paolo Bonzini wrote:



On 03/11/2015 04:56, Xiao Guangrong wrote:



On 11/03/2015 05:12 AM, Paolo Bonzini wrote:



On 02/11/2015 10:13, Xiao Guangrong wrote:

Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a
directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Signed-off-by: Xiao Guangrong 
---
   exec.c | 80
++
   1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 9075f4d..db0fdaf 100644
--- a/exec.c
+++ b/exec.c
@@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
   }

   #ifdef __linux__
+static bool path_is_dir(const char *path)
+{
+struct stat fs;
+
+return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
+}
+
+static int open_ram_file_path(RAMBlock *block, const char *path,
size_t size)
+{
+char *filename;
+char *sanitized_name;
+char *c;
+int fd;
+
+if (!path_is_dir(path)) {
+int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
+
+flags |= O_EXCL;
+return open(path, flags);
+}
+
+/* Make name safe to use with mkstemp by replacing '/' with '_'. */
+sanitized_name = g_strdup(memory_region_name(block->mr));
+for (c = sanitized_name; *c != '\0'; c++) {
+if (*c == '/') {
+*c = '_';
+}
+}
+filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
+   sanitized_name);
+g_free(sanitized_name);
+fd = mkstemp(filename);
+if (fd >= 0) {
+unlink(filename);
+/*
+ * ftruncate is not supported by hugetlbfs in older
+ * hosts, so don't bother bailing out on errors.
+ * If anything goes wrong with it under other filesystems,
+ * mmap will fail.
+ */
+if (ftruncate(fd, size)) {
+perror("ftruncate");
+}
+}
+g_free(filename);
+
+return fd;
+}
+
   static void *file_ram_alloc(RAMBlock *block,
   ram_addr_t memory,
   const char *path,
   Error **errp)
   {
-char *filename;
-char *sanitized_name;
-char *c;
   void *area;
   int fd;
   uint64_t pagesize;
@@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
   goto error;
   }

-/* Make name safe to use with mkstemp by replacing '/' with '_'. */
-sanitized_name = g_strdup(memory_region_name(block->mr));
-for (c = sanitized_name; *c != '\0'; c++) {
-if (*c == '/')
-*c = '_';
-}
-
-filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
-   sanitized_name);
-g_free(sanitized_name);
+memory = ROUND_UP(memory, pagesize);

-fd = mkstemp(filename);
+fd = open_ram_file_path(block, path, memory);
   if (fd < 0) {
   error_setg_errno(errp, errno,
"unable to create backing store for path
%s", path);
-g_free(filename);
   goto error;
   }
-unlink(filename);
-g_free(filename);
-
-memory = ROUND_UP(memory, pagesize);
-
-/*
- * ftruncate is not supported by hugetlbfs in older
- * hosts, so don't bother bailing out on errors.
- * If anything goes wrong with it under other filesystems,
- * mmap will fail.
- */
-if (ftruncate(fd, memory)) {
-perror("ftruncate");
-}

   area = qemu_ram_mmap(fd, memory, pagesize, block->flags &
RAM_SHARED);
   if (area == MAP_FAILED) {



I was going to send tomorrow a pull request for a similar patch,
"backends/hostmem-file: Allow to specify full pathname for backing file".

The main difference seems to be your usage of O_EXCL.  Can you explain
why you added it?


It' used if we pass a block device as a NVDIMM backend memory:
  O_EXCL can be used without O_CREAT if pathname refers to a block
device.  If the block device
  is in use by the system (e.g., mounted), open() fails with the error EBUSY


That makes sense, but I think it's better to be consistent with the
handling of block devices.  Block devices do not use O_EXCL when QEMU
opens them; I guess in principle it would also be possible to share a
single pmem backend between multiple guests.


Yup. Will make a separate patch to do this. :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-02 Thread Vladimir Sementsov-Ogievskiy 

On 02.11.2015 12:13, Xiao Guangrong wrote:

Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a


It isn't try to allow, it allows, as I understand)


directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Signed-off-by: Xiao Guangrong 
---
  exec.c | 80 ++
  1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 9075f4d..db0fdaf 100644
--- a/exec.c
+++ b/exec.c
@@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
  }
  
  #ifdef __linux__

+static bool path_is_dir(const char *path)
+{
+struct stat fs;
+
+return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
+}
+
+static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
+{
+char *filename;
+char *sanitized_name;
+char *c;
+int fd;
+
+if (!path_is_dir(path)) {
+int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
+
+flags |= O_EXCL;
+return open(path, flags);
+}
+
+/* Make name safe to use with mkstemp by replacing '/' with '_'. */
+sanitized_name = g_strdup(memory_region_name(block->mr));
+for (c = sanitized_name; *c != '\0'; c++) {
+if (*c == '/') {
+*c = '_';
+}
+}
+filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
+   sanitized_name);
+g_free(sanitized_name);


one empty line will be very nice here, and it was in master branch


+fd = mkstemp(filename);
+if (fd >= 0) {
+unlink(filename);
+/*
+ * ftruncate is not supported by hugetlbfs in older
+ * hosts, so don't bother bailing out on errors.
+ * If anything goes wrong with it under other filesystems,
+ * mmap will fail.
+ */
+if (ftruncate(fd, size)) {
+perror("ftruncate");
+}
+}
+g_free(filename);
+
+return fd;
+}
+
  static void *file_ram_alloc(RAMBlock *block,
  ram_addr_t memory,
  const char *path,
  Error **errp)
  {
-char *filename;
-char *sanitized_name;
-char *c;
  void *area;
  int fd;
  uint64_t pagesize;
@@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
  goto error;
  }
  
-/* Make name safe to use with mkstemp by replacing '/' with '_'. */

-sanitized_name = g_strdup(memory_region_name(block->mr));
-for (c = sanitized_name; *c != '\0'; c++) {
-if (*c == '/')
-*c = '_';
-}
-
-filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
-   sanitized_name);
-g_free(sanitized_name);
+memory = ROUND_UP(memory, pagesize);
  
-fd = mkstemp(filename);

+fd = open_ram_file_path(block, path, memory);
  if (fd < 0) {
  error_setg_errno(errp, errno,
   "unable to create backing store for path %s", path);
-g_free(filename);
  goto error;
  }
-unlink(filename);
-g_free(filename);
-
-memory = ROUND_UP(memory, pagesize);
-
-/*
- * ftruncate is not supported by hugetlbfs in older
- * hosts, so don't bother bailing out on errors.
- * If anything goes wrong with it under other filesystems,
- * mmap will fail.
- */
-if (ftruncate(fd, memory)) {
-perror("ftruncate");
-}
  
  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);

  if (area == MAP_FAILED) {



Reviewed-by: Vladimir Sementsov-Ogievskiy 

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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-02 Thread Xiao Guangrong 



On 11/02/2015 11:12 PM, Vladimir Sementsov-Ogievskiy wrote:

On 02.11.2015 12:13, Xiao Guangrong wrote:

Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a


It isn't try to allow, it allows, as I understand)...


Err... Sorry for my English, but what is the different between:
”This patch tries to allow it to work on file directly“ and
"This patch allows it to work on file directly"

:(




directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Signed-off-by: Xiao Guangrong 
---
  exec.c | 80 ++
  1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 9075f4d..db0fdaf 100644
--- a/exec.c
+++ b/exec.c
@@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
  }
  #ifdef __linux__
+static bool path_is_dir(const char *path)
+{
+struct stat fs;
+
+return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
+}
+
+static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
+{
+char *filename;
+char *sanitized_name;
+char *c;
+int fd;
+
+if (!path_is_dir(path)) {
+int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
+
+flags |= O_EXCL;
+return open(path, flags);
+}
+
+/* Make name safe to use with mkstemp by replacing '/' with '_'. */
+sanitized_name = g_strdup(memory_region_name(block->mr));
+for (c = sanitized_name; *c != '\0'; c++) {
+if (*c == '/') {
+*c = '_';
+}
+}
+filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
+   sanitized_name);
+g_free(sanitized_name);


one empty line will be very nice here, and it was in master branch


+fd = mkstemp(filename);
+if (fd >= 0) {
+unlink(filename);
+/*
+ * ftruncate is not supported by hugetlbfs in older
+ * hosts, so don't bother bailing out on errors.
+ * If anything goes wrong with it under other filesystems,
+ * mmap will fail.
+ */
+if (ftruncate(fd, size)) {
+perror("ftruncate");
+}
+}
+g_free(filename);
+
+return fd;
+}
+
  static void *file_ram_alloc(RAMBlock *block,
  ram_addr_t memory,
  const char *path,
  Error **errp)
  {
-char *filename;
-char *sanitized_name;
-char *c;
  void *area;
  int fd;
  uint64_t pagesize;
@@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
  goto error;
  }
-/* Make name safe to use with mkstemp by replacing '/' with '_'. */
-sanitized_name = g_strdup(memory_region_name(block->mr));
-for (c = sanitized_name; *c != '\0'; c++) {
-if (*c == '/')
-*c = '_';
-}
-
-filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
-   sanitized_name);
-g_free(sanitized_name);
+memory = ROUND_UP(memory, pagesize);
-fd = mkstemp(filename);
+fd = open_ram_file_path(block, path, memory);
  if (fd < 0) {
  error_setg_errno(errp, errno,
   "unable to create backing store for path %s", path);
-g_free(filename);
  goto error;
  }
-unlink(filename);
-g_free(filename);
-
-memory = ROUND_UP(memory, pagesize);
-
-/*
- * ftruncate is not supported by hugetlbfs in older
- * hosts, so don't bother bailing out on errors.
- * If anything goes wrong with it under other filesystems,
- * mmap will fail.
- */
-if (ftruncate(fd, memory)) {
-perror("ftruncate");
-}
  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);
  if (area == MAP_FAILED) {



Reviewed-by: Vladimir Sementsov-Ogievskiy 


Thanks for your review.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-02 Thread Paolo Bonzini 


On 02/11/2015 10:13, Xiao Guangrong wrote:
> Currently, file_ram_alloc() only works on directory - it creates a file
> under @path and do mmap on it
> 
> This patch tries to allow it to work on file directly, if @path is a
> directory it works as before, otherwise it treats @path as the target
> file then directly allocate memory from it
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  exec.c | 80 
> ++
>  1 file changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 9075f4d..db0fdaf 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
>  }
>  
>  #ifdef __linux__
> +static bool path_is_dir(const char *path)
> +{
> +struct stat fs;
> +
> +return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
> +}
> +
> +static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
> +{
> +char *filename;
> +char *sanitized_name;
> +char *c;
> +int fd;
> +
> +if (!path_is_dir(path)) {
> +int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
> +
> +flags |= O_EXCL;
> +return open(path, flags);
> +}
> +
> +/* Make name safe to use with mkstemp by replacing '/' with '_'. */
> +sanitized_name = g_strdup(memory_region_name(block->mr));
> +for (c = sanitized_name; *c != '\0'; c++) {
> +if (*c == '/') {
> +*c = '_';
> +}
> +}
> +filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
> +   sanitized_name);
> +g_free(sanitized_name);
> +fd = mkstemp(filename);
> +if (fd >= 0) {
> +unlink(filename);
> +/*
> + * ftruncate is not supported by hugetlbfs in older
> + * hosts, so don't bother bailing out on errors.
> + * If anything goes wrong with it under other filesystems,
> + * mmap will fail.
> + */
> +if (ftruncate(fd, size)) {
> +perror("ftruncate");
> +}
> +}
> +g_free(filename);
> +
> +return fd;
> +}
> +
>  static void *file_ram_alloc(RAMBlock *block,
>  ram_addr_t memory,
>  const char *path,
>  Error **errp)
>  {
> -char *filename;
> -char *sanitized_name;
> -char *c;
>  void *area;
>  int fd;
>  uint64_t pagesize;
> @@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
>  goto error;
>  }
>  
> -/* Make name safe to use with mkstemp by replacing '/' with '_'. */
> -sanitized_name = g_strdup(memory_region_name(block->mr));
> -for (c = sanitized_name; *c != '\0'; c++) {
> -if (*c == '/')
> -*c = '_';
> -}
> -
> -filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
> -   sanitized_name);
> -g_free(sanitized_name);
> +memory = ROUND_UP(memory, pagesize);
>  
> -fd = mkstemp(filename);
> +fd = open_ram_file_path(block, path, memory);
>  if (fd < 0) {
>  error_setg_errno(errp, errno,
>   "unable to create backing store for path %s", path);
> -g_free(filename);
>  goto error;
>  }
> -unlink(filename);
> -g_free(filename);
> -
> -memory = ROUND_UP(memory, pagesize);
> -
> -/*
> - * ftruncate is not supported by hugetlbfs in older
> - * hosts, so don't bother bailing out on errors.
> - * If anything goes wrong with it under other filesystems,
> - * mmap will fail.
> - */
> -if (ftruncate(fd, memory)) {
> -perror("ftruncate");
> -}
>  
>  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);
>  if (area == MAP_FAILED) {
> 

I was going to send tomorrow a pull request for a similar patch,
"backends/hostmem-file: Allow to specify full pathname for backing file".

The main difference seems to be your usage of O_EXCL.  Can you explain
why you added it?

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/35] exec: allow file_ram_alloc to work on file

2015-11-02 Thread Xiao Guangrong 



On 11/03/2015 05:12 AM, Paolo Bonzini wrote:



On 02/11/2015 10:13, Xiao Guangrong wrote:

Currently, file_ram_alloc() only works on directory - it creates a file
under @path and do mmap on it

This patch tries to allow it to work on file directly, if @path is a
directory it works as before, otherwise it treats @path as the target
file then directly allocate memory from it

Signed-off-by: Xiao Guangrong 
---
  exec.c | 80 ++
  1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 9075f4d..db0fdaf 100644
--- a/exec.c
+++ b/exec.c
@@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void)
  }

  #ifdef __linux__
+static bool path_is_dir(const char *path)
+{
+struct stat fs;
+
+return stat(path, ) == 0 && S_ISDIR(fs.st_mode);
+}
+
+static int open_ram_file_path(RAMBlock *block, const char *path, size_t size)
+{
+char *filename;
+char *sanitized_name;
+char *c;
+int fd;
+
+if (!path_is_dir(path)) {
+int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY;
+
+flags |= O_EXCL;
+return open(path, flags);
+}
+
+/* Make name safe to use with mkstemp by replacing '/' with '_'. */
+sanitized_name = g_strdup(memory_region_name(block->mr));
+for (c = sanitized_name; *c != '\0'; c++) {
+if (*c == '/') {
+*c = '_';
+}
+}
+filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
+   sanitized_name);
+g_free(sanitized_name);
+fd = mkstemp(filename);
+if (fd >= 0) {
+unlink(filename);
+/*
+ * ftruncate is not supported by hugetlbfs in older
+ * hosts, so don't bother bailing out on errors.
+ * If anything goes wrong with it under other filesystems,
+ * mmap will fail.
+ */
+if (ftruncate(fd, size)) {
+perror("ftruncate");
+}
+}
+g_free(filename);
+
+return fd;
+}
+
  static void *file_ram_alloc(RAMBlock *block,
  ram_addr_t memory,
  const char *path,
  Error **errp)
  {
-char *filename;
-char *sanitized_name;
-char *c;
  void *area;
  int fd;
  uint64_t pagesize;
@@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block,
  goto error;
  }

-/* Make name safe to use with mkstemp by replacing '/' with '_'. */
-sanitized_name = g_strdup(memory_region_name(block->mr));
-for (c = sanitized_name; *c != '\0'; c++) {
-if (*c == '/')
-*c = '_';
-}
-
-filename = g_strdup_printf("%s/qemu_back_mem.%s.XX", path,
-   sanitized_name);
-g_free(sanitized_name);
+memory = ROUND_UP(memory, pagesize);

-fd = mkstemp(filename);
+fd = open_ram_file_path(block, path, memory);
  if (fd < 0) {
  error_setg_errno(errp, errno,
   "unable to create backing store for path %s", path);
-g_free(filename);
  goto error;
  }
-unlink(filename);
-g_free(filename);
-
-memory = ROUND_UP(memory, pagesize);
-
-/*
- * ftruncate is not supported by hugetlbfs in older
- * hosts, so don't bother bailing out on errors.
- * If anything goes wrong with it under other filesystems,
- * mmap will fail.
- */
-if (ftruncate(fd, memory)) {
-perror("ftruncate");
-}

  area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED);
  if (area == MAP_FAILED) {



I was going to send tomorrow a pull request for a similar patch,
"backends/hostmem-file: Allow to specify full pathname for backing file".

The main difference seems to be your usage of O_EXCL.  Can you explain
why you added it?


It' used if we pass a block device as a NVDIMM backend memory:
 O_EXCL can be used without O_CREAT if pathname refers to a block device.  If 
the block device
 is in use by the system (e.g., mounted), open() fails with the error EBUSY
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html