Re: [Qemu-devel] [PATCH v6 2/8] New -mem-path option - unlink.

[Michael S. Tsirkin]

On Mon, Jan 13, 2014 at 03:25:13PM +0100, Antonios Motakis wrote:
> The unlink option allows the created file to be externally deleted
> after QEMU is terminated.
> 
>  - unlink=on|off - default on, unlink the file after opening it
> 
> Signed-off-by: Antonios Motakis <address@hidden>
> Signed-off-by: Nikolay Nikolaev <address@hidden>


I have doubts about this patch.

In particular we seem to commit to a specific
file naming scheme without ever documenting
its users or adding any tests.

Please document who uses this in the commit log,
document the scheme in docs/ and add a test so we
don't break this without noticing.


> ---
>  exec.c          | 18 +++++++++++++-----
>  qemu-options.hx |  7 ++++---
>  vl.c            |  4 ++++
>  3 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 1c40a0d..30f4019 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -999,7 +999,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      int flags;
>      unsigned long hpagesize;
>      QemuOpts *opts;
> -    unsigned int mem_prealloc = 0, mem_share = 0;
> +    unsigned int mem_prealloc = 0, mem_share = 0, mem_unlink = 1;
>  
>      hpagesize = gethugepagesize(path);
>      if (!hpagesize) {
> @@ -1020,6 +1020,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      if (opts) {
>          mem_prealloc = qemu_opt_get_bool(opts, "prealloc", 0);
>          mem_share = qemu_opt_get_bool(opts, "share", 0);
> +        mem_unlink = qemu_opt_get_bool(opts, "unlink", 1);
>      }
>  
>      /* Make name safe to use with mkstemp by replacing '/' with '_'. */
> @@ -1029,18 +1030,25 @@ static void *file_ram_alloc(RAMBlock *block,
>              *c = '_';
>      }
>  
> -    filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path,
> -                               sanitized_name);
> +    filename = g_strdup_printf("%s/qemu_back_mem.%s%s", path, sanitized_name,
> +                               (mem_unlink) ? ".XXXXXX" : "");
>      g_free(sanitized_name);
>  
> -    fd = mkstemp(filename);
> +    if (mem_unlink) {
> +        fd = mkstemp(filename);
> +    } else {
> +        fd = open(filename, O_CREAT | O_RDWR | O_EXCL,
> +                S_IRWXU | S_IRWXG | S_IRWXO);
> +    }
>      if (fd < 0) {
>          perror("unable to create guest RAM backing store");
>          g_free(filename);
>          return NULL;
>      }
>  
> -    unlink(filename);
> +    if (mem_unlink) {
> +        unlink(filename);
> +    }
>      g_free(filename);
>  
>      memory = (memory + hpagesize - 1) & ~(hpagesize - 1);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 60ecc95..a12af97 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -221,14 +221,15 @@ gigabytes respectively.
>  ETEXI
>  
>  DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath,
> -    "-mem-path [path=]path[,prealloc=on|off][,share=on|off]\n"
> +    "-mem-path 
> [path=]path[,prealloc=on|off][,share=on|off][,unlink=on|off]\n"
>      "                provide backing storage for guest RAM\n"
>      "                path= a directory path for the backing store\n"
>      "                prealloc= preallocate guest memory [default disabled]\n"
> -    "                share= enable mmap share flag [default disabled]\n",
> +    "                share= enable mmap share flag [default disabled]\n"
> +    "                unlink= enable unlinking the guest RAM files [default 
> enabled]\n",
>          QEMU_ARCH_ALL)
>  STEXI
> address@hidden -mem-path address@hidden,prealloc=on|off][,share=on|off]
> address@hidden -mem-path 
> address@hidden,prealloc=on|off][,share=on|off][,unlink=on|off]
>  @findex -mem-path
>  Allocate guest RAM from a temporarily created file in @var{path}.
>  ETEXI
> diff --git a/vl.c b/vl.c
> index e98abc8..5034bb6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -546,6 +546,10 @@ static QemuOptsList qemu_mem_path_opts = {
>              .name = "share",
>              .type = QEMU_OPT_BOOL,
>          },
> +        {
> +            .name = "unlink",
> +            .type = QEMU_OPT_BOOL,
> +        },
>          { /* end of list */ }
>      },
>  };
> -- 
> 1.8.3.2
>