Patchwork [06/11] chardev: add file chardev support to chardev-add (qmp)

login
register
mail settings
Submitter Gerd Hoffmann
Date Jan. 7, 2013, 1:55 p.m.
Message ID <1357566928-25361-7-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/209928/
State New
Headers show

Comments

Gerd Hoffmann - Jan. 7, 2013, 1:55 p.m.
Add support for file chardevs.  Output file is mandatory,
input file is optional.  Both file names and file descriptor
passing is supported.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    9 +++++-
 qemu-char.c      |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 2 deletions(-)
Eric Blake - Jan. 9, 2013, 6:12 p.m.
On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
> Add support for file chardevs.  Output file is mandatory,
> input file is optional.  Both file names and file descriptor
> passing is supported.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    9 +++++-
>  qemu-char.c      |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e3f0d44..8904d36 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3030,9 +3030,16 @@
>  #
>  # Since: 1.4
>  ##
> +{ 'union': 'ChardevFileSource', 'data': { 'path' : 'str',
> +                                          'fd'   : 'str' } }
> +
> +{ 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
> +                                   'out' : 'ChardevFileSource' } }
> +
>  { 'type': 'ChardevDummy', 'data': { } }
>  
> -{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' } }
> +{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
> +                                       'null' : 'ChardevDummy' } }
>  
>  { 'command': 'chardev-add', 'data': {'id'      : 'str',
>                                       'backend' : 'ChardevBackend' } }

An example in qmp-commands.hx would be helpful; am I correct that an
example would be:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "file", "data" : {
         "in"  : { "type" : "fd", "data" : "namedfd" },
         "out" : { "type" : "path", "data" : "/path/to/file" } } } } }
<- { "return": {} }

where namedfd was previously given via 'getfd'?

Do we need the complexity of supporting fd passing explicitly?  'getfd'
is less than ideal compared to 'add-fd', and for 'add-fd', we would pass
via "path":"/dev/fdset/nnn".  That is, why do we need to bend over
backwards to support an alternate syntax for fd passing in a new
command, when we can already use existing commands to get fd passing for
free?
Gerd Hoffmann - Jan. 10, 2013, 9:10 a.m.
On 01/09/13 19:12, Eric Blake wrote:
> On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
>> Add support for file chardevs.  Output file is mandatory, input
>> file is optional.  Both file names and file descriptor passing is
>> supported.
>> 
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- 
>> qapi-schema.json |    9 +++++- qemu-char.c      |   77
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files
>> changed, 84 insertions(+), 2 deletions(-)
>> 
>> diff --git a/qapi-schema.json b/qapi-schema.json index
>> e3f0d44..8904d36 100644 --- a/qapi-schema.json +++
>> b/qapi-schema.json @@ -3030,9 +3030,16 @@ # # Since: 1.4 ## +{
>> 'union': 'ChardevFileSource', 'data': { 'path' : 'str', +
>> 'fd'   : 'str' } } + +{ 'type': 'ChardevFile', 'data': { '*in' :
>> 'ChardevFileSource', +                                   'out' :
>> 'ChardevFileSource' } } + { 'type': 'ChardevDummy', 'data': { }
>> }
>> 
>> -{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' }
>> } +{ 'union': 'ChardevBackend', 'data': { 'file' :
>> 'ChardevFile', +                                       'null' :
>> 'ChardevDummy' } }
>> 
>> { 'command': 'chardev-add', 'data': {'id'      : 'str', 'backend'
>> : 'ChardevBackend' } }
> 
> An example in qmp-commands.hx would be helpful; am I correct that
> an example would be:
> 
> -> { "execute" : "chardev-add", "arguments" : { "id" : "foo", 
> "backend" : { "type" : "file", "data" : { "in"  : { "type" : "fd",
> "data" : "namedfd" }, "out" : { "type" : "path", "data" :
> "/path/to/file" } } } } } <- { "return": {} }
> 
> where namedfd was previously given via 'getfd'?

Yes.

> Do we need the complexity of supporting fd passing explicitly?
> 'getfd' is less than ideal compared to 'add-fd', and for 'add-fd',
> we would pass via "path":"/dev/fdset/nnn".  That is, why do we need
> to bend over backwards to support an alternate syntax for fd
> passing in a new command, when we can already use existing commands
> to get fd passing for free?

