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 <csmcq...@linux.vnet.ibm.com>
> > > Reviewed-by: Jason J. Herne <jjhe...@linux.vnet.ibm.com>
> > > Reviewed-by: Boris Fiuczynski <fiu...@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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
