[Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Eduardo Habkost]

This is a resubmit of a previous series I sent as a RFC, with some changes to
prepare for an upcoming patch that will make additional changes to the default
config-file loading code.

This series needs be applied on top of the "./configure --confdir" series I
sent today.

Changes v1 -> v2:
 - Moved some code to qemu-config-arch.c, as it will eventually have
   arch-specific features

Eduardo Habkost (3):
  qemu-config.h: include qemu-option.h
  -readconfig: use QemuOpts option format (v2)
  -readconfig: accept fd=<fd> option (v2)

 Makefile.target    |    2 +-
 qemu-config-arch.c |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-config-arch.h |   15 +++++++++
 qemu-config.c      |   46 ++++++++++++++++++++++-----
 qemu-config.h      |   10 +++++-
 qemu-options.hx    |    6 ++--
 vl.c               |    7 ++--
 7 files changed, 155 insertions(+), 17 deletions(-)
 create mode 100644 qemu-config-arch.c
 create mode 100644 qemu-config-arch.h

-- 
1.7.3.2

[Qemu-devel] [PATCH 1/3] qemu-config.h: include qemu-option.h

[Eduardo Habkost]

It uses QemuOptList, so it needs qemu-option.h to be included.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-config.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/qemu-config.h b/qemu-config.h
index 20d707f..ba7796c 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_CONFIG_H
 #define QEMU_CONFIG_H
 
+#include "qemu-option.h"
+
 extern QemuOptsList qemu_fsdev_opts;
 extern QemuOptsList qemu_virtfs_opts;
 extern QemuOptsList qemu_spice_opts;
-- 
1.7.3.2

[Qemu-devel] [PATCH 2/3] -readconfig: use QemuOpts option format (v2)

[Eduardo Habkost]

This changes -readconfig to use the name=value[,name=value] option
format.

It keeps "-readconfig filename" working by using the implied "path"
option name, but it changes the existing syntax a little, meaning that
files with "=" and "," in the filename have to be escaped.

I chose to break compatibility in those cases, instead of adding a new
option and keeping -readconfig as a legacy option. If you think adding a
new option is better, please let me know.

Most of the code is being added to qemu-config-arch.c because the
command-line handling of -readconfig will eventually depend on
arch-specific stuff, such as the list of default config files, and
qemu-config.o is not arch-specific.

Changes v1 -> v2:
 - Create qemu-config-arch.c for the readconfig-specific code
   that will depend on arch-specific stuff

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 Makefile.target    |    2 +-
 qemu-config-arch.c |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-config-arch.h |   15 +++++++++++
 qemu-config.c      |   34 +++++++++++++++++--------
 qemu-config.h      |    4 ++-
 qemu-options.hx    |    4 +-
 vl.c               |    7 +++--
 7 files changed, 117 insertions(+), 18 deletions(-)
 create mode 100644 qemu-config-arch.c
 create mode 100644 qemu-config-arch.h

diff --git a/Makefile.target b/Makefile.target
index 37fb7ed..e48044b 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -204,7 +204,7 @@ endif #CONFIG_BSD_USER
 # System emulator target
 ifdef CONFIG_SOFTMMU
 
-obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o ioport.o
+obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o ioport.o qemu-config-arch.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 obj-$(CONFIG_NO_PCI) += pci-stub.o
diff --git a/qemu-config-arch.c b/qemu-config-arch.c
new file mode 100644
index 0000000..9180b73
--- /dev/null
+++ b/qemu-config-arch.c
@@ -0,0 +1,69 @@
+/*
+ * Arch-specific Qemu config handling
+ *
+ * Copyright (c) 2012 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu-config-arch.h"
+#include "qemu-option.h"
+
+/* Not on vm_config_groups because this is not a global option setable by -set
+ * (QEMU_OPTION_set), just settings used to parse -readconfig argument.
+ */
+static QemuOptsList qemu_readconfig_opts = {
+    .name = "readconfig-opts",
+    .implied_opt_name = "path",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_readconfig_opts.head),
+    .desc = {
+        {
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+        },
+        { /*End of list */ }
+    },
+};
+
+/* Read Qemu config file based on parsed QemuOpts object
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+static int qemu_read_config_opts(QemuOpts *opts)
+{
+    const char *path = qemu_opt_get(opts, "path");
+    if (!path) {
+        return -EINVAL;
+    }
+    return qemu_read_config_filename(path);
+}
+
+/* Read config file based on option arguments on 'arg'
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+int qemu_read_config_arg(const char *arg)
+{
+    QemuOpts *opts = qemu_opts_parse(&qemu_readconfig_opts, arg, 1);
+    if (!opts) {
+        return -EINVAL;
+    }
+    int r = qemu_read_config_opts(opts);
+    qemu_opts_del(opts);
+    return r;
+}
diff --git a/qemu-config-arch.h b/qemu-config-arch.h
new file mode 100644
index 0000000..3a9943d
--- /dev/null
+++ b/qemu-config-arch.h
@@ -0,0 +1,15 @@
+#ifndef QEMU_CONFIG_ARCH_H
+#define QEMU_CONFIG_ARCH_H
+/* Arch-specific qemu config code
+ *
+ * Contains functions that depend on arch-specific variables and settings,
+ * such as the handling of -readconfig help options.
+ */
+
+#include "qemu-config.h"
+
+/* Read config based on key=value options using QemuOpts syntax
+ */
+int qemu_read_config_arg(const char *arg);
+
+#endif /* QEMU_CONFIG_ARCH_H */
diff --git a/qemu-config.c b/qemu-config.c
index be84a03..a2c8453 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -816,21 +816,33 @@ out:
     return res;
 }
 
-int qemu_read_config_file(const char *filename)
+/* Read Qemu config file from open file
+ *
+ * The file will be closed before returning.
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+static int qemu_read_config_file(FILE *f, const char *filename)
 {
-    FILE *f = fopen(filename, "r");
     int ret;
-
-    if (f == NULL) {
-        return -errno;
+    if (qemu_config_parse(f, vm_config_groups, filename) == 0) {
+        ret = 0;
+    } else {
+        ret = -EINVAL;
     }
-
-    ret = qemu_config_parse(f, vm_config_groups, filename);
     fclose(f);
+    return ret;
+}
 
-    if (ret == 0) {
-        return 0;
-    } else {
-        return -EINVAL;
+/* Read Qemu config file from 'filename'
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+int qemu_read_config_filename(const char *filename)
+{
+    FILE *f = fopen(filename, "r");
+    if (f == NULL) {
+        return -errno;
     }
+    return qemu_read_config_file(f, filename);
 }
diff --git a/qemu-config.h b/qemu-config.h
index ba7796c..c2ab4fc 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -16,6 +16,8 @@ void qemu_add_globals(void);
 void qemu_config_write(FILE *fp);
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
 
-int qemu_read_config_file(const char *filename);
+/* Read config from filename
+ */
+int qemu_read_config_filename(const char *filename);
 
 #endif /* QEMU_CONFIG_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index 39578f1..5d14f92 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2655,9 +2655,9 @@ Old param mode (ARM only).
 ETEXI
 
 DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
-    "-readconfig <file>\n", QEMU_ARCH_ALL)
+    "-readconfig [path=]<file>\n", QEMU_ARCH_ALL)
 STEXI
-@item -readconfig @var{file}
+@item -readconfig [type=]@var{file}
 @findex -readconfig
 Read device configuration from @var{file}.
 ETEXI
diff --git a/vl.c b/vl.c
index bd95539..26e6738 100644
--- a/vl.c
+++ b/vl.c
@@ -146,6 +146,7 @@ int main(int argc, char **argv)
 #include "qjson.h"
 #include "qemu-option.h"
 #include "qemu-config.h"
+#include "qemu-config-arch.h"
 #include "qemu-options.h"
 #include "qmp-commands.h"
 #include "main-loop.h"
