Patchwork [02/18] Introduce read() to FdMigrationState.

login
register
mail settings
Submitter Yoshiaki Tamura
Date Feb. 10, 2011, 9:30 a.m.
Message ID <1297330258-20494-3-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/82580/
State New
Headers show

Comments

Yoshiaki Tamura - Feb. 10, 2011, 9:30 a.m.
Currently FdMigrationState doesn't support read(), and this patch
introduces it to get response from the other side.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 migration-tcp.c |   15 +++++++++++++++
 migration.c     |   13 +++++++++++++
 migration.h     |    3 +++
 3 files changed, 31 insertions(+), 0 deletions(-)
Anthony Liguori - Feb. 10, 2011, 9:54 a.m.
On 02/10/2011 10:30 AM, Yoshiaki Tamura wrote:
> Currently FdMigrationState doesn't support read(), and this patch
> introduces it to get response from the other side.
>
> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>    

Migration is unidirectional.  Changing this is fundamental and not 
something to be done lightly.

I thought we previously discussed using a protocol wrapper around the 
existing migration protocol?

Regards,

Anthony Liguori

> ---
>   migration-tcp.c |   15 +++++++++++++++
>   migration.c     |   13 +++++++++++++
>   migration.h     |    3 +++
>   3 files changed, 31 insertions(+), 0 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index b55f419..55777c8 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -39,6 +39,20 @@ static int socket_write(FdMigrationState *s, const void * buf, size_t size)
>       return send(s->fd, buf, size, 0);
>   }
>
> +static int socket_read(FdMigrationState *s, const void * buf, size_t size)
> +{
> +    ssize_t len;
> +
> +    do {
> +        len = recv(s->fd, (void *)buf, size, 0);
> +    } while (len == -1&&  socket_error() == EINTR);
> +    if (len == -1) {
> +        len = -socket_error();
> +    }
> +
> +    return len;
> +}
> +
>   static int tcp_close(FdMigrationState *s)
>   {
>       DPRINTF("tcp_close\n");
> @@ -94,6 +108,7 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
>
>       s->get_error = socket_errno;
>       s->write = socket_write;
> +    s->read = socket_read;
>       s->close = tcp_close;
>       s->mig_state.cancel = migrate_fd_cancel;
>       s->mig_state.get_status = migrate_fd_get_status;
> diff --git a/migration.c b/migration.c
> index 3612572..f0df5fc 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -340,6 +340,19 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>       return ret;
>   }
>
> +int migrate_fd_get_buffer(void *opaque, uint8_t *data, int64_t pos, size_t size)
> +{
> +    FdMigrationState *s = opaque;
> +    int ret;
> +
> +    ret = s->read(s, data, size);
> +    if (ret == -1) {
> +        ret = -(s->get_error(s));
> +    }
> +
> +    return ret;
> +}
> +
>   void migrate_fd_connect(FdMigrationState *s)
>   {
>       int ret;
> diff --git a/migration.h b/migration.h
> index 2170792..88a6987 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -48,6 +48,7 @@ struct FdMigrationState
>       int (*get_error)(struct FdMigrationState*);
>       int (*close)(struct FdMigrationState*);
>       int (*write)(struct FdMigrationState*, const void *, size_t);
> +    int (*read)(struct FdMigrationState *, const void *, size_t);
>       void *opaque;
>   };
>
> @@ -116,6 +117,8 @@ void migrate_fd_put_notify(void *opaque);
>
>   ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size);
>
> +int migrate_fd_get_buffer(void *opaque, uint8_t *data, int64_t pos, size_t size);
> +
>   void migrate_fd_connect(FdMigrationState *s);
>
>   void migrate_fd_put_ready(void *opaque);
>
Yoshiaki Tamura - Feb. 10, 2011, 10 a.m.
2011/2/10 Anthony Liguori <anthony@codemonkey.ws>:
> On 02/10/2011 10:30 AM, Yoshiaki Tamura wrote:
>>
>> Currently FdMigrationState doesn't support read(), and this patch
>> introduces it to get response from the other side.
>>
>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>>
>
> Migration is unidirectional.  Changing this is fundamental and not something
> to be done lightly.
>
> I thought we previously discussed using a protocol wrapper around the
> existing migration protocol?

AFAIR, I don't think we had that discussion before.  I applied
comments from Stefan though.  If I missed the discussion, could
you please give me the link?

Thanks,

Yoshi

