Re: [libvirt] [PATCH] qemu-migration: Disallow migration of read only disk

[Daniel P. Berrange]

On Fri, Sep 16, 2016 at 04:03:55PM -0400, Jason J. Herne wrote:
> On 09/14/2016 10:40 AM, Daniel P. Berrange wrote:
> > On Wed, Sep 14, 2016 at 10:37:07AM -0400, Corey S. McQuay wrote:
> > > Currently Libvirt allows attempts to migrate read only disks. Qemu cannot 
> > > handle this as read only
> > > disks cannot be written to on the destination system. The end result is a 
> > > cryptic error message
> > > and a failed migration.
> > > 
> > > This patch causes migration to fail earlier and provides a meaningful 
> > > error message stating that
> > > migrating read only disks is not supported.
> > > 
> > > Signed-off-by: Corey S. McQuay 
> > > Reviewed-by: Jason J. Herne 
> > > Reviewed-by: Boris Fiuczynski 
> > > ---
> > >   src/qemu/qemu_migration.c | 25 +
> > >   1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index e451ef6..3311711 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -2392,6 +2392,28 @@ qemuMigrationIsSafe(virDomainDefPtr def,
> > >   return true;
> > >   }
> > > 
> > > +static bool
> > > +qemuMigrationAreAllDisksRW(virDomainDefPtr def,
> > > +size_t nmigrate_disks,
> > > +const char **migrate_disks)
> > > +{
> > > +size_t i;
> > > +
> > > +for (i = 0; i < def->ndisks; i++) {
> > > +virDomainDiskDefPtr disk = def->disks[i];
> > > +
> > > +if (qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) &&
> > > +disk->src->readonly) {
> > > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > > +   _("Cannot migrate read-only disk %s"),
> > > +   disk->dst);
> > > +return false;
> > > +}
> > > +}
> > > +
> > > +return true;
> > > +}
> > 
> > We already have multiple places in the migration code which iterate
> > over all disks, determining which are migratable. IMHO we should
> > just add an readonly check in one of those, rather than adding yet
> > another iteration.
> > 
> 
> Hi Daniel,
> 
> I'm not seeing a suitable existing location for this new check to live.
> The only place I see migration code loop over the disks for error checking
> is in qemuMigrationBeginPhase.
> 
> for (i = 0; i < nmigrate_disks; i++) {
> for (j = 0; j < vm->def->ndisks; j++) {
> if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> break;
> }
> 
> if (j == vm->def->ndisks) {
> virReportError(VIR_ERR_INVALID_ARG,
>_("disk target %s not found"),
>migrate_disks[i]);
> goto cleanup;
> }
> }
> 
> And putting inside a nested loop would be kind of silly :)
> It seems to be that all other disk loops are in locations
> that do not run before migration begins, or their purpose
> is not for error checking.
> 
> It may make sense to add the check to either of the following:
> qemuMigrationDriveMirror
> qemuMigrationStartNBDServer

Yes, those are exactly what I'd expect it to be.


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-migration: Disallow migration of read only disk

[Jason J. Herne]

On 09/14/2016 10:40 AM, Daniel P. Berrange wrote:

On Wed, Sep 14, 2016 at 10:37:07AM -0400, Corey S. McQuay wrote:

Currently Libvirt allows attempts to migrate read only disks. Qemu cannot 
handle this as read only
disks cannot be written to on the destination system. The end result is a 
cryptic error message
and a failed migration.

This patch causes migration to fail earlier and provides a meaningful error 
message stating that
migrating read only disks is not supported.

Signed-off-by: Corey S. McQuay 
Reviewed-by: Jason J. Herne 
Reviewed-by: Boris Fiuczynski 
---
  src/qemu/qemu_migration.c | 25 +
  1 file changed, 25 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index e451ef6..3311711 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2392,6 +2392,28 @@ qemuMigrationIsSafe(virDomainDefPtr def,
  return true;
  }

+static bool
+qemuMigrationAreAllDisksRW(virDomainDefPtr def,
+size_t nmigrate_disks,
+const char **migrate_disks)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; i++) {
+virDomainDiskDefPtr disk = def->disks[i];
+
+if (qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) &&
+disk->src->readonly) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("Cannot migrate read-only disk %s"),
+   disk->dst);
+return false;
+}
+}
+
+return true;
+}