@@ -2351,12 +2352,12 @@ int main(int argc, char **argv, char **envp)
     if (defconfig) {
         int ret;
 
-        ret = qemu_read_config_file(CONFIG_QEMU_CONFDIR "/qemu.conf");
+        ret = qemu_read_config_filename(CONFIG_QEMU_CONFDIR "/qemu.conf");
         if (ret < 0 && ret != -ENOENT) {
             exit(1);
         }
 
-        ret = qemu_read_config_file(arch_config_name);
+        ret = qemu_read_config_filename(arch_config_name);
         if (ret < 0 && ret != -ENOENT) {
             exit(1);
         }
@@ -3144,7 +3145,7 @@ int main(int argc, char **argv, char **envp)
             }
             case QEMU_OPTION_readconfig:
                 {
-                    int ret = qemu_read_config_file(optarg);
+                    int ret = qemu_read_config_arg(optarg);
                     if (ret < 0) {
                         fprintf(stderr, "read config %s: %s\n", optarg,
                             strerror(-ret));
-- 
1.7.3.2

[Qemu-devel] [PATCH 3/3] -readconfig: accept fd=<fd> option (v2)

[Eduardo Habkost]

Chhanges v1 -> v2:
 - Moved code to qemu-config-arch.c

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-config-arch.c |   21 +++++++++++++++++++--
 qemu-config.c      |   16 ++++++++++++++++
 qemu-config.h      |    4 ++++
 qemu-options.hx    |    6 +++---
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/qemu-config-arch.c b/qemu-config-arch.c
index 9180b73..ee5d515 100644
--- a/qemu-config-arch.c
+++ b/qemu-config-arch.c
@@ -23,6 +23,7 @@
  */
 #include "qemu-config-arch.h"
 #include "qemu-option.h"
+#include "qemu-error.h"
 
 /* Not on vm_config_groups because this is not a global option setable by -set
  * (QEMU_OPTION_set), just settings used to parse -readconfig argument.
@@ -36,21 +37,37 @@ static QemuOptsList qemu_readconfig_opts = {
             .name = "path",
             .type = QEMU_OPT_STRING,
         },
+        {
+            .name = "fd",
+            .type = QEMU_OPT_NUMBER,
+        },
         { /*End of list */ }
     },
 };
 
+
 /* Read Qemu config file based on parsed QemuOpts object
  *
  * Returns 0 on success, -errno on failure.
  */
 static int qemu_read_config_opts(QemuOpts *opts)
 {
+    int fd = -1;
+    uint64_t fd_arg = qemu_opt_get_number(opts, "fd", (uint64_t)-1);
     const char *path = qemu_opt_get(opts, "path");
-    if (!path) {
+
+    if (fd_arg != (uint64_t)-1) {
+        fd = fd_arg;
+    }
+
+    if (path) {
+        return qemu_read_config_filename(path);
+    } else if (fd >= 0) {
+        return qemu_read_config_fd(fd);
+    } else {
+        error_report("no fd or path set for config file");
         return -EINVAL;
     }
-    return qemu_read_config_filename(path);
 }
 
 /* Read config file based on option arguments on 'arg'
diff --git a/qemu-config.c b/qemu-config.c
index a2c8453..750626d 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -846,3 +846,19 @@ int qemu_read_config_filename(const char *filename)
     }
     return qemu_read_config_file(f, filename);
 }
+
+/* Read Qemu config file from file descriptor
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+int qemu_read_config_fd(int fd)
+{
+    /* For the "<fd:%d>" pseudo-filename, used only for error messages */
+    char fname[16];
+    FILE *f = fdopen(fd, "r");
+    if (f == NULL) {
+        return -errno;
+    }
+    snprintf(fname, sizeof(fname), "<fd:%d>", fd);
+    return qemu_read_config_file(f, fname);
+}
diff --git a/qemu-config.h b/qemu-config.h
index c2ab4fc..28356a6 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -20,4 +20,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
  */
 int qemu_read_config_filename(const char *filename);
 
+/* Read config from file descriptor
+ */
+int qemu_read_config_fd(int fd);
+
 #endif /* QEMU_CONFIG_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index 5d14f92..ae64ca8 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2655,11 +2655,11 @@ Old param mode (ARM only).
 ETEXI
 
 DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
