Re: [libvirt] [PATCH 3/4] parallels: add disks correctly

On Thu, May 08, 2014 at 11:37:09AM +0100, Daniel P. Berrange wrote: > On Wed, May 07, 2014 at 10:04:08PM +0400, Dmitry Guryanov wrote: > > Disks support in this driver was implemented with an assumption, > > that disk images can't be created by hand, without VM. So > > complex storage driver was implemented with workaround. > > > > This is not true, we can create new disks using ploop tool. > > So the first step to reimplement disks support in parallels > > driver is to do not use information from the storage driver, > > until we will implement VIR_STORAGE_TYPE_VOLUME disks. > > > > So after this patch disks can be added in the same way as > > in any other driver: you create a disk image and then add > > an entry to the XML definition of the domain with path to that > > image file, for example: > > > > <disk type='file' device='disk'> > > > > <driver type='ploop'/> > > <source file='/storage/harddisk1.hdd'/> > > <target dev='sda' bus='sata'/> > > <address type='drive' controller='0' bus='0' target='0' unit='0'/> > > > > </disk> > > > > This patch makes parallels storage driver useless, but I'll fix it > > later. Now you can create an image by hand, using ploop tool, > > and then add it to some domain. > > > > Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com&gt; > > --- > > > > src/parallels/parallels_driver.c | 83 > > ++++++++++++++-------------------------- 1 file changed, 28 > > insertions(+), 55 deletions(-) > > > > diff --git a/src/parallels/parallels_driver.c > > b/src/parallels/parallels_driver.c index b2de12f..67b28c4 100644 > > --- a/src/parallels/parallels_driver.c > > +++ b/src/parallels/parallels_driver.c > > @@ -1605,17 +1605,34 @@ parallelsApplyVideoParams(parallelsDomObjPtr > > pdom, > > > > return 0; > > > > } > > > > -static int parallelsAddHddByVolume(parallelsDomObjPtr pdom, > > - virDomainDiskDefPtr disk, > > - virStoragePoolObjPtr pool, > > - virStorageVolDefPtr voldef) > > +static int parallelsAddHdd(parallelsDomObjPtr pdom, > > + virDomainDiskDefPtr disk) > > > > { > > > > int ret = -1; > > > > + const char *src = virDomainDiskGetSource(disk); > > + int type = virDomainDiskGetType(disk); > > > > const char *strbus; > > > > virCommandPtr cmd = virCommandNewArgList(PRLCTL, "set", pdom->uuid, > > > > "--device-add", "hdd", > > NULL); > > > > - virCommandAddArgFormat(cmd, "--size=%lluM", voldef->target.capacity > > >> 20); + > > + if (type == VIR_STORAGE_TYPE_FILE) { > > + int format = virDomainDiskGetFormat(disk); > > + > > + if (format != VIR_STORAGE_FILE_PLOOP) > > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, > > + _("Invalid disk format: %d"), type); > > Missing a 'goto cleanup' after reporting the error, so that > execution returns Since this was the only bug in the series, I've gone ahead and fixed it myself and pushed all four patches to git.

Regards, Daniel