>
> Regards,
>
> Anthony Liguori
>
>> ---
>>  migration-tcp.c |   15 +++++++++++++++
>>  migration.c     |   13 +++++++++++++
>>  migration.h     |    3 +++
>>  3 files changed, 31 insertions(+), 0 deletions(-)
>>
>> diff --git a/migration-tcp.c b/migration-tcp.c
>> index b55f419..55777c8 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -39,6 +39,20 @@ static int socket_write(FdMigrationState *s, const void
>> * buf, size_t size)
>>      return send(s->fd, buf, size, 0);
>>  }
>>
>> +static int socket_read(FdMigrationState *s, const void * buf, size_t
>> size)
>> +{
>> +    ssize_t len;
>> +
>> +    do {
>> +        len = recv(s->fd, (void *)buf, size, 0);
>> +    } while (len == -1&&  socket_error() == EINTR);
>> +    if (len == -1) {
>> +        len = -socket_error();
>> +    }
>> +
>> +    return len;
>> +}
>> +
>>  static int tcp_close(FdMigrationState *s)
>>  {
>>      DPRINTF("tcp_close\n");
>> @@ -94,6 +108,7 @@ MigrationState *tcp_start_outgoing_migration(Monitor
>> *mon,
>>
>>      s->get_error = socket_errno;
>>      s->write = socket_write;
>> +    s->read = socket_read;
>>      s->close = tcp_close;
>>      s->mig_state.cancel = migrate_fd_cancel;
>>      s->mig_state.get_status = migrate_fd_get_status;
>> diff --git a/migration.c b/migration.c
>> index 3612572..f0df5fc 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -340,6 +340,19 @@ ssize_t migrate_fd_put_buffer(void *opaque, const
>> void *data, size_t size)
>>      return ret;
>>  }
>>
>> +int migrate_fd_get_buffer(void *opaque, uint8_t *data, int64_t pos,
>> size_t size)
>> +{
>> +    FdMigrationState *s = opaque;
>> +    int ret;
>> +
>> +    ret = s->read(s, data, size);
>> +    if (ret == -1) {
>> +        ret = -(s->get_error(s));
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  void migrate_fd_connect(FdMigrationState *s)
>>  {
>>      int ret;
>> diff --git a/migration.h b/migration.h
>> index 2170792..88a6987 100644
>> --- a/migration.h
>> +++ b/migration.h
>> @@ -48,6 +48,7 @@ struct FdMigrationState
>>      int (*get_error)(struct FdMigrationState*);
>>      int (*close)(struct FdMigrationState*);
>>      int (*write)(struct FdMigrationState*, const void *, size_t);
>> +    int (*read)(struct FdMigrationState *, const void *, size_t);
>>      void *opaque;
>>  };
>>
>> @@ -116,6 +117,8 @@ void migrate_fd_put_notify(void *opaque);
>>
>>  ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t
>> size);
>>
>> +int migrate_fd_get_buffer(void *opaque, uint8_t *data, int64_t pos,
>> size_t size);
>> +
>>  void migrate_fd_connect(FdMigrationState *s);
>>
>>  void migrate_fd_put_ready(void *opaque);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Daniel P. Berrange - Feb. 10, 2011, 10:18 a.m.
On Thu, Feb 10, 2011 at 10:54:01AM +0100, Anthony Liguori wrote:
> On 02/10/2011 10:30 AM, Yoshiaki Tamura wrote:
> >Currently FdMigrationState doesn't support read(), and this patch
> >introduces it to get response from the other side.
> >
> >Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
> 
> Migration is unidirectional.  Changing this is fundamental and not
> something to be done lightly.

Making it bi-directional might break libvirt's save/restore
to file support which uses migration, passing a unidirectional
FD for the file. It could also break libvirt's secure tunnelled
migration support which is currently only expecting to have
data sent in one direction on the socket.

Daniel
Yoshiaki Tamura - Feb. 10, 2011, 10:23 a.m.
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
> On Thu, Feb 10, 2011 at 10:54:01AM +0100, Anthony Liguori wrote:
>> On 02/10/2011 10:30 AM, Yoshiaki Tamura wrote:
>> >Currently FdMigrationState doesn't support read(), and this patch
>> >introduces it to get response from the other side.
>> >
>> >Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>>
>> Migration is unidirectional.  Changing this is fundamental and not
>> something to be done lightly.
>
> Making it bi-directional might break libvirt's save/restore
> to file support which uses migration, passing a unidirectional
> FD for the file. It could also break libvirt's secure tunnelled
> migration support which is currently only expecting to have
> data sent in one direction on the socket.

Hi Daniel,

IIUC, this patch isn't something to make existing live migration
bi-directional.  Just opens up a way for Kemari to use it.  Do
you think it's dangerous for libvirt still?

Thanks,

Yoshi

>
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Daniel P. Berrange - Feb. 10, 2011, 10:44 a.m.
On Thu, Feb 10, 2011 at 07:23:33PM +0900, Yoshiaki Tamura wrote:
> 2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
> > On Thu, Feb 10, 2011 at 10:54:01AM +0100, Anthony Liguori wrote:
> >> On 02/10/2011 10:30 AM, Yoshiaki Tamura wrote:
> >> >Currently FdMigrationState doesn't support read(), and this patch
> >> >introduces it to get response from the other side.
> >> >
> >> >Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
> >>
> >> Migration is unidirectional.  Changing this is fundamental and not
> >> something to be done lightly.
> >
> > Making it bi-directional might break libvirt's save/restore
> > to file support which uses migration, passing a unidirectional
> > FD for the file. It could also break libvirt's secure tunnelled
> > migration support which is currently only expecting to have
> > data sent in one direction on the socket.
> 
> Hi Daniel,
> 
> IIUC, this patch isn't something to make existing live migration
> bi-directional.  Just opens up a way for Kemari to use it.  Do
> you think it's dangerous for libvirt still?

The key is for it to be a no-op for any usage of the existing
'migrate' command. I had thought this was wiring up read into
the event loop too, so it would be poll()ing for reads, but
after re-reading I see this isn't the case here.

Regards,
Daniel
Yoshiaki Tamura - Feb. 10, 2011, 10:51 a.m.
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
> On Thu, Feb 10, 2011 at 07:23:33PM +0900, Yoshiaki Tamura wrote:
>> 2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
>> > On Thu, Feb 10, 2011 at 10:54:01AM +0100, Anthony Liguori wrote:
>> >> On 02/10/2011 10:30 AM, Yoshiaki Tamura wrote:
>> >> >Currently FdMigrationState doesn't support read(), and this patch
>> >> >introduces it to get response from the other side.
>> >> >
>> >> >Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>> >>
>> >> Migration is unidirectional.  Changing this is fundamental and not
>> >> something to be done lightly.
>> >
>> > Making it bi-directional might break libvirt's save/restore
>> > to file support which uses migration, passing a unidirectional
>> > FD for the file. It could also break libvirt's secure tunnelled
>> > migration support which is currently only expecting to have
>> > data sent in one direction on the socket.
>>
>> Hi Daniel,
>>
>> IIUC, this patch isn't something to make existing live migration
>> bi-directional.  Just opens up a way for Kemari to use it.  Do
>> you think it's dangerous for libvirt still?
>
> The key is for it to be a no-op for any usage of the existing
> 'migrate' command. I had thought this was wiring up read into
> the event loop too, so it would be poll()ing for reads, but
> after re-reading I see this isn't the case here.

It's a no-op for existing migration related code.  Anthony, did
you have the same concern?

Yoshi

>
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch

diff --git a/migration-tcp.c b/migration-tcp.c
index b55f419..55777c8 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -39,6 +39,20 @@  static int socket_write(FdMigrationState *s, const void * buf, size_t size)
     return send(s->fd, buf, size, 0);
 }
 
+static int socket_read(FdMigrationState *s, const void * buf, size_t size)
+{
+    ssize_t len;
+
+    do {
+        len = recv(s->fd, (void *)buf, size, 0);
+    } while (len == -1 && socket_error() == EINTR);
+    if (len == -1) {
+        len = -socket_error();
+    }
+
+    return len;
+}
+
 static int tcp_close(FdMigrationState *s)
 {
     DPRINTF("tcp_close\n");
@@ -94,6 +108,7 @@  MigrationState *tcp_start_outgoing_migration(Monitor *mon,
 
     s->get_error = socket_errno;
     s->write = socket_write;
+    s->read = socket_read;
     s->close = tcp_close;
     s->mig_state.cancel = migrate_fd_cancel;
     s->mig_state.get_status = migrate_fd_get_status;
diff --git a/migration.c b/migration.c
index 3612572..f0df5fc 100644
--- a/migration.c
+++ b/migration.c
@@ -340,6 +340,19 @@  ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
     return ret;
 }
 
+int migrate_fd_get_buffer(void *opaque, uint8_t *data, int64_t pos, size_t size)
+{
+    FdMigrationState *s = opaque;
+    int ret;
+
+    ret = s->read(s, data, size);
+    if (ret == -1) {
+        ret = -(s->get_error(s));
+    }
+
+    return ret;
+}
+
 void migrate_fd_connect(FdMigrationState *s)
 {
     int ret;
diff --git a/migration.h b/migration.h
index 2170792..88a6987 100644
--- a/migration.h
+++ b/migration.h
@@ -48,6 +48,7 @@  struct FdMigrationState
     int (*get_error)(struct FdMigrationState*);
     int (*close)(struct FdMigrationState*);
     int (*write)(struct FdMigrationState*, const void *, size_t);
+    int (*read)(struct FdMigrationState *, const void *, size_t);
     void *opaque;
 };
 
@@ -116,6 +117,8 @@  void migrate_fd_put_notify(void *opaque);
 
 ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size);
 
+int migrate_fd_get_buffer(void *opaque, uint8_t *data, int64_t pos, size_t size);
+
 void migrate_fd_connect(FdMigrationState *s);
 
 void migrate_fd_put_ready(void *opaque);