-    "-readconfig [path=]<file>\n", QEMU_ARCH_ALL)
+    "-readconfig [path=]<file>|fd=<fd>\n", QEMU_ARCH_ALL)
 STEXI
-@item -readconfig [type=]@var{file}
+@item -readconfig [path=]@var{file}|fd=@var{fd}
 @findex -readconfig
-Read device configuration from @var{file}.
+Read device configuration from @var{file}, or from file descriptor @var{fd}.
 ETEXI
 DEF("writeconfig", HAS_ARG, QEMU_OPTION_writeconfig,
     "-writeconfig <file>\n"
-- 
1.7.3.2

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Anthony Liguori]

On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
> This is a resubmit of a previous series I sent as a RFC, with some changes to
> prepare for an upcoming patch that will make additional changes to the default
> config-file loading code.
>
> This series needs be applied on top of the "./configure --confdir" series I
> sent today.

Why not just use /dev/fd/N  ?

Regards,

Anthony Liguori

>
> Changes v1 ->  v2:
>   - Moved some code to qemu-config-arch.c, as it will eventually have
>     arch-specific features
>
> Eduardo Habkost (3):
>    qemu-config.h: include qemu-option.h
>    -readconfig: use QemuOpts option format (v2)
>    -readconfig: accept fd=<fd>  option (v2)
>
>   Makefile.target    |    2 +-
>   qemu-config-arch.c |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qemu-config-arch.h |   15 +++++++++
>   qemu-config.c      |   46 ++++++++++++++++++++++-----
>   qemu-config.h      |   10 +++++-
>   qemu-options.hx    |    6 ++--
>   vl.c               |    7 ++--
>   7 files changed, 155 insertions(+), 17 deletions(-)
>   create mode 100644 qemu-config-arch.c
>   create mode 100644 qemu-config-arch.h
>

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Eduardo Habkost]

On Mon, Mar 19, 2012 at 10:10:27AM -0500, Anthony Liguori wrote:
> On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
> >This is a resubmit of a previous series I sent as a RFC, with some changes to
> >prepare for an upcoming patch that will make additional changes to the default
> >config-file loading code.
> >
> >This series needs be applied on top of the "./configure --confdir" series I
> >sent today.
> 
> Why not just use /dev/fd/N  ?

Personally, I don't like filenames with special meanings (as not every
OS has /dev/fd we would have to treat them specially), or filenames that
become non-extensible mini-languages by themselves. Many other
command-line options use the key=value syntax, and some already have an
"fd" option, so this just follows the convention.

Also, this is more extensible to allow more options to be added to
-readconfig if needed (for example: debugging options, or the
help=defconfig option I added on the RFC series I sent after this one).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Anthony Liguori]

On 03/19/2012 10:37 AM, Eduardo Habkost wrote:
> On Mon, Mar 19, 2012 at 10:10:27AM -0500, Anthony Liguori wrote:
>> On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
>>> This is a resubmit of a previous series I sent as a RFC, with some changes to
>>> prepare for an upcoming patch that will make additional changes to the default
>>> config-file loading code.
>>>
>>> This series needs be applied on top of the "./configure --confdir" series I
>>> sent today.
>>
>> Why not just use /dev/fd/N  ?
>
> Personally, I don't like filenames with special meanings (as not every
> OS has /dev/fd we would have to treat them specially), or filenames that
> become non-extensible mini-languages by themselves. Many other
> command-line options use the key=value syntax, and some already have an
> "fd" option, so this just follows the convention.

But you're also breaking compat, which is not something to be done lightly.

>
> Also, this is more extensible to allow more options to be added to
> -readconfig if needed (for example: debugging options, or the
> help=defconfig option I added on the RFC series I sent after this one).

I'd personally prefer to keep readconfig simple.  See the series I sent out as 
an RFC.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Eduardo Habkost]

