[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 <csmcquay at linux.vnet.ibm.com>
> > > Reviewed-by: Jason J. Herne <jjherne at linux.vnet.ibm.com>
> > > Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> > > ---
> > >   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 :|