[libvirt] [PATCH 0/4] parallels: reimplement hard disks support

This patch series makes working with hard disks more natural to the libvirt API. It removes all links between storage and hypervisor driver, since only disks of type VIR_STORAGE_TYPE_VOLUME should use information from storage driver. Also it removes ability to create a VM where you want, because it's not really needed. You only need the ability to create hard disk images in the selected place. Dmitry Guryanov (4): parallels: add VIR_STORAGE_FILE_PLOOP format parallels: set file format in virDomainDef parallels: add disks correctly parallels: create VMs in the default place src/parallels/parallels_driver.c | 148 +++++++++------------------------------ src/util/virstoragefile.c | 4 +- src/util/virstoragefile.h | 1 + 3 files changed, 37 insertions(+), 116 deletions(-) -- 1.9.0

Add VIR_STORAGE_FILE_PLOOP format. This format is used to store disk images for virtual machines in PCS and containers in PCS, OpenVZ and also in Parallels Desktop for Mac. This format is described on OpenVZ site - https://openvz.org/Ploop (together with ploop devices). It consists of XML descriptor and one or more image files: base image and deltas. Format of the image files described here: https://openvz.org/Ploop/format. This patch only adds VIR_STORAGE_FILE_PLOOP constant, consequent patches will use it in parallels driver. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/util/virstoragefile.c | 4 +++- src/util/virstoragefile.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 96af27b..5c1ab62 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -62,7 +62,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, "cloop", "dmg", "iso", "vpc", "vdi", /* Not direct file formats, but used for various drivers */ - "fat", "vhd", + "fat", "vhd", "ploop", /* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk") @@ -230,6 +230,8 @@ static struct FileTypeInfo const fileTypeInfo[] = { -1, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, {0}, 0, 0, 0, 0, NULL, NULL }, + [VIR_STORAGE_FILE_PLOOP] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, + -1, {0}, 0, 0, 0, 0, NULL, NULL }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 6072409..55de3a2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -72,6 +72,7 @@ typedef enum { /* Not direct file formats, but used for various drivers */ VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, + VIR_STORAGE_FILE_PLOOP, /* Not a format, but a marker: all formats below this point have * libvirt support for following a backing chain */ -- 1.9.0

On Wed, May 07, 2014 at 10:04:06PM +0400, Dmitry Guryanov wrote:
Add VIR_STORAGE_FILE_PLOOP format. This format is used to store disk images for virtual machines in PCS and containers in PCS, OpenVZ and also in Parallels Desktop for Mac.
This format is described on OpenVZ site - https://openvz.org/Ploop (together with ploop devices). It consists of XML descriptor and one or more image files: base image and deltas. Format of the image files described here: https://openvz.org/Ploop/format.
Hmm, so 'ploop' now refers to both a kernel device driver and also an image format ? I thought that historically 'ploop' was just a device driver, which in turn would support raw or qcow2, or <blah> image formats ? 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 :|

On Thursday 08 May 2014 10:43:41 Daniel P. Berrange wrote:
On Wed, May 07, 2014 at 10:04:06PM +0400, Dmitry Guryanov wrote:
Add VIR_STORAGE_FILE_PLOOP format. This format is used to store disk images for virtual machines in PCS and containers in PCS, OpenVZ and also in Parallels Desktop for Mac.
This format is described on OpenVZ site - https://openvz.org/Ploop (together with ploop devices). It consists of XML descriptor and one or more image files: base image and deltas. Format of the image files described here: https://openvz.org/Ploop/format.
Hmm, so 'ploop' now refers to both a kernel device driver and also an image format ? I thought that historically 'ploop' was just a device driver, which in turn would support raw or qcow2, or <blah> image formats ?
Yes, you're right, we don't have any separate name for image format, so in man pages it's called ploop1 or expanded. Now ploop devices support only raw and ploop images.
Regards, Daniel
-- Dmitry Guryanov