On Mon, Mar 19, 2012 at 10:39:10AM -0500, Anthony Liguori wrote:
> On 03/19/2012 10:37 AM, Eduardo Habkost wrote:
> >On Mon, Mar 19, 2012 at 10:10:27AM -0500, Anthony Liguori wrote:
> >>On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
> >>>This is a resubmit of a previous series I sent as a RFC, with some changes to
> >>>prepare for an upcoming patch that will make additional changes to the default
> >>>config-file loading code.
> >>>
> >>>This series needs be applied on top of the "./configure --confdir" series I
> >>>sent today.
> >>
> >>Why not just use /dev/fd/N  ?
> >
> >Personally, I don't like filenames with special meanings (as not every
> >OS has /dev/fd we would have to treat them specially), or filenames that
> >become non-extensible mini-languages by themselves. Many other
> >command-line options use the key=value syntax, and some already have an
> >"fd" option, so this just follows the convention.
> 
> But you're also breaking compat, which is not something to be done lightly.

True. In that case, I propose adding a "-config" option that is more
extensible than the current one, instead of defining a new mini-language
for -readconfig that looks like a file path but has hidden complexities
behind it.


> >Also, this is more extensible to allow more options to be added to
> >-readconfig if needed (for example: debugging options, or the
> >help=defconfig option I added on the RFC series I sent after this one).
> 
> I'd personally prefer to keep readconfig simple.  See the series I
> sent out as an RFC.

If you are supporting FDs, the complexity is already there. Using the
key=value format just makes the complexity explicit (and familiar, for
humans and code that already uses key=value for other options) instead
of hiding it behind magic filenames.

I still have to take a look at your series.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Anthony Liguori]

On 03/19/2012 11:06 AM, Eduardo Habkost wrote:
> On Mon, Mar 19, 2012 at 10:39:10AM -0500, Anthony Liguori wrote:
>> On 03/19/2012 10:37 AM, Eduardo Habkost wrote:
>>> On Mon, Mar 19, 2012 at 10:10:27AM -0500, Anthony Liguori wrote:
>>>> On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
>>>>> This is a resubmit of a previous series I sent as a RFC, with some changes to
>>>>> prepare for an upcoming patch that will make additional changes to the default
>>>>> config-file loading code.
>>>>>
>>>>> This series needs be applied on top of the "./configure --confdir" series I
>>>>> sent today.
>>>>
>>>> Why not just use /dev/fd/N  ?
>>>
>>> Personally, I don't like filenames with special meanings (as not every
>>> OS has /dev/fd we would have to treat them specially), or filenames that
>>> become non-extensible mini-languages by themselves. Many other
>>> command-line options use the key=value syntax, and some already have an
>>> "fd" option, so this just follows the convention.
>>
>> But you're also breaking compat, which is not something to be done lightly.
>
> True. In that case, I propose adding a "-config" option that is more
> extensible than the current one, instead of defining a new mini-language
> for -readconfig that looks like a file path but has hidden complexities
> behind it.

What?  /dev/fd is a pretty standard unix feature.  It'll work today with 
existing versions of QEMU.

>
>
>>> Also, this is more extensible to allow more options to be added to
>>> -readconfig if needed (for example: debugging options, or the
>>> help=defconfig option I added on the RFC series I sent after this one).
>>
>> I'd personally prefer to keep readconfig simple.  See the series I
>> sent out as an RFC.
>
> If you are supporting FDs, the complexity is already there. Using the
> key=value format just makes the complexity explicit (and familiar, for
> humans and code that already uses key=value for other options) instead
> of hiding it behind magic filenames.
>
> I still have to take a look at your series.

I think there are cases where /dev/fd doesn't make any sense as an interface 
(like tun/tap) because tun/tap doesn't have normal file semantics.

Likewise, block devices don't act like normal files so you need to treat fd 
differently than you treat a file path.

But for something like the BIOS file, config files, kernels, etc, they are all 
just simple files and instead of making everything take a fd:, we can (and 
should) just use the existing unix mechanism for this.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Eduardo Habkost]

