On 11/18/2015 01:13 PM, Pavel Boldin wrote:
> Add qemuNBDTunnelAcceptAndPipe function that is called to handle POLLIN
> on the UNIX socket connection from the QEMU's NBD server.
>
> The function creates a pipe of a remote stream connected to the QEMU
> NBD Unix socket on destination and a local stream connected to
> the incoming connection from the source QEMU's NBD.
>
> Signed-off-by: Pavel Boldin <pboldin@xxxxxxxxxxxx>
> ---
> src/qemu/qemu_migration.c | 134 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 132 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 0682fd8..0f35c13 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3987,6 +3987,9 @@ struct _qemuMigrationSpec {
>
> #define TUNNEL_SEND_BUF_SIZE 65536
>
> +typedef struct _qemuMigrationPipe qemuMigrationPipe;
> +typedef qemuMigrationPipe *qemuMigrationPipePtr;
> +
> typedef struct _qemuMigrationIOThread qemuMigrationIOThread;
> typedef qemuMigrationIOThread *qemuMigrationIOThreadPtr;
> struct _qemuMigrationIOThread {
> @@ -3997,9 +4000,124 @@ struct _qemuMigrationIOThread {
> virError err;
> int wakeupRecvFD;
> int wakeupSendFD;
> + qemuMigrationPipePtr pipes;
> + virConnectPtr dconn;
> + unsigned char uuid[VIR_UUID_BUFLEN];
> +};
> +
> +struct _qemuMigrationPipe {
> + qemuMigrationPipePtr next;
> + qemuMigrationIOThreadPtr data;
> + virStreamPtr local;
> + virStreamPtr remote;
> };
>
> static void
> +qemuMigrationPipeClose(qemuMigrationPipePtr pipe, bool abort)
> +{
> + virStreamEventUpdateCallback(pipe->local, 0);
> + virStreamEventUpdateCallback(pipe->remote, 0);
> +
> + if (abort) {
> + virStreamAbort(pipe->local);
> + virStreamAbort(pipe->remote);
> + } else {
> + virStreamFinish(pipe->local);
> + virStreamFinish(pipe->remote);
> + }
> +
> + virObjectUnref(pipe->local);
> + virObjectUnref(pipe->remote);
> +}
> +
Norm seems to be 2 blank lines between functions. There's only one here.
> +static qemuMigrationPipePtr
> +qemuMigrationPipeCreate(virStreamPtr local, virStreamPtr remote)
> +{
> + qemuMigrationPipePtr pipe = NULL;
> +
> + if (VIR_ALLOC(pipe) < 0)
> + goto error;
> +
> + pipe->local = local;
> + pipe->remote = remote;
> +
> + return pipe;
> +
> + error:
> + virStreamEventRemoveCallback(local);
> + virStreamEventRemoveCallback(remote);
> + VIR_FREE(pipe);
> + return NULL;
> +}
> +
but two here...
> +
> +static int
> +qemuNBDTunnelAcceptAndPipe(qemuMigrationIOThreadPtr data)
> +{
> + int fd, ret;
> + virStreamPtr local = NULL, remote = NULL;
> + qemuMigrationPipePtr pipe = NULL;
> +
> + while ((fd = accept(data->unixSock, NULL, NULL)) < 0) {
> + if (errno == EAGAIN || errno == EINTR)
> + continue;
> + virReportSystemError(
> + errno, "%s", _("failed to accept connection from qemu"));
> + goto abrt;
> + }
> +
> + if (!(local = virStreamNew(data->dconn, VIR_STREAM_NONBLOCK)))
> + goto abrt;
> +
> + if (!(remote = virStreamNew(data->dconn, VIR_STREAM_NONBLOCK)))
> + goto abrt;
> +
> + ret = virDomainMigrateOpenTunnel(data->dconn,
> + remote,
> + data->uuid,
> + VIR_MIGRATE_TUNNEL_NBD);
> +
> + if (ret < 0)
> + goto abrt;
> +
> + if (virFDStreamOpen(local, fd) < 0)
> + goto abrt;
> +
> + if (!(pipe = qemuMigrationPipeCreate(local, remote)))
> + goto abrt;
> +
> + pipe->data = data;
> + pipe->next = data->pipes;
> + data->pipes = pipe;
Didn't think too long about this insertion code, but how many times can
this happen? It's the removal code which removes them all at one time
that's just has me wondering.
> +
> + return 0;
> +
> + abrt:
> + VIR_FORCE_CLOSE(fd);
> + virStreamAbort(local);
> + virStreamAbort(remote);
> +
> + virObjectUnref(local);
> + virObjectUnref(remote);
> + return -1;
> +}
> +
back to just one blank line.
> +static void
> +qemuMigrationPipesStop(qemuMigrationPipePtr pipe, bool abort)
> +{
> + qemuMigrationPipePtr tmp;
> +
> + while (pipe) {
> + tmp = pipe->next;
> +
> + qemuMigrationPipeClose(pipe, abort);
> + VIR_FREE(pipe);
> +
> + pipe = tmp;
> + }
> +}
> +
And again only 1 line...
> +static void
> qemuMigrationIOFunc(void *arg)
> {
> qemuMigrationIOThreadPtr data = arg;
> @@ -4081,9 +4199,14 @@ qemuMigrationIOFunc(void *arg)
> break;
> }
> }
> +
> + if (fds[2].revents & (POLLIN | POLLERR | POLLHUP) &&
> + qemuNBDTunnelAcceptAndPipe(data) < 0)
> + goto abrt;
This would only seem to be necessary if we have a dconn && uuid set...
> }
>
> virStreamFinish(data->qemuStream);
> + qemuMigrationPipesStop(data->pipes, false);
>
> VIR_FORCE_CLOSE(data->qemuSock);
> VIR_FREE(buffer);
> @@ -4097,6 +4220,7 @@ qemuMigrationIOFunc(void *arg)
> err = NULL;
> }
> virStreamAbort(data->qemuStream);
> + qemuMigrationPipesStop(data->pipes, true);
> if (err) {
> virSetError(err);
> virFreeError(err);
Looks like we can get to error: from within the polling loop, but we
don't use qemuMigrationPipesStop there
> @@ -4114,7 +4238,9 @@ qemuMigrationIOFunc(void *arg)
>
>
> static qemuMigrationIOThreadPtr
> -qemuMigrationStartTunnel(virStreamPtr qemuStream)
> +qemuMigrationStartTunnel(virStreamPtr qemuStream,
> + virConnectPtr dconn,
> + unsigned char uuid[VIR_UUID_BUFLEN])
Again the "const unsigned char *uuid"
So what happens when we don't have nbd disks to migrate? Do we really
need to have dconn and uuid? Seems that is the assumption...
> {
> qemuMigrationIOThreadPtr io = NULL;
> int wakeupFD[2] = { -1, -1 };
> @@ -4132,6 +4258,8 @@ qemuMigrationStartTunnel(virStreamPtr qemuStream)
> io->qemuSock = io->unixSock = -1;
> io->wakeupRecvFD = wakeupFD[0];
> io->wakeupSendFD = wakeupFD[1];
> + io->dconn = dconn;
> + memcpy(io->uuid, uuid, VIR_UUID_BUFLEN);
>
> if (virThreadCreate(&io->thread, true,
> qemuMigrationIOFunc,
> @@ -4337,7 +4465,9 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> VIR_WARN("unable to provide data for graphics client relocation");
>
> if (spec->fwdType != MIGRATION_FWD_DIRECT) {
> - if (!(iothread = qemuMigrationStartTunnel(spec->fwd.stream)))
> + if (!(iothread = qemuMigrationStartTunnel(spec->fwd.stream,
> + dconn,
> + mig->uuid)))
So this will only matter if we have mig->nbd, migrate_flags &
(*DISK|*INC), true? An assumption that's not always true I would think.
And thus the 'need' to start things up with that enabled is unnecessary.
Seems you'd want to keep qemuMigrationStartTunnel as is, then if/when we
known we're going to have this functionality have a "set" function for
dconn && uuid... prior to of course setting the nbd_tunnel* in iothread.
John
FWIW: I'm going to stop here. I've got a backlog of other things to look
at right now and it seems the next one makes things even a bit more
complicated... Plus as I said in my .0 response - starting at patch 16
I wasn't able to apply the changes.
> goto cancel;
>
> if (nmigrate_disks &&
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list