Patchwork [32/41] qemu-file: add writable socket QEMUFile

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 15, 2013, 5:47 p.m.
Message ID <1360950433-17106-33-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/220808/
State New
Headers show

Comments

Paolo Bonzini - Feb. 15, 2013, 5:47 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/migration/qemu-file.h |    2 +-
 migration-tcp.c               |    2 +-
 migration-unix.c              |    2 +-
 savevm.c                      |   33 +++++++++++++++++++++++++++++++--
 util/osdep.c                  |    6 +++++-
 5 files changed, 39 insertions(+), 6 deletions(-)
Orit Wasserman - Feb. 21, 2013, 8:09 a.m.
On 02/15/2013 07:47 PM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/migration/qemu-file.h |    2 +-
>  migration-tcp.c               |    2 +-
>  migration-unix.c              |    2 +-
>  savevm.c                      |   33 +++++++++++++++++++++++++++++++--
>  util/osdep.c                  |    6 +++++-
>  5 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 987e719..25e8461 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -76,7 +76,7 @@ typedef struct QEMUFileOps {
>  QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
>  QEMUFile *qemu_fopen(const char *filename, const char *mode);
>  QEMUFile *qemu_fdopen(int fd, const char *mode);
> -QEMUFile *qemu_fopen_socket(int fd);
> +QEMUFile *qemu_fopen_socket(int fd, const char *mode);
>  QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
>  int qemu_get_fd(QEMUFile *f);
>  int qemu_fclose(QEMUFile *f);
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e78a296..7d975b5 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -95,7 +95,7 @@ static void tcp_accept_incoming_migration(void *opaque)
>          goto out;
>      }
>  
> -    f = qemu_fopen_socket(c);
> +    f = qemu_fopen_socket(c, "rb");
>      if (f == NULL) {
>          fprintf(stderr, "could not qemu_fopen socket\n");
>          goto out;
> diff --git a/migration-unix.c b/migration-unix.c
> index 218835a..4693b43 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -95,7 +95,7 @@ static void unix_accept_incoming_migration(void *opaque)
>          goto out;
>      }
>  
> -    f = qemu_fopen_socket(c);
> +    f = qemu_fopen_socket(c, "rb");
>      if (f == NULL) {
>          fprintf(stderr, "could not qemu_fopen socket\n");
>          goto out;
> diff --git a/savevm.c b/savevm.c
> index f593acd..b6cf415 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -198,6 +198,18 @@ static int socket_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>      return len;
>  }
>  
> +static int socket_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
> +{
> +    QEMUFileSocket *s = opaque;
> +    ssize_t len;
> +
> +    len = qemu_send_full(s->fd, buf, size, 0);
> +    if (len == -1) {
> +        len = -socket_error();
> +    }

We won't always detect partial buffer write here
Why not check len < size? this way you won't need to change qemu_send_full
> +    return len;
> +}
> +
>  static int socket_close(void *opaque)
>  {
>      QEMUFileSocket *s = opaque;
> @@ -369,12 +381,29 @@ static const QEMUFileOps socket_read_ops = {
>      .close =      socket_close
>  };
>  
> -QEMUFile *qemu_fopen_socket(int fd)
> +static const QEMUFileOps socket_write_ops = {
> +    .get_fd =     socket_get_fd,
> +    .put_buffer = socket_put_buffer,
> +    .close =      socket_close
> +};
> +
> +QEMUFile *qemu_fopen_socket(int fd, const char *mode)
>  {
>      QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
>  
> +    if (mode == NULL ||
> +	(mode[0] != 'r' && mode[0] != 'w') ||
> +	mode[1] != 'b' || mode[2] != 0) {
> +        fprintf(stderr, "qemu_fopen: Argument validity check failed\n");
> +        return NULL;
> +    }
> +
>      s->fd = fd;
> -    s->file = qemu_fopen_ops(s, &socket_read_ops);
> +    if (mode[0] == 'w') {
> +        s->file = qemu_fopen_ops(s, &socket_write_ops);
> +    } else {
> +        s->file = qemu_fopen_ops(s, &socket_read_ops);
> +    }
>      return s->file;
>  }
>  
> diff --git a/util/osdep.c b/util/osdep.c
> index 5b51a03..be168e5 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -243,8 +243,12 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
>      while (count) {
>          ret = write(fd, buf, count);
>          if (ret < 0) {
> -            if (errno == EINTR)
> +            if (errno == EINTR) {
>                  continue;
> +            }
> +            if (total == 0) {
> +                total = ret;
> +            }

I don't like it, first it only detect the first write failure (if the error happens when total !=0
the function won't return -1).
Secondly this behavior is different from the function documentation.

Regards,
Orit
>              break;
>          }
>  
>
Paolo Bonzini - Feb. 21, 2013, 12:09 p.m.
> > +static int socket_put_buffer(void *opaque, const uint8_t *buf,
> > int64_t pos, int size)
> > +{
> > +    QEMUFileSocket *s = opaque;
> > +    ssize_t len;
> > +
> > +    len = qemu_send_full(s->fd, buf, size, 0);
> > +    if (len == -1) {
> > +        len = -socket_error();
> > +    }
> 
> We won't always detect partial buffer write here
> Why not check len < size? this way you won't need to change
> qemu_send_full

Ok.

Paolo

> > +    return len;
> > +}
> > +
> >  static int socket_close(void *opaque)
> >  {
> >      QEMUFileSocket *s = opaque;
> > @@ -369,12 +381,29 @@ static const QEMUFileOps socket_read_ops = {
> >      .close =      socket_close
> >  };
> >  
> > -QEMUFile *qemu_fopen_socket(int fd)
> > +static const QEMUFileOps socket_write_ops = {
> > +    .get_fd =     socket_get_fd,
> > +    .put_buffer = socket_put_buffer,
> > +    .close =      socket_close
> > +};
> > +
> > +QEMUFile *qemu_fopen_socket(int fd, const char *mode)
> >  {
> >      QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
> >  
> > +    if (mode == NULL ||
> > +	(mode[0] != 'r' && mode[0] != 'w') ||
> > +	mode[1] != 'b' || mode[2] != 0) {
> > +        fprintf(stderr, "qemu_fopen: Argument validity check
> > failed\n");
> > +        return NULL;
> > +    }
> > +
> >      s->fd = fd;
> > -    s->file = qemu_fopen_ops(s, &socket_read_ops);
> > +    if (mode[0] == 'w') {
> > +        s->file = qemu_fopen_ops(s, &socket_write_ops);
> > +    } else {
> > +        s->file = qemu_fopen_ops(s, &socket_read_ops);
> > +    }
> >      return s->file;
> >  }
> >  
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 5b51a03..be168e5 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -243,8 +243,12 @@ ssize_t qemu_write_full(int fd, const void
> > *buf, size_t count)
> >      while (count) {
> >          ret = write(fd, buf, count);
> >          if (ret < 0) {
> > -            if (errno == EINTR)
> > +            if (errno == EINTR) {
> >                  continue;
> > +            }
> > +            if (total == 0) {
> > +                total = ret;
> > +            }
> 
> I don't like it, first it only detect the first write failure (if the
> error happens when total !=0
> the function won't return -1).
> Secondly this behavior is different from the function documentation.
> 
> Regards,
> Orit
> >              break;
> >          }
> >  
> > 
> 
>

Patch

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 987e719..25e8461 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -76,7 +76,7 @@  typedef struct QEMUFileOps {
 QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
 QEMUFile *qemu_fopen(const char *filename, const char *mode);
 QEMUFile *qemu_fdopen(int fd, const char *mode);
-QEMUFile *qemu_fopen_socket(int fd);
+QEMUFile *qemu_fopen_socket(int fd, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
 int qemu_get_fd(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
diff --git a/migration-tcp.c b/migration-tcp.c
index e78a296..7d975b5 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -95,7 +95,7 @@  static void tcp_accept_incoming_migration(void *opaque)
         goto out;
     }
 
-    f = qemu_fopen_socket(c);
+    f = qemu_fopen_socket(c, "rb");
     if (f == NULL) {
         fprintf(stderr, "could not qemu_fopen socket\n");
         goto out;
diff --git a/migration-unix.c b/migration-unix.c
index 218835a..4693b43 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -95,7 +95,7 @@  static void unix_accept_incoming_migration(void *opaque)
         goto out;
     }
 
-    f = qemu_fopen_socket(c);
+    f = qemu_fopen_socket(c, "rb");
     if (f == NULL) {
         fprintf(stderr, "could not qemu_fopen socket\n");
         goto out;
diff --git a/savevm.c b/savevm.c
index f593acd..b6cf415 100644
--- a/savevm.c
+++ b/savevm.c
@@ -198,6 +198,18 @@  static int socket_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
     return len;
 }
 
+static int socket_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
+{
+    QEMUFileSocket *s = opaque;
+    ssize_t len;
+
+    len = qemu_send_full(s->fd, buf, size, 0);
+    if (len == -1) {
+        len = -socket_error();
+    }
+    return len;
+}
+
 static int socket_close(void *opaque)
 {
     QEMUFileSocket *s = opaque;
@@ -369,12 +381,29 @@  static const QEMUFileOps socket_read_ops = {
     .close =      socket_close
 };
 
-QEMUFile *qemu_fopen_socket(int fd)
+static const QEMUFileOps socket_write_ops = {
+    .get_fd =     socket_get_fd,
+    .put_buffer = socket_put_buffer,
+    .close =      socket_close
+};
+
+QEMUFile *qemu_fopen_socket(int fd, const char *mode)
 {
     QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
 
+    if (mode == NULL ||
+	(mode[0] != 'r' && mode[0] != 'w') ||
+	mode[1] != 'b' || mode[2] != 0) {
+        fprintf(stderr, "qemu_fopen: Argument validity check failed\n");
+        return NULL;
+    }
+
     s->fd = fd;
-    s->file = qemu_fopen_ops(s, &socket_read_ops);
+    if (mode[0] == 'w') {
+        s->file = qemu_fopen_ops(s, &socket_write_ops);
+    } else {
+        s->file = qemu_fopen_ops(s, &socket_read_ops);
+    }
     return s->file;
 }
 
diff --git a/util/osdep.c b/util/osdep.c
index 5b51a03..be168e5 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -243,8 +243,12 @@  ssize_t qemu_write_full(int fd, const void *buf, size_t count)
     while (count) {
         ret = write(fd, buf, count);
         if (ret < 0) {
-            if (errno == EINTR)
+            if (errno == EINTR) {
                 continue;
+            }
+            if (total == 0) {
+                total = ret;
+            }
             break;
         }