9:30 p.m.
Hello,
This 3 patches fix different things, used by nova.
Dmitry Guryanov (3):
parallels: support NULL virDomainVideoAccelDefPtr
parallels: set format for real disk devices
parallels: use disk name to set disk index
src/parallels/parallels_sdk.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
--
1.9.3
9:30 p.m.
New subject: [libvirt] [PATCH 1/3] parallels: support NULL virDomainVideoAccelDefPtr
I support if virDomainVideoAccelDefPtr is NULL it means
default values for video acceleration. So we don't need
to report error.
Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com>
---
src/parallels/parallels_sdk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
index 0b05bc1..0980f50 100644
--- a/src/parallels/parallels_sdk.c
+++ b/src/parallels/parallels_sdk.c
@@ -2029,7 +2029,7 @@ static int prlsdkCheckVideoUnsupportedParams(virDomainDefPtr def)
return -1;
}
- if (v->accel == NULL || v->accel->support2d || v->accel->support3d) {
+ if (v->accel != NULL && (v->accel->support2d ||
v->accel->support3d)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parallels Cloud Server doesn't support "
"setting video acceleration parameters."));
--
1.9.3
4:24 a.m.
New subject: [libvirt] [PATCH 1/3] parallels: support NULL virDomainVideoAccelDefPtr
Looks OK
Best,
Maxim Nestratov
________________________________
От: Dmitry Guryanov<mailto:dguryanov@parallels.com>
Отправлено: 10.12.2014 17:30
Кому: libvir-list@redhat.com<mailto:libvir-list@redhat.com>
Копия: Alexander Burluka<mailto:mipt-aburluka@parallels.com>; Maxim
Nestratov<mailto:mnestratov@parallels.com>; Dmitry
Guryanov<mailto:dguryanov@parallels.com>
Тема: [PATCH 1/3] parallels: support NULL virDomainVideoAccelDefPtr
I support if virDomainVideoAccelDefPtr is NULL it means
default values for video acceleration. So we don't need
to report error.
Signed-off-by: Dmitry Guryanov <dguryanov(a)parallels.com>
---
src/parallels/parallels_sdk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
index 0b05bc1..0980f50 100644
--- a/src/parallels/parallels_sdk.c
+++ b/src/parallels/parallels_sdk.c
@@ -2029,7 +2029,7 @@ static int prlsdkCheckVideoUnsupportedParams(virDomainDefPtr def)
return -1;
}
- if (v->accel == NULL || v->accel->support2d || v->accel->support3d)
{
+ if (v->accel != NULL && (v->accel->support2d ||
v->accel->support3d)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parallels Cloud Server doesn't support "
"setting video acceleration parameters."));
--
1.9.3
9:30 p.m.
New subject: [libvirt] [PATCH 2/3] parallels: set format for real disk devices
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>
---
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;
}
--
1.9.3
6:21 p.m.
New subject: [libvirt] [PATCH 2/3] parallels: set format for real disk devices
On 12/10/14 14:30, Dmitry Guryanov wrote:
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>
---
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)) {
Incorrect indentation.
+
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid disk format: %d"),
disk->src->type);
This error message looks misleading. How about "Invalid format of disk
'%s'" disk->src->path ?
Also the same error message is a few lines above so it's probably worth
a separate cleanup.
+ goto cleanup;
+ }
emutype = PDT_USE_REAL_DEVICE;
}
I'll fix the indentation before pushing.
Peter
9:18 p.m.
New subject: [libvirt] [PATCH 2/3] parallels: set format for real disk devices
On Thursday 11 December 2014 11:21:23 Peter Krempa wrote:
On 12/10/14 14:30, Dmitry Guryanov wrote:
> 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>
> ---
>
> 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)) {
Incorrect indentation.
> +
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Invalid disk format: %d"),
> disk->src->type);
This error message looks misleading. How about "Invalid format of disk
'%s'" disk->src->path ?
Also the same error message is a few lines above so it's probably worth
a separate cleanup.
I can fix it, because I have to resend 3rd patch.
> + goto cleanup;
> + }
>
> emutype = PDT_USE_REAL_DEVICE;
>
> }
I'll fix the indentation before pushing.
Peter
--
Dmitry Guryanov
9:30 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
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>
---
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) {
--
1.9.3
4:24 a.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
Looks ok to me
Best,
Maxim Nestratov
________________________________
От: Dmitry Guryanov<mailto:dguryanov@parallels.com>
Отправлено: 10.12.2014 17:30
Кому: libvir-list@redhat.com<mailto:libvir-list@redhat.com>
Копия: Alexander Burluka<mailto:mipt-aburluka@parallels.com>; Maxim
Nestratov<mailto:mnestratov@parallels.com>; Dmitry
Guryanov<mailto:dguryanov@parallels.com>
Тема: [PATCH 3/3] parallels: use disk name to set disk index
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>
---
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) {
--
1.9.3
6:32 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
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>
---
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?
+ 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) {
9:21 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
On Thursday 11 December 2014 11:32:14 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>
> ---
>
> 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?
I think it should be used, if virDomainDeviceInfoIsSet() returns false. I
don't really know, if anybody uses it, but it seems more correct to me to
specify position on a bus, than make an assumption about device name inside
guest OS.
Nova uses device name.
> + 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) {
--
Dmitry Guryanov
9:24 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
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>
> ---
> 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
--
|: 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 :|
9:26 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
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>
> > ---
> > 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
--
|: 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 :|
7 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
On Thursday 11 December 2014 13:26:48 Daniel P. Berrange wrote:
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>
> > > ---
> > >
> > > 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.
Sorry, missed this message. You're right, I've sent new version of patches.
Regards,
Daniel
--
Dmitry Guryanov
11:06 p.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
On Thursday 11 December 2014 13:24:17 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>
> > ---
> >
> > 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
I think we can call virDomainDiskDefAssignAddress if disk->info is not set,
because all we need to do is to create device in PCS config, and disk->info
will not be saved anywhere in libvirt.
Regards,
Daniel
--
Dmitry Guryanov
1:59 a.m.
New subject: [libvirt] [PATCH 3/3] parallels: use disk name to set disk index
On Thursday 11 December 2014 13:24:17 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>
> > ---
> >
> > 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
Now virDomainDefParseString calls virDomainDiskDefAssignAddress if device
address isn't specified in xml. I've misunderstood meaning of fields of
virDomainDeviceDriveAddress structure. Drive position on bus is written to
'unit', not 'target'. So I think using correct fields of
virDomainDeviceDriveAddress will be enough.
Regards,
Daniel
--
Dmitry Guryanov
3598
days inactive
3600
days old
14 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Dmitry Guryanov -
Maxim Nestratov -
Peter Krempa