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

[Antonios Motakis]

On Tue, Jan 14, 2014 at 12:16 PM, Michael S. Tsirkin <address@hidden> wrote:

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.

We added this feature based on the comments we received on this mailing list from reviewers. We do not need it from our point of view, so for us it is straightforward to remove it.

 

> ---
>  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]
> address@hidden -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
>