On Mon, Mar 19, 2012 at 11:21:07AM -0500, Anthony Liguori wrote:
> On 03/19/2012 11:06 AM, Eduardo Habkost wrote:
> >On Mon, Mar 19, 2012 at 10:39:10AM -0500, Anthony Liguori wrote:
> >>On 03/19/2012 10:37 AM, Eduardo Habkost wrote:
> >>>On Mon, Mar 19, 2012 at 10:10:27AM -0500, Anthony Liguori wrote:
> >>>>On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
> >>>>>This is a resubmit of a previous series I sent as a RFC, with some changes to
> >>>>>prepare for an upcoming patch that will make additional changes to the default
> >>>>>config-file loading code.
> >>>>>
> >>>>>This series needs be applied on top of the "./configure --confdir" series I
> >>>>>sent today.
> >>>>
> >>>>Why not just use /dev/fd/N  ?
> >>>
> >>>Personally, I don't like filenames with special meanings (as not every
> >>>OS has /dev/fd we would have to treat them specially), or filenames that
> >>>become non-extensible mini-languages by themselves. Many other
> >>>command-line options use the key=value syntax, and some already have an
> >>>"fd" option, so this just follows the convention.
> >>
> >>But you're also breaking compat, which is not something to be done lightly.
> >
> >True. In that case, I propose adding a "-config" option that is more
> >extensible than the current one, instead of defining a new mini-language
> >for -readconfig that looks like a file path but has hidden complexities
> >behind it.
> 
> What?  /dev/fd is a pretty standard unix feature.  It'll work today
> with existing versions of QEMU.

When I replied, I was thinking of this proposal:
http://marc.info/?l=qemu-devel&m=133076148831720
If you are proposing having zero special-case code to Qemu because of
/dev/fd, it's OK to me.

> >>>Also, this is more extensible to allow more options to be added to
> >>>-readconfig if needed (for example: debugging options, or the
> >>>help=defconfig option I added on the RFC series I sent after this one).
> >>
> >>I'd personally prefer to keep readconfig simple.  See the series I
> >>sent out as an RFC.
> >
> >If you are supporting FDs, the complexity is already there. Using the
> >key=value format just makes the complexity explicit (and familiar, for
> >humans and code that already uses key=value for other options) instead
> >of hiding it behind magic filenames.
> >
> >I still have to take a look at your series.
> 
> I think there are cases where /dev/fd doesn't make any sense as an
> interface (like tun/tap) because tun/tap doesn't have normal file
> semantics.
> 
> Likewise, block devices don't act like normal files so you need to
> treat fd differently than you treat a file path.
> 
> But for something like the BIOS file, config files, kernels, etc,
> they are all just simple files and instead of making everything take
> a fd:, we can (and should) just use the existing unix mechanism for
> this.

OK to me, if that doesn't require code to handle "/dev/fd/" as special
inside Qemu, but just use what the OS provides. I am just against making
magic filenames special.

I was still considering adding a key=value interface for -readconfig (or
an equivalent -config parameter) because of the debugging and probing
options I would like to add, but those options could be enabled/disabled
somewhere else, so there wouldn't be a strong reason for it anymore.

On the other hand, I:

- Would like to let the user (a human) list where are the default config
  files being used by Qemu;
- Don't want to add Yet Another "-option ?" parameter to Qemu (e.g.
  "-readconfig ?").

Do you have any suggestions for that?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Anthony Liguori]

