It seems file format is usually specified event for real block devices. So report that file format is raw in virDomainGetXMLDesc and add checks for proper file format to prlsdkAddDisk. Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com&gt; --- src/parallels/parallels_sdk.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 0980f50..e2a1e6c 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -476,6 +476,7 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk, virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_PLOOP); } else { virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK); + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); } if (isCdrom) @@ -2493,6 +2494,15 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) emutype = PDT_USE_IMAGE_FILE; } else { + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && + (virDomainDiskGetFormat(disk) != VIR_STORAGE_FILE_RAW && + virDomainDiskGetFormat(disk) != VIR_STORAGE_FILE_NONE && + virDomainDiskGetFormat(disk) != VIR_STORAGE_FILE_AUTO)) {

+ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid disk format: %d"), disk->src->type);

+ goto cleanup; + } emutype = PDT_USE_REAL_DEVICE; }

Use guest disk name to determine disk position on bus, because Openstack/nova don't set virDomainDeviceInfo. Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com&gt; --- src/parallels/parallels_sdk.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index e2a1e6c..c85e6d9 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -2467,6 +2467,7 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) int ret = -1; PRL_VM_DEV_EMULATION_TYPE emutype; PRL_MASS_STORAGE_INTERFACE_TYPE sdkbus; + int idx; if (prlsdkCheckDiskUnsupportedParams(disk) < 0) return -1; @@ -2535,7 +2536,14 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) pret = PrlVmDev_SetIfaceType(sdkdisk, sdkbus); prlsdkCheckRetGoto(pret, cleanup); - pret = PrlVmDev_SetStackIndex(sdkdisk, disk->info.addr.drive.target); + idx = virDiskNameToIndex(disk->dst);

+ if (idx < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk type '%s'"), disk->dst); + goto cleanup; + } + + pret = PrlVmDev_SetStackIndex(sdkdisk, idx); prlsdkCheckRetGoto(pret, cleanup); switch (disk->cachemode) {

On 12/10/14 14:30, Dmitry Guryanov wrote: > Use guest disk name to determine disk position on > bus, because Openstack/nova don't set virDomainDeviceInfo. > > Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com&gt; > --- > src/parallels/parallels_sdk.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c > index e2a1e6c..c85e6d9 100644 > --- a/src/parallels/parallels_sdk.c > +++ b/src/parallels/parallels_sdk.c > @@ -2467,6 +2467,7 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) > int ret = -1; > PRL_VM_DEV_EMULATION_TYPE emutype; > PRL_MASS_STORAGE_INTERFACE_TYPE sdkbus; > + int idx; > > if (prlsdkCheckDiskUnsupportedParams(disk) < 0) > return -1; > @@ -2535,7 +2536,14 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) > pret = PrlVmDev_SetIfaceType(sdkdisk, sdkbus); > prlsdkCheckRetGoto(pret, cleanup); > > - pret = PrlVmDev_SetStackIndex(sdkdisk, disk->info.addr.drive.target); > + idx = virDiskNameToIndex(disk->dst); Shouldn't this be used only if disk->info.addr.drive.target doesn't contain any info? Does anybody ever set it meaningfully so that it does work?

On Thu, Dec 11, 2014 at 01:24:17PM +0000, Daniel P. Berrange wrote: > On Thu, Dec 11, 2014 at 11:32:14AM +0100, Peter Krempa wrote: > > On 12/10/14 14:30, Dmitry Guryanov wrote: > > > Use guest disk name to determine disk position on > > > bus, because Openstack/nova don't set virDomainDeviceInfo. > > > > > > Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com&gt; > > > --- > > > > > > src/parallels/parallels_sdk.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/parallels/parallels_sdk.c > > > b/src/parallels/parallels_sdk.c > > > index e2a1e6c..c85e6d9 100644 > > > --- a/src/parallels/parallels_sdk.c > > > +++ b/src/parallels/parallels_sdk.c > > > @@ -2467,6 +2467,7 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, > > > virDomainDiskDefPtr disk)> > > > > > int ret = -1; > > > PRL_VM_DEV_EMULATION_TYPE emutype; > > > PRL_MASS_STORAGE_INTERFACE_TYPE sdkbus; > > > > > > + int idx; > > > > > > if (prlsdkCheckDiskUnsupportedParams(disk) < 0) > > > > > > return -1; > > > > > > @@ -2535,7 +2536,14 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, > > > virDomainDiskDefPtr disk)> > > > > > pret = PrlVmDev_SetIfaceType(sdkdisk, sdkbus); > > > prlsdkCheckRetGoto(pret, cleanup); > > > > > > - pret = PrlVmDev_SetStackIndex(sdkdisk, > > > disk->info.addr.drive.target); > > > + idx = virDiskNameToIndex(disk->dst); > > > > Shouldn't this be used only if disk->info.addr.drive.target doesn't > > contain any info? Does anybody ever set it meaningfully so that it does > > work? > > Strictly speaking the right thing todo would be to have a method called > before starting the guest, which populates the disk->info.addr based > on disk->dst, if the user hasn't already provided that info. Then this > particularly code here would be unchanged. eg something needs to be > calling virDomainDiskDefAssignAddress And actually, the domain XML parser is calling that when no address is set, so the address should be present. That said, shouldn't this code be using the 'unit' attribute rather than the 'target' attribute, or a combo of both of them, depending onthe controller type.

Regards, Daniel

On Thu, Dec 11, 2014 at 11:32:14AM +0100, Peter Krempa wrote: > On 12/10/14 14:30, Dmitry Guryanov wrote: > > Use guest disk name to determine disk position on > > bus, because Openstack/nova don't set virDomainDeviceInfo. > > > > Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com&gt; > > --- > > > > src/parallels/parallels_sdk.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/src/parallels/parallels_sdk.c > > b/src/parallels/parallels_sdk.c > > index e2a1e6c..c85e6d9 100644 > > --- a/src/parallels/parallels_sdk.c > > +++ b/src/parallels/parallels_sdk.c > > @@ -2467,6 +2467,7 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, > > virDomainDiskDefPtr disk)> > > > int ret = -1; > > PRL_VM_DEV_EMULATION_TYPE emutype; > > PRL_MASS_STORAGE_INTERFACE_TYPE sdkbus; > > > > + int idx; > > > > if (prlsdkCheckDiskUnsupportedParams(disk) < 0) > > > > return -1; > > > > @@ -2535,7 +2536,14 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, > > virDomainDiskDefPtr disk)> > > > pret = PrlVmDev_SetIfaceType(sdkdisk, sdkbus); > > prlsdkCheckRetGoto(pret, cleanup); > > > > - pret = PrlVmDev_SetStackIndex(sdkdisk, > > disk->info.addr.drive.target); > > + idx = virDiskNameToIndex(disk->dst); > > Shouldn't this be used only if disk->info.addr.drive.target doesn't > contain any info? Does anybody ever set it meaningfully so that it does > work? Strictly speaking the right thing todo would be to have a method called before starting the guest, which populates the disk->info.addr based on disk->dst, if the user hasn't already provided that info. Then this particularly code here would be unchanged. eg something needs to be calling virDomainDiskDefAssignAddress

Regards, Daniel