On Wed, May 07, 2014 at 10:04:06PM +0400, Dmitry Guryanov wrote:
Add VIR_STORAGE_FILE_PLOOP format. This format is used to store disk images for virtual machines in PCS and containers in PCS, OpenVZ and also in Parallels Desktop for Mac.
This format is described on OpenVZ site - https://openvz.org/Ploop (together with ploop devices). It consists of XML descriptor and one or more image files: base image and deltas. Format of the image files described here: https://openvz.org/Ploop/format.
This patch only adds VIR_STORAGE_FILE_PLOOP constant, consequent patches will use it in parallels driver.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/util/virstoragefile.c | 4 +++- src/util/virstoragefile.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 96af27b..5c1ab62 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -62,7 +62,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, "cloop", "dmg", "iso", "vpc", "vdi", /* Not direct file formats, but used for various drivers */ - "fat", "vhd", + "fat", "vhd", "ploop", /* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk")
@@ -230,6 +230,8 @@ static struct FileTypeInfo const fileTypeInfo[] = { -1, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, {0}, 0, 0, 0, 0, NULL, NULL }, + [VIR_STORAGE_FILE_PLOOP] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, + -1, {0}, 0, 0, 0, 0, NULL, NULL },
Is there no magic byte sequence to identify the ploop image format ? ACK if there is none. 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 :|

On Thursday 08 May 2014 11:58:22 Daniel P. Berrange wrote:
On Wed, May 07, 2014 at 10:04:06PM +0400, Dmitry Guryanov wrote:
Add VIR_STORAGE_FILE_PLOOP format. This format is used to store disk images for virtual machines in PCS and containers in PCS, OpenVZ and also in Parallels Desktop for Mac.
This format is described on OpenVZ site - https://openvz.org/Ploop (together with ploop devices). It consists of XML descriptor and one or more image files: base image and deltas. Format of the image files described here: https://openvz.org/Ploop/format.
This patch only adds VIR_STORAGE_FILE_PLOOP constant, consequent patches will use it in parallels driver.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> ---
src/util/virstoragefile.c | 4 +++- src/util/virstoragefile.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 96af27b..5c1ab62 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -62,7 +62,7 @@ VIR_ENUM_IMPL(virStorageFileFormat,
"cloop", "dmg", "iso", "vpc", "vdi", /* Not direct file formats, but used for various drivers */
- "fat", "vhd", + "fat", "vhd", "ploop",
/* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk")
@@ -230,6 +230,8 @@ static struct FileTypeInfo const fileTypeInfo[] = {
-1, {0}, 0, 0, 0, 0, NULL, NULL },
[VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
-1, {0}, 0, 0, 0, 0, NULL, NULL },
+ [VIR_STORAGE_FILE_PLOOP] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, + -1, {0}, 0, 0, 0, 0, NULL, NULL },
Is there no magic byte sequence to identify the ploop image format ?
It refers to a directory with DiskDescriptor.xml file, base image and possibly some deltas. So it's not a file format.
ACK if there is none.
Regards, Daniel
-- Dmitry Guryanov

Set file format in virDomainDef structure to produce correct XML in virDomainGetXMLDesc function. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 35798ac..b2de12f 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -321,6 +321,8 @@ parallelsGetHddInfo(virDomainDefPtr def, if (virDomainDiskSetSource(disk, tmp) < 0) return -1; + + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_PLOOP); } tmp = virJSONValueObjectGetString(value, "port"); -- 1.9.0

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@parallels.com> --- 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); + + virCommandAddArg(cmd, "--image"); + } else if (VIR_STORAGE_TYPE_BLOCK) { + virCommandAddArg(cmd, "--device"); + } else { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Invalid disk type: %d"), type); + goto cleanup; + } + + virCommandAddArg(cmd, src); if (!(strbus = parallelsGetDiskBusName(disk->bus))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, @@ -1632,54 +1649,10 @@ static int parallelsAddHddByVolume(parallelsDomObjPtr pdom, if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (parallelsStorageVolDefRemove(pool, voldef)) - goto cleanup; - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; -} - -static int parallelsAddHdd(virConnectPtr conn, - parallelsDomObjPtr pdom, - virDomainDiskDefPtr disk) -{ - parallelsConnPtr privconn = conn->privateData; - virStorageVolDefPtr voldef = NULL; - virStoragePoolObjPtr pool = NULL; - virStorageVolPtr vol = NULL; - int ret = -1; - const char *src = virDomainDiskGetSource(disk); - - if (!(vol = parallelsStorageVolLookupByPathLocked(conn, src))) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find volume with path '%s'"), src); - return -1; - } - - pool = virStoragePoolObjFindByName(&privconn->pools, vol->pool); - if (!pool) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find storage pool with name '%s'"), - vol->pool); - goto cleanup; - } - - voldef = virStorageVolDefFindByPath(pool, src); - if (!voldef) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find storage volume definition for path '%s'"), - src); - goto cleanup; - } - - ret = parallelsAddHddByVolume(pdom, disk, pool, voldef); cleanup: - if (pool) - virStoragePoolObjUnlock(pool); - virObjectUnref(vol); + virCommandFree(cmd); return ret; } @@ -1700,7 +1673,7 @@ static int parallelsRemoveHdd(parallelsDomObjPtr pdom, } static int -parallelsApplyDisksParams(virConnectPtr conn, parallelsDomObjPtr pdom, +parallelsApplyDisksParams(parallelsDomObjPtr pdom, virDomainDiskDefPtr *olddisks, int nold, virDomainDiskDefPtr *newdisks, int nnew) { @@ -1766,7 +1739,7 @@ parallelsApplyDisksParams(virConnectPtr conn, parallelsDomObjPtr pdom, if (found) continue; - if (parallelsAddHdd(conn, pdom, newdisk)) + if (parallelsAddHdd(pdom, newdisk)) return -1; } @@ -1954,7 +1927,7 @@ parallelsApplyIfacesParams(parallelsDomObjPtr pdom, } static int -parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr new) +parallelsApplyChanges(virDomainObjPtr dom, virDomainDefPtr new) { char buf[32]; size_t i; @@ -2191,7 +2164,7 @@ parallelsApplyChanges(virConnectPtr conn, virDomainObjPtr dom, virDomainDefPtr n if (parallelsApplyVideoParams(pdom, old->videos, old->nvideos, new->videos, new->nvideos) < 0) return -1; - if (parallelsApplyDisksParams(conn, pdom, old->disks, old->ndisks, + if (parallelsApplyDisksParams(pdom, old->disks, old->ndisks, new->disks, new->ndisks) < 0) return -1; if (parallelsApplyIfacesParams(pdom, old->nets, old->nnets, @@ -2341,7 +2314,7 @@ parallelsDomainDefineXML(virConnectPtr conn, const char *xml) } } - if (parallelsApplyChanges(conn, olddom, def) < 0) { + if (parallelsApplyChanges(olddom, def) < 0) { virObjectUnlock(olddom); goto cleanup; } -- 1.9.0

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@parallels.com> --- 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
+ + virCommandAddArg(cmd, "--image"); + } else if (VIR_STORAGE_TYPE_BLOCK) { + virCommandAddArg(cmd, "--device"); + } else { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Invalid disk type: %d"), type); + goto cleanup; + } + + virCommandAddArg(cmd, src);
if (!(strbus = parallelsGetDiskBusName(disk->bus))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, @@ -1632,54 +1649,10 @@ static int parallelsAddHddByVolume(parallelsDomObjPtr pdom, if (virCommandRun(cmd, NULL) < 0) goto cleanup;
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 :|

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@parallels.com> --- 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 -- |: 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 :|

On Monday 19 May 2014 13:06:21 Daniel P. Berrange wrote: > 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@parallels.com> > > > --- > > > > > > 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. Thanks, Daniel, I thought you have objections against first patch, so was waiting for your answer :) > > Regards, > Daniel -- Dmitry Guryanov

Each VM consists of a set of files in PCS: config, hard disk images, log file, memory dump. All these files are stored in a per-vm directory. When we create a new VM, we can ether specify path to the VM or create the VM in a default path (<default path>/<vm name>.pvm). This default path can be configured with command prlsrvctl user set --def-vm-home <path> command. Currenty parallels driver creates VM in the same place, where first hard disk is located. Let's change this logic and create VMs in the default path. It will be much clearer and allow us to create VMs without hard disks. Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 63 ++-------------------------------------- 1 file changed, 3 insertions(+), 60 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 67b28c4..f3ef260 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -2175,74 +2175,17 @@ parallelsApplyChanges(virDomainObjPtr dom, virDomainDefPtr new) } static int -parallelsCreateVm(virConnectPtr conn, virDomainDefPtr def) +parallelsCreateVm(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDefPtr def) { - parallelsConnPtr privconn = conn->privateData; - size_t i; - virStorageVolDefPtr privvol = NULL; - virStoragePoolObjPtr pool = NULL; - virStorageVolPtr vol = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - const char *src; - - for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK) - continue; - - src = virDomainDiskGetSource(def->disks[i]); - vol = parallelsStorageVolLookupByPathLocked(conn, src); - if (!vol) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find volume with path '%s'"), - src); - return -1; - } - break; - } - - if (!vol) { - /* We determine path to VM directory from volume, so - * let's report error if no disk until better solution - * will be found */ - virReportError(VIR_ERR_INVALID_ARG, - _("Can't create VM '%s' without hard disks"), - def->name ? def->name : _("(unnamed)")); - return -1; - } - - pool = virStoragePoolObjFindByName(&privconn->pools, vol->pool); - if (!pool) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find storage pool with name '%s'"), - vol->pool); - goto error; - } - - privvol = virStorageVolDefFindByPath(pool, src); - if (!privvol) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find storage volume definition for path '%s'"), - src); - goto error2; - } virUUIDFormat(def->uuid, uuidstr); - if (parallelsCmdRun(PRLCTL, "create", def->name, "--dst", - pool->def->target.path, "--no-hdd", + if (parallelsCmdRun(PRLCTL, "create", def->name, "--no-hdd", "--uuid", uuidstr, NULL) < 0) - goto error2; - - virStoragePoolObjUnlock(pool); - virObjectUnref(vol); + return -1; return 0; - - error2: - virStoragePoolObjUnlock(pool); - error: - virObjectUnref(vol); - return -1; } static int -- 1.9.0

On Wed, May 07, 2014 at 10:04:09PM +0400, Dmitry Guryanov wrote:
Each VM consists of a set of files in PCS: config, hard disk images, log file, memory dump. All these files are stored in a per-vm directory. When we create a new VM, we can ether specify path to the VM or create the VM in a default path (<default path>/<vm name>.pvm). This default path can be configured with command prlsrvctl user set --def-vm-home <path> command.
Currenty parallels driver creates VM in the same place, where first hard disk is located. Let's change this logic and create VMs in the default path. It will be much clearer and allow us to create VMs without hard disks.
Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- src/parallels/parallels_driver.c | 63 ++-------------------------------------- 1 file changed, 3 insertions(+), 60 deletions(-)
ACK 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 :|
participants (2)
-
Daniel P. Berrange
-
Dmitry Guryanov