Oh, didn't know that.  Was just following what SocketAddress does and
what Paolo suggested.  It isn't needed indeed.

cheers,
  Gerd
Paolo Bonzini - Jan. 10, 2013, 10:35 a.m.
Il 10/01/2013 10:10, Gerd Hoffmann ha scritto:
> 
>> > Do we need the complexity of supporting fd passing explicitly?
>> > 'getfd' is less than ideal compared to 'add-fd', and for 'add-fd',
>> > we would pass via "path":"/dev/fdset/nnn".  That is, why do we need
>> > to bend over backwards to support an alternate syntax for fd
>> > passing in a new command, when we can already use existing commands
>> > to get fd passing for free?
> Oh, didn't know that.  Was just following what SocketAddress does and
> what Paolo suggested.  It isn't needed indeed.

Yeah, I hadn't followed fdset either.  We really need more documentation.

Paolo

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index e3f0d44..8904d36 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3030,9 +3030,16 @@ 
 #
 # Since: 1.4
 ##
+{ 'union': 'ChardevFileSource', 'data': { 'path' : 'str',
+                                          'fd'   : 'str' } }
+
+{ 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
+                                   'out' : 'ChardevFileSource' } }
+
 { 'type': 'ChardevDummy', 'data': { } }
 
-{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' } }
+{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
+                                       'null' : 'ChardevDummy' } }
 
 { 'command': 'chardev-add', 'data': {'id'      : 'str',
                                      'backend' : 'ChardevBackend' } }
diff --git a/qemu-char.c b/qemu-char.c
index c9ae8f3..91b0a57 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2938,11 +2938,86 @@  CharDriverState *qemu_char_get_next_serial(void)
     return serial_hds[next_serial++];
 }
 
+#ifdef _WIN32
+
+static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
+{
+    HANDLE out;
+
+    if (file->in) {
+        error_setg(errp, "input file not supported");
+        return NULL;
+    }
+    if (file->out->kind != CHARDEV_FILE_SOURCE_KIND_PATH) {
+        error_setg(errp, "only file paths supported");
+        return NULL;
+    }
+
+    out = CreateFile(file->out->path, GENERIC_WRITE, FILE_SHARE_READ, NULL,
+                     OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+    if (out == INVALID_HANDLE_VALUE) {
+        error_setg(errp, "open %s failed", file->out->path);
+        return NULL;
+    }
+    return qemu_chr_open_win_file(out);
+}
+
+#else /* WIN32 */
+
+static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
+                                        Error **errp)
+{
+    int fd = -1;
+
+    switch (src->kind) {
+    case CHARDEV_FILE_SOURCE_KIND_PATH:
+        TFR(fd = qemu_open(src->path, flags, 0666));
+        if (fd == -1) {
+            error_setg(errp, "open %s: %s", src->path, strerror(errno));
+        }
+        break;
+    case CHARDEV_FILE_SOURCE_KIND_FD:
+        fd = monitor_get_fd(cur_mon, src->fd, errp);
+        break;
+    default:
+        error_setg(errp, "unknown chardev file source (%d)", src->kind);
+        break;
+    }
+    return fd;
+}
+
+static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
+{
+    int flags, in = -1, out = -1;
+
+    flags = O_WRONLY | O_TRUNC | O_CREAT | O_BINARY;
+    out = qmp_chardev_open_file_source(file->out, flags, errp);
+    if (error_is_set(errp)) {
+        return NULL;
+    }
+
+    if (file->in) {
+        flags = O_RDONLY;
+        in = qmp_chardev_open_file_source(file->in, flags, errp);
+        if (error_is_set(errp)) {
+            qemu_close(out);
+            return NULL;
+        }
+    }
+
+    return qemu_chr_open_fd(in, out);
+}
+
+#endif /* WIN32 */
+
 void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
 {
     CharDriverState *chr;
 
     switch (backend->kind) {
+    case CHARDEV_BACKEND_KIND_FILE:
+        chr = qmp_chardev_open_file(backend->file, errp);
+        break;
     case CHARDEV_BACKEND_KIND_NULL:
         chr = qemu_chr_open_null(NULL);
         break;
@@ -2951,7 +3026,7 @@  void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
         return;
     }
 
-    if (chr == NULL) {
+    if (chr == NULL && !error_is_set(errp)) {
         error_setg(errp, "Failed to create chardev");
     }
     if (chr) {