On 03/19/2012 11:39 AM, Eduardo Habkost wrote:
> On Mon, Mar 19, 2012 at 11:21:07AM -0500, Anthony Liguori wrote:
>> On 03/19/2012 11:06 AM, Eduardo Habkost wrote:
>>> On Mon, Mar 19, 2012 at 10:39:10AM -0500, Anthony Liguori wrote:
>>>> On 03/19/2012 10:37 AM, Eduardo Habkost wrote:
>>>>> On Mon, Mar 19, 2012 at 10:10:27AM -0500, Anthony Liguori wrote:
>>>>>> On 03/19/2012 09:54 AM, Eduardo Habkost wrote:
>>>>>>> This is a resubmit of a previous series I sent as a RFC, with some changes to
>>>>>>> prepare for an upcoming patch that will make additional changes to the default
>>>>>>> config-file loading code.
>>>>>>>
>>>>>>> This series needs be applied on top of the "./configure --confdir" series I
>>>>>>> sent today.
>>>>>>
>>>>>> Why not just use /dev/fd/N  ?
>>>>>
>>>>> Personally, I don't like filenames with special meanings (as not every
>>>>> OS has /dev/fd we would have to treat them specially), or filenames that
>>>>> become non-extensible mini-languages by themselves. Many other
>>>>> command-line options use the key=value syntax, and some already have an
>>>>> "fd" option, so this just follows the convention.
>>>>
>>>> But you're also breaking compat, which is not something to be done lightly.
>>>
>>> True. In that case, I propose adding a "-config" option that is more
>>> extensible than the current one, instead of defining a new mini-language
>>> for -readconfig that looks like a file path but has hidden complexities
>>> behind it.
>>
>> What?  /dev/fd is a pretty standard unix feature.  It'll work today
>> with existing versions of QEMU.
>
> When I replied, I was thinking of this proposal:
> http://marc.info/?l=qemu-devel&m=133076148831720
> If you are proposing having zero special-case code to Qemu because of
> /dev/fd, it's OK to me.
>
>>>>> Also, this is more extensible to allow more options to be added to
>>>>> -readconfig if needed (for example: debugging options, or the
>>>>> help=defconfig option I added on the RFC series I sent after this one).
>>>>
>>>> I'd personally prefer to keep readconfig simple.  See the series I
>>>> sent out as an RFC.
>>>
>>> If you are supporting FDs, the complexity is already there. Using the
>>> key=value format just makes the complexity explicit (and familiar, for
>>> humans and code that already uses key=value for other options) instead
>>> of hiding it behind magic filenames.
>>>
>>> I still have to take a look at your series.
>>
>> I think there are cases where /dev/fd doesn't make any sense as an
>> interface (like tun/tap) because tun/tap doesn't have normal file
>> semantics.
>>
>> Likewise, block devices don't act like normal files so you need to
>> treat fd differently than you treat a file path.
>>
>> But for something like the BIOS file, config files, kernels, etc,
>> they are all just simple files and instead of making everything take
>> a fd:, we can (and should) just use the existing unix mechanism for
>> this.
>
> OK to me, if that doesn't require code to handle "/dev/fd/" as special
> inside Qemu, but just use what the OS provides. I am just against making
> magic filenames special.
>
> I was still considering adding a key=value interface for -readconfig (or
> an equivalent -config parameter) because of the debugging and probing
> options I would like to add, but those options could be enabled/disabled
> somewhere else, so there wouldn't be a strong reason for it anymore.
>
> On the other hand, I:
>
> - Would like to let the user (a human) list where are the default config
>    files being used by Qemu;

This should be provided via a QMP command and as part of -query-capabilities 
(see previous series).

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Eduardo Habkost]

On Mon, Mar 19, 2012 at 11:45:00AM -0500, Anthony Liguori wrote:
> >On the other hand, I:
> >
> >- Would like to let the user (a human) list where are the default config
> >   files being used by Qemu;
> 
> This should be provided via a QMP command and as part of
> -query-capabilities (see previous series).

Is this usable by a human?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] -readconfig: accept fd=<fd> option (v2)

[Anthony Liguori]

On 03/19/2012 11:51 AM, Eduardo Habkost wrote:
> On Mon, Mar 19, 2012 at 11:45:00AM -0500, Anthony Liguori wrote:
>>> On the other hand, I:
>>>
>>> - Would like to let the user (a human) list where are the default config
>>>    files being used by Qemu;
>>
>> This should be provided via a QMP command and as part of
>> -query-capabilities (see previous series).
>
> Is this usable by a human?

It surely is for the users that would care about this.

Normally, this stuff is printed in either --help or --version FWIW.  But we 
can't touch either because of libvirt (hence my other series).  So if we fix 
that problem, we can provide a nicer --help/--version output that has this 
information.

Regards,

Anthony Liguori

>