We already have multiple places in the migration code which iterate
over all disks, determining which are migratable. IMHO we should
just add an readonly check in one of those, rather than adding yet
another iteration.



Hi Daniel,

I'm not seeing a suitable existing location for this new check to live.
The only place I see migration code loop over the disks for error checking
is in qemuMigrationBeginPhase.

for (i = 0; i < nmigrate_disks; i++) {
for (j = 0; j < vm->def->ndisks; j++) {
if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
break;
}

if (j == vm->def->ndisks) {
virReportError(VIR_ERR_INVALID_ARG,
   _("disk target %s not found"),
   migrate_disks[i]);
goto cleanup;
}
}

And putting inside a nested loop would be kind of silly :)
It seems to be that all other disk loops are in locations
that do not run before migration begins, or their purpose
is not for error checking.

It may make sense to add the check to either of the following:
qemuMigrationDriveMirror
qemuMigrationStartNBDServer

But I have not researched those enough yet to know for sure.
I'll look into these on Monday, unless someone can tell me in
the meantime if this makes sense or not?


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-migration: Disallow migration of read only disk

[Daniel P. Berrange]

On Wed, Sep 14, 2016 at 10:37:07AM -0400, Corey S. McQuay wrote:
> Currently Libvirt allows attempts to migrate read only disks. Qemu cannot 
> handle this as read only
> disks cannot be written to on the destination system. The end result is a 
> cryptic error message
> and a failed migration.
> 
> This patch causes migration to fail earlier and provides a meaningful error 
> message stating that
> migrating read only disks is not supported.
> 
> Signed-off-by: Corey S. McQuay 
> Reviewed-by: Jason J. Herne 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_migration.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index e451ef6..3311711 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2392,6 +2392,28 @@ qemuMigrationIsSafe(virDomainDefPtr def,
>  return true;
>  }
>  
> +static bool
> +qemuMigrationAreAllDisksRW(virDomainDefPtr def,
> +size_t nmigrate_disks,
> +const char **migrate_disks)
> +{
> +size_t i;
> +
> +for (i = 0; i < def->ndisks; i++) {
> +virDomainDiskDefPtr disk = def->disks[i];
> +
> +if (qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) &&
> +disk->src->readonly) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("Cannot migrate read-only disk %s"),
> +   disk->dst);
> +return false;
> +}
> +}
> +
> +return true;
> +}

We already have multiple places in the migration code which iterate
over all disks, determining which are migratable. IMHO we should
just add an readonly check in one of those, rather than adding yet
another iteration.


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu-migration: Disallow migration of read only disk

[Corey S. McQuay]

Currently Libvirt allows attempts to migrate read only disks. Qemu cannot 
handle this as read only
disks cannot be written to on the destination system. The end result is a 
cryptic error message
and a failed migration.

This patch causes migration to fail earlier and provides a meaningful error 
message stating that
migrating read only disks is not supported.

Signed-off-by: Corey S. McQuay 
Reviewed-by: Jason J. Herne 
Reviewed-by: Boris Fiuczynski 
---
 src/qemu/qemu_migration.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index e451ef6..3311711 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2392,6 +2392,28 @@ qemuMigrationIsSafe(virDomainDefPtr def,
 return true;
 }
 
+static bool
+qemuMigrationAreAllDisksRW(virDomainDefPtr def,
+size_t nmigrate_disks,
+const char **migrate_disks)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; i++) {
+virDomainDiskDefPtr disk = def->disks[i];
+
+if (qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) &&
+disk->src->readonly) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("Cannot migrate read-only disk %s"),
+   disk->dst);
+return false;
+}
+}
+
+return true;
+}
+
 /** qemuMigrationSetOffline
  * Pause domain for non-live migration.
  */
@@ -3137,6 +3159,9 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
 !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks))
 goto cleanup;
 
+if (!qemuMigrationAreAllDisksRW(vm->def, nmigrate_disks, migrate_disks))
+goto cleanup;
+
 if (flags & VIR_MIGRATE_POSTCOPY &&
 (!(flags & VIR_MIGRATE_LIVE) ||
  flags & VIR_MIGRATE_PAUSED)) {
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list