3:49 p.m.
Hello,
This patch series reworks the VirtualBox storage device handling code,
brief summary:
* Extend libvirt schema to specify IDE controller model as VBox supports
changing IDE model to PIIX3, PIIX4 or ICH6 and there are known cases
of legacy guest OSes being very sensitive to that.
* Make the driver recognize <controller> element at define time which
allows to specify controller model. Previously the driver was
completely ignoring that element.
* Allow to create vbox SAS controllers via
<controller type="scsi" model="lsisas1068" />
* Handle removable devices - define and dump devices without media.
* Make sure media is closed when undefining VMs. Leaving media unclosed
may cause errors when defining VMs pointing at the same local path.
Dawid Zamirski (6):
vbox: Close media when undefining domains.
domain: Allow 'model' attribute for ide controller.
docs: Add info about ide model attribute.
vbox: Add more IStorageController API mappings.
vbox: Process controller definitions from xml.
vbox: Update XML dump of storage devices.
docs/formatdomain.html.in | 3 +
docs/schemas/domaincommon.rng | 18 +-
src/conf/domain_conf.c | 9 +
src/conf/domain_conf.h | 9 +
src/libvirt_private.syms | 2 +
src/vbox/vbox_common.c | 1212 +++++++++++++++++++++++------------------
src/vbox/vbox_common.h | 21 +
src/vbox/vbox_tmpl.c | 87 +--
src/vbox/vbox_uniformed_api.h | 3 +
9 files changed, 790 insertions(+), 574 deletions(-)
--
2.14.2
3:49 p.m.
New subject: [libvirt] [PATCH 1/6] vbox: Close media when undefining domains.
From: Dawid Zamirski <dzamirski(a)datto.com>
When registering a VM we call OpenMedium on each disk image which adds it
to vbox's global media registry. Therefore, we should make sure to call
Close when unregistering VM so we cleanup the media registry entries
after ourselves - this does not remove disk image files. This follows
the behaviour of the VBoxManage unregistervm command.
---
src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index dffeabde0..ac3b8fa00 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, IMachine
**machine)
{
nsresult rc;
vboxArray media = VBOX_ARRAY_INITIALIZER;
+ size_t i;
+
rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid->value,
machine);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_NO_DOMAIN, "%s",
@@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, IMachine
**machine)
return rc;
}
- /* We're not interested in the array returned by the Unregister method,
- * but in the side effect of unregistering the virtual machine. In order
- * to call the Unregister method correctly we need to use the vboxArray
- * wrapper here. */
rc = vboxArrayGetWithUintArg(&media, *machine,
(*machine)->vtbl->Unregister,
- CleanupMode_DetachAllReturnNone);
+ CleanupMode_DetachAllReturnHardDisksOnly);
+
+ if (NS_FAILED(rc))
+ goto cleanup;
+
+ /* close each medium attached to VM to remove from media registry */
+ for (i = 0; i < media.count; i++) {
+ IMedium *medium = media.items[i];
+
+ if (!medium)
+ continue;
+
+ /* it's ok to ignore failure here - e.g. it may be used by another VM */
+ medium->vtbl->Close(medium);
+ }
+
+ cleanup:
vboxArrayUnalloc(&media);
return rc;
}
--
2.14.2
2:44 p.m.
New subject: [libvirt] [PATCH 1/6] vbox: Close media when undefining domains.
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski(a)datto.com>
When registering a VM we call OpenMedium on each disk image which adds it
to vbox's global media registry. Therefore, we should make sure to call
Close when unregistering VM so we cleanup the media registry entries
after ourselves - this does not remove disk image files. This follows
the behaviour of the VBoxManage unregistervm command.
---
src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
I'm not a vbox expert by any stretch, looking at this mostly because
it's been sitting on list unreviewed...
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index dffeabde0..ac3b8fa00 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, IMachine
**machine)
{
nsresult rc;
vboxArray media = VBOX_ARRAY_INITIALIZER;
+ size_t i;
+
rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid->value,
machine);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_NO_DOMAIN, "%s",
@@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, IMachine
**machine)
return rc;
}
- /* We're not interested in the array returned by the Unregister method,
- * but in the side effect of unregistering the virtual machine. In order
- * to call the Unregister method correctly we need to use the vboxArray
- * wrapper here. */
I think you should keep the second sentence here - "In order to call..."
- IDC either way, but that would seem to still be important.
rc = vboxArrayGetWithUintArg(&media, *machine,
(*machine)->vtbl->Unregister,
- CleanupMode_DetachAllReturnNone);
+ CleanupMode_DetachAllReturnHardDisksOnly);
+
+ if (NS_FAILED(rc))
+ goto cleanup;
+
+ /* close each medium attached to VM to remove from media registry */
+ for (i = 0; i < media.count; i++) {
+ IMedium *medium = media.items[i];
+
+ if (!medium)
+ continue;
+
So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
through some great pain to determine whether it's a "fake" disk or not -
would this code need the same kind of logic?
/me wonders how much the existing code could be made more common -
beyond the "isCurrent" in the latter function the code appears to be the
same. If this code needs it, then it might be worth the effort to pass
'isCurrent' to some common helper and for the other caller, just pass
true instead... Which would be similar if this code needed that.
+ /* it's ok to ignore failure here - e.g. it may be used
by another VM */
+ medium->vtbl->Close(medium);
You could place an ignore_value(); around this. Again see the two API's
above they have a detailed explanation.
John
+ }
+
+ cleanup:
vboxArrayUnalloc(&media);
return rc;
}
4:28 p.m.
New subject: [libvirt] [PATCH 1/6] vbox: Close media when undefining domains.
On Tue, 2017-10-17 at 15:44 -0400, John Ferlan wrote:
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski <dzamirski(a)datto.com>
>
> When registering a VM we call OpenMedium on each disk image which
> adds it
> to vbox's global media registry. Therefore, we should make sure to
> call
> Close when unregistering VM so we cleanup the media registry
> entries
> after ourselves - this does not remove disk image files. This
> follows
> the behaviour of the VBoxManage unregistervm command.
> ---
> src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
I'm not a vbox expert by any stretch, looking at this mostly because
it's been sitting on list unreviewed...
Thank you :-)
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index dffeabde0..ac3b8fa00 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID
> *iid, IMachine **machine)
> {
> nsresult rc;
> vboxArray media = VBOX_ARRAY_INITIALIZER;
> + size_t i;
> +
> rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid-
> >value, machine);
> if (NS_FAILED(rc)) {
> virReportError(VIR_ERR_NO_DOMAIN, "%s",
> @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data,
> vboxIID *iid, IMachine **machine)
> return rc;
> }
>
> - /* We're not interested in the array returned by the
> Unregister method,
> - * but in the side effect of unregistering the virtual
> machine. In order
> - * to call the Unregister method correctly we need to use the
> vboxArray
> - * wrapper here. */
I think you should keep the second sentence here - "In order to
call..."
- IDC either way, but that would seem to still be important.
Since this patch adds a code that loops over this array to close each
IMedium instance it's self-documenting so no there's need for code
comments here.
> rc = vboxArrayGetWithUintArg(&media, *machine,
(*machine)-
> >vtbl->Unregister,
> - CleanupMode_DetachAllReturnNone);
> + CleanupMode_DetachAllReturnHardDi
> sksOnly);
> +
> + if (NS_FAILED(rc))
> + goto cleanup;
> +
> + /* close each medium attached to VM to remove from media
> registry */
> + for (i = 0; i < media.count; i++) {
> + IMedium *medium = media.items[i];
> +
> + if (!medium)
> + continue;
> +
So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
through some great pain to determine whether it's a "fake" disk or
not -
would this code need the same kind of logic?
/me wonders how much the existing code could be made more common -
beyond the "isCurrent" in the latter function the code appears to be
the
same. If this code needs it, then it might be worth the effort to
pass
'isCurrent' to some common helper and for the other caller, just pass
true instead... Which would be similar if this code needed that.
I intentionally tried to stay away from touching the snapshot related
functions even though it seems that they could share some code. Since
I've been working on this on and off between projects at work, at
initial stages it seemed like a couple of lines of code "here and
there" would let me set/change vbox storage controller models via
libvirtxml. Unfortunately, as I dug deeper and tested all sorts of
combinations more bugs (even in master branch) seemed to pop out and
the series grew to be much bigger than I originally intended. However,
since I'll have to reorganize the commits for V2 anyway, I'll take a
look at those functions and see what I can do.
> + /* it's ok to ignore failure here - e.g. it may be used by
> another VM */
> + medium->vtbl->Close(medium);
You could place an ignore_value(); around this. Again see the two
API's
above they have a detailed explanation.
John
> + }
> +
> + cleanup:
> vboxArrayUnalloc(&media);
> return rc;
> }
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:31 a.m.
New subject: [libvirt] [PATCH 1/6] vbox: Close media when undefining domains.
On 10/17/2017 05:28 PM, Dawid Zamirski wrote:
On Tue, 2017-10-17 at 15:44 -0400, John Ferlan wrote:
>
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
>> From: Dawid Zamirski <dzamirski(a)datto.com>
>>
>> When registering a VM we call OpenMedium on each disk image which
>> adds it
>> to vbox's global media registry. Therefore, we should make sure to
>> call
>> Close when unregistering VM so we cleanup the media registry
>> entries
>> after ourselves - this does not remove disk image files. This
>> follows
>> the behaviour of the VBoxManage unregistervm command.
>> ---
>> src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++-----
>> 1 file changed, 19 insertions(+), 5 deletions(-)
>>
>
> I'm not a vbox expert by any stretch, looking at this mostly because
> it's been sitting on list unreviewed...
Thank you :-)
>
>> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
>> index dffeabde0..ac3b8fa00 100644
>> --- a/src/vbox/vbox_tmpl.c
>> +++ b/src/vbox/vbox_tmpl.c
>> @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID
>> *iid, IMachine **machine)
>> {
>> nsresult rc;
>> vboxArray media = VBOX_ARRAY_INITIALIZER;
>> + size_t i;
>> +
>> rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid-
>>> value, machine);
>> if (NS_FAILED(rc)) {
>> virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data,
>> vboxIID *iid, IMachine **machine)
>> return rc;
>> }
>>
>> - /* We're not interested in the array returned by the
>> Unregister method,
>> - * but in the side effect of unregistering the virtual
>> machine. In order
>> - * to call the Unregister method correctly we need to use the
>> vboxArray
>> - * wrapper here. */
>
> I think you should keep the second sentence here - "In order to
> call..."
> - IDC either way, but that would seem to still be important.
>
Since this patch adds a code that loops over this array to close each
IMedium instance it's self-documenting so no there's need for code
comments here.
That's fine - for you I'm assuming it's obvious why the wrapper is
needed to call Unregister, but for me - less so.
>> rc = vboxArrayGetWithUintArg(&media, *machine,
(*machine)-
>>> vtbl->Unregister,
>> - CleanupMode_DetachAllReturnNone);
>> + CleanupMode_DetachAllReturnHardDi
>> sksOnly);
>> +
>> + if (NS_FAILED(rc))
>> + goto cleanup;
>> +
>> + /* close each medium attached to VM to remove from media
>> registry */
>> + for (i = 0; i < media.count; i++) {
>> + IMedium *medium = media.items[i];
>> +
>> + if (!medium)
>> + continue;
>> +
>
> So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
> APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
> through some great pain to determine whether it's a "fake" disk or
> not -
> would this code need the same kind of logic?
>
> /me wonders how much the existing code could be made more common -
> beyond the "isCurrent" in the latter function the code appears to be
> the
> same. If this code needs it, then it might be worth the effort to
> pass
> 'isCurrent' to some common helper and for the other caller, just pass
> true instead... Which would be similar if this code needed that.
I intentionally tried to stay away from touching the snapshot related
functions even though it seems that they could share some code. Since
I've been working on this on and off between projects at work, at
initial stages it seemed like a couple of lines of code "here and
there" would let me set/change vbox storage controller models via
libvirtxml. Unfortunately, as I dug deeper and tested all sorts of
combinations more bugs (even in master branch) seemed to pop out and
the series grew to be much bigger than I originally intended. However,
since I'll have to reorganize the commits for V2 anyway, I'll take a
look at those functions and see what I can do.
I try to avoid snapshot too ;-) I understand how making adjustments in
libvirt can be like peeling back layers of an onion that cause you to
cry each time you have to enter some other piece of code because either
you discover some new bug/issue or what you're doing affects some other
piece of code. That is though one reason why we encourage using "small
enough" functionality changing/fixing patches rather than some massive
patch doing many different things. Another reason for smaller patches
has to do with git bisect and being able to determine which patch broke
some functionality - you bisect back to some 800 line patch bomb and
attempt to figure out what caused the failure... Many times what I find
myself doing when I trip across some existing issue is stashing work and
rebasing to the patch before I've stashed, generating a patch for the
issue I uncover and then dealing with the merge conflicts after that.
John
>
>> + /* it's ok to ignore failure here - e.g. it may be used by
>> another VM */
>> + medium->vtbl->Close(medium);
>
> You could place an ignore_value(); around this. Again see the two
> API's
> above they have a detailed explanation.
>
>
> John
>
>> + }
>> +
>> + cleanup:
>> vboxArrayUnalloc(&media);
>> return rc;
>> }
>>
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3:49 p.m.
New subject: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.
From: Dawid Zamirski <dzamirski(a)datto.com>
The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
needed to allow setting IDE controller model in VirtualBox driver.
---
docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
src/conf/domain_conf.c | 9 +++++++++
src/conf/domain_conf.h | 9 +++++++++
src/libvirt_private.syms | 2 ++
4 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4dbda6932..c3f1557f0 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1927,12 +1927,11 @@
<ref name="address"/>
</optional>
<choice>
- <!-- fdc/ide/sata/ccid have only the common attributes -->
+ <!-- fdc/sata/ccid have only the common attributes -->
<group>
<attribute name="type">
<choice>
<value>fdc</value>
- <value>ide</value>
<value>sata</value>
<value>ccid</value>
</choice>
@@ -1993,6 +1992,21 @@
</attribute>
</optional>
</group>
+ <!-- ide has an optional attribute "model" -->
+ <group>
+ <attribute name="type">
+ <value>ide</value>
+ </attribute>
+ <optional>
+ <attribute name="model">
+ <choice>
+ <value>piix3</value>
+ <value>piix4</value>
+ <value>ich6</value>
+ </choice>
+ </attribute>
+ </optional>
+ </group>
<!-- pci has an optional attribute "model" -->
<group>
<attribute name="type">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 54be9028d..493bf83ff 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,
VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
"qemu-xhci",
"none")
+VIR_ENUM_IMPL(virDomainControllerModelIDE, VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
+ "piix3",
+ "piix4",
+ "ich6")
+
VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
"mount",
"block",
@@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef
*def,
return virDomainControllerModelUSBTypeFromString(model);
else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
return virDomainControllerModelPCITypeFromString(model);
+ else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+ return virDomainControllerModelIDETypeFromString(model);
return -1;
}
@@ -9482,6 +9489,8 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
return virDomainControllerModelUSBTypeToString(model);
else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
return virDomainControllerModelPCITypeToString(model);
+ else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+ return virDomainControllerModelIDETypeToString(model);
return NULL;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a42efcfa6..d7f4c3f1e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -748,6 +748,14 @@ typedef enum {
VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
} virDomainControllerModelUSB;
+typedef enum {
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3,
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4,
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6,
+
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST
+} virDomainControllerModelIDE;
+
# define IS_USB2_CONTROLLER(ctrl) \
(((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \
@@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI)
VIR_ENUM_DECL(virDomainControllerPCIModelName)
VIR_ENUM_DECL(virDomainControllerModelSCSI)
VIR_ENUM_DECL(virDomainControllerModelUSB)
+VIR_ENUM_DECL(virDomainControllerModelIDE)
VIR_ENUM_DECL(virDomainFS)
VIR_ENUM_DECL(virDomainFSDriver)
VIR_ENUM_DECL(virDomainFSAccessMode)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9243c5591..616b14f82 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex;
virDomainControllerInsert;
virDomainControllerInsertPreAlloced;
virDomainControllerIsPSeriesPHB;
+virDomainControllerModelIDETypeFromString;
+virDomainControllerModelIDETypeToString;
virDomainControllerModelPCITypeToString;
virDomainControllerModelSCSITypeFromString;
virDomainControllerModelSCSITypeToString;
--
2.14.2
2:46 p.m.
New subject: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski(a)datto.com>
The optional values are 'piix3', 'piix4' or 'ich6'. Those will
be
needed to allow setting IDE controller model in VirtualBox driver.
---
docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
src/conf/domain_conf.c | 9 +++++++++
src/conf/domain_conf.h | 9 +++++++++
src/libvirt_private.syms | 2 ++
4 files changed, 36 insertions(+), 2 deletions(-)
Patches 2 & 3 should be combined and you'll need to provide an xml2xml
example here, modifying tests/qemuxml2xmltest.c in order to add your
test. Search for controller type='scsi' and model= being set in any of
the tests/*/*.xml files. Then generate your test that would have
controller type='ide' ... model='xxx' (just one example is fine).
You may also need to alter virDomainControllerDefValidate to ensure
perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
aren't lost if ->model is filled in.
I am far from being an expert on IDE controllers and their existing
assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
references you will find the existing ones. My assumption has been that
the current model assumption is piix3 - so by adding piix4 and ich6
you'll need to alter code in order to handle any assumptions those
models have; otherwise, once code finds IDE it assumes piix3 and those
assumptions may not work well for piix4 and ich6.
diff --git a/docs/schemas/domaincommon.rng
b/docs/schemas/domaincommon.rng
index 4dbda6932..c3f1557f0 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1927,12 +1927,11 @@
<ref name="address"/>
</optional>
<choice>
- <!-- fdc/ide/sata/ccid have only the common attributes -->
+ <!-- fdc/sata/ccid have only the common attributes -->
<group>
<attribute name="type">
<choice>
<value>fdc</value>
- <value>ide</value>
<value>sata</value>
<value>ccid</value>
</choice>
@@ -1993,6 +1992,21 @@
</attribute>
</optional>
</group>
+ <!-- ide has an optional attribute "model" -->
+ <group>
+ <attribute name="type">
+ <value>ide</value>
+ </attribute>
+ <optional>
+ <attribute name="model">
+ <choice>
+ <value>piix3</value>
+ <value>piix4</value>
+ <value>ich6</value>
+ </choice>
+ </attribute>
+ </optional>
+ </group>
<!-- pci has an optional attribute "model" -->
<group>
<attribute name="type">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 54be9028d..493bf83ff 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,
VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
"qemu-xhci",
"none")
+VIR_ENUM_IMPL(virDomainControllerModelIDE, VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
+ "piix3",
+ "piix4",
+ "ich6")
+
VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
"mount",
"block",
@@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef
*def,
return virDomainControllerModelUSBTypeFromString(model);
else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
return virDomainControllerModelPCITypeFromString(model);
+ else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+ return virDomainControllerModelIDETypeFromString(model);
return -1;
}
@@ -9482,6 +9489,8 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr
def,
return virDomainControllerModelUSBTypeToString(model);
else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
return virDomainControllerModelPCITypeToString(model);
+ else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+ return virDomainControllerModelIDETypeToString(model);
return NULL;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a42efcfa6..d7f4c3f1e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -748,6 +748,14 @@ typedef enum {
VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
} virDomainControllerModelUSB;
+typedef enum {
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3,
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4,
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6,
+
+ VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST
+} virDomainControllerModelIDE;
+
# define IS_USB2_CONTROLLER(ctrl) \
(((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \
@@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI)
VIR_ENUM_DECL(virDomainControllerPCIModelName)
VIR_ENUM_DECL(virDomainControllerModelSCSI)
VIR_ENUM_DECL(virDomainControllerModelUSB)
+VIR_ENUM_DECL(virDomainControllerModelIDE)
VIR_ENUM_DECL(virDomainFS)
VIR_ENUM_DECL(virDomainFSDriver)
VIR_ENUM_DECL(virDomainFSAccessMode)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9243c5591..616b14f82 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex;
virDomainControllerInsert;
virDomainControllerInsertPreAlloced;
virDomainControllerIsPSeriesPHB;
+virDomainControllerModelIDETypeFromString;
+virDomainControllerModelIDETypeToString;
virDomainControllerModelPCITypeToString;
virDomainControllerModelSCSITypeFromString;
virDomainControllerModelSCSITypeToString;
"Currently" you probably don't need to export the *IDEType{To|From}*
functions since domain_conf is the only consumer; however, seeing how
the *SCSIType{To|From}* has multiple/external consumers makes me wonder
if more consumers are needed for IDE.
In particular, in qemuBuildControllerDevStr and some assumptions in
src/qemu/qemu_domain_address.c related to where controllers live on the
bus for piix3 (FWIW: This was written before the above last two
paragraphs - it's what prompted me to consider the needs).
John
11:23 a.m.
New subject: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.
On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski <dzamirski(a)datto.com>
>
> The optional values are 'piix3', 'piix4' or 'ich6'. Those
will be
> needed to allow setting IDE controller model in VirtualBox driver.
> ---
> docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
> src/conf/domain_conf.c | 9 +++++++++
> src/conf/domain_conf.h | 9 +++++++++
> src/libvirt_private.syms | 2 ++
> 4 files changed, 36 insertions(+), 2 deletions(-)
>
Patches 2 & 3 should be combined and you'll need to provide an
xml2xml
example here, modifying tests/qemuxml2xmltest.c in order to add your
test. Search for controller type='scsi' and model= being set in any
of
the tests/*/*.xml files. Then generate your test that would have
controller type='ide' ... model='xxx' (just one example is fine).
I understand that the patch with test cases should be separated,
corrent?
You may also need to alter virDomainControllerDefValidate to ensure
perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
aren't lost if ->model is filled in.
That's clear, no problem...
I am far from being an expert on IDE controllers and their existing
assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
references you will find the existing ones. My assumption has been
that
the current model assumption is piix3 - so by adding piix4 and ich6
you'll need to alter code in order to handle any assumptions those
models have; otherwise, once code finds IDE it assumes piix3 and
those
assumptions may not work well for piix4 and ich6.
...though, I'm a little concerned with this part. While I'm somewhat
familiar with libvirt qemu driver and can confirm that it implies PIIX3
for IDE (at least libvirt does by checking if emulated machine is 440FX
although qemu itself seems to support PIIX4 as well [1]), I'm not so
sure I could easily determine "defaults" for other drivers like ESX
(especially this one being closed-source) - AFAIK those drivers
basically allow to attach "an IDE controller" without any specifics on
what particular model of the southbridge the hypervisor should emulate
- I guess it's likely determined by the "machine" type or hwVersion
(ESX) and I suppose vbox is an "oddball" here allowing to set IDE
controller model independently. Would it be acceptable to update each
driver to check that ->model == -1 and error out if it's not? In other
words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
everything else would accept just -1 - guess those could be enforced
by implementing virDomainDeviceDefValidateCallback in each driver.
[1] https://github.com/qemu/qemu/blob/master/hw/ide/piix.c#L285
> diff --git a/docs/schemas/domaincommon.rng
> b/docs/schemas/domaincommon.rng
> index 4dbda6932..c3f1557f0 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1927,12 +1927,11 @@
> <ref name="address"/>
> </optional>
> <choice>
> - <!-- fdc/ide/sata/ccid have only the common attributes
> -->
> + <!-- fdc/sata/ccid have only the common attributes -->
> <group>
> <attribute name="type">
> <choice>
> <value>fdc</value>
> - <value>ide</value>
> <value>sata</value>
> <value>ccid</value>
> </choice>
> @@ -1993,6 +1992,21 @@
> </attribute>
> </optional>
> </group>
> + <!-- ide has an optional attribute "model" -->
> + <group>
> + <attribute name="type">
> + <value>ide</value>
> + </attribute>
> + <optional>
> + <attribute name="model">
> + <choice>
> + <value>piix3</value>
> + <value>piix4</value>
> + <value>ich6</value>
> + </choice>
> + </attribute>
> + </optional>
> + </group>
> <!-- pci has an optional attribute "model" -->
> <group>
> <attribute name="type">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 54be9028d..493bf83ff 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,
> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
> "qemu-xhci",
> "none")
>
> +VIR_ENUM_IMPL(virDomainControllerModelIDE,
> VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
> + "piix3",
> + "piix4",
> + "ich6")
> +
> VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
> "mount",
> "block",
> @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const
> virDomainControllerDef *def,
> return virDomainControllerModelUSBTypeFromString(model);
> else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> return virDomainControllerModelPCITypeFromString(model);
> + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> + return virDomainControllerModelIDETypeFromString(model);
>
> return -1;
> }
> @@ -9482,6 +9489,8 @@
> virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
> return virDomainControllerModelUSBTypeToString(model);
> else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> return virDomainControllerModelPCITypeToString(model);
> + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> + return virDomainControllerModelIDETypeToString(model);
>
> return NULL;
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a42efcfa6..d7f4c3f1e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -748,6 +748,14 @@ typedef enum {
> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
> } virDomainControllerModelUSB;
>
> +typedef enum {
> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3,
> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4,
> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6,
> +
> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST
> +} virDomainControllerModelIDE;
> +
> # define IS_USB2_CONTROLLER(ctrl) \
> (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
> ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1
> || \
> @@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI)
> VIR_ENUM_DECL(virDomainControllerPCIModelName)
> VIR_ENUM_DECL(virDomainControllerModelSCSI)
> VIR_ENUM_DECL(virDomainControllerModelUSB)
> +VIR_ENUM_DECL(virDomainControllerModelIDE)
> VIR_ENUM_DECL(virDomainFS)
> VIR_ENUM_DECL(virDomainFSDriver)
> VIR_ENUM_DECL(virDomainFSAccessMode)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9243c5591..616b14f82 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex;
> virDomainControllerInsert;
> virDomainControllerInsertPreAlloced;
> virDomainControllerIsPSeriesPHB;
> +virDomainControllerModelIDETypeFromString;
> +virDomainControllerModelIDETypeToString;
> virDomainControllerModelPCITypeToString;
> virDomainControllerModelSCSITypeFromString;
> virDomainControllerModelSCSITypeToString;
>
"Currently" you probably don't need to export the *IDEType{To|From}*
functions since domain_conf is the only consumer; however, seeing how
the *SCSIType{To|From}* has multiple/external consumers makes me
wonder
if more consumers are needed for IDE.
In particular, in qemuBuildControllerDevStr and some assumptions in
src/qemu/qemu_domain_address.c related to where controllers live on
the
bus for piix3 (FWIW: This was written before the above last two
paragraphs - it's what prompted me to consider the needs).
John
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:51 p.m.
New subject: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.
On 10/18/2017 12:23 PM, Dawid Zamirski wrote:
On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
>
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
>> From: Dawid Zamirski <dzamirski(a)datto.com>
>>
>> The optional values are 'piix3', 'piix4' or 'ich6'. Those
will be
>> needed to allow setting IDE controller model in VirtualBox driver.
>> ---
>> docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
>> src/conf/domain_conf.c | 9 +++++++++
>> src/conf/domain_conf.h | 9 +++++++++
>> src/libvirt_private.syms | 2 ++
>> 4 files changed, 36 insertions(+), 2 deletions(-)
>>
>
> Patches 2 & 3 should be combined and you'll need to provide an
> xml2xml
> example here, modifying tests/qemuxml2xmltest.c in order to add your
> test. Search for controller type='scsi' and model= being set in any
> of
> the tests/*/*.xml files. Then generate your test that would have
> controller type='ide' ... model='xxx' (just one example is fine).
>
I understand that the patch with test cases should be separated,
corrent?
Hmmm.... Typically when one makes .rng and domain_conf changes, then I
expect to also see *.html.in and tests/qemuxml2xmltest.c changes as well
as including the new .xml files in the input/output directories
(tests/qemuxml2argvdata and tests/qemuxml2xmloutdata). But this isn't
qemu.... So I guess I'm too used to reviewing qemu changes!
Since you're adding this for vbox and there isn't the same type xml
validation test for that (I see only tests/vboxsnapshotxmltest.c) that
just means you have less to do now. BTW: You will note there are
bhyvexml2xmltest and lxcxml2xmltests, but nothing for vbox, so I guess
it's a moot point since there's no way to test. Hey it is something to
add some day in the future if you or someone else is so inclined.
FWIW: If you made qemu driver changes, such as qemu_command.c - then one
would change the qemuxml2argvtest.c and create a .args output file.
> You may also need to alter virDomainControllerDefValidate to
ensure
> perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> aren't lost if ->model is filled in.
>
That's clear, no problem...
Of course perhaps a moot point too ...
> I am far from being an expert on IDE controllers and their
existing
> assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> references you will find the existing ones. My assumption has been
> that
> the current model assumption is piix3 - so by adding piix4 and ich6
> you'll need to alter code in order to handle any assumptions those
> models have; otherwise, once code finds IDE it assumes piix3 and
> those
> assumptions may not work well for piix4 and ich6.
>
...though, I'm a little concerned with this part. While I'm somewhat
familiar with libvirt qemu driver and can confirm that it implies PIIX3
for IDE (at least libvirt does by checking if emulated machine is 440FX
although qemu itself seems to support PIIX4 as well [1]), I'm not so
sure I could easily determine "defaults" for other drivers like ESX
(especially this one being closed-source) - AFAIK those drivers
basically allow to attach "an IDE controller" without any specifics on
what particular model of the southbridge the hypervisor should emulate
- I guess it's likely determined by the "machine" type or hwVersion
(ESX) and I suppose vbox is an "oddball" here allowing to set IDE
controller model independently. Would it be acceptable to update each
driver to check that ->model == -1 and error out if it's not? In other
words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
everything else would accept just -1 - guess those could be enforced
by implementing virDomainDeviceDefValidateCallback in each driver.
[1] https://github.com/qemu/qemu/blob/master/hw/ide/piix.c#L285
Well, again, it's my qemu review bias clearly showing through. The qemu
code will know nothing about models so it'll just happily ignore the
model type (at least for now until someday someone decides differently).
So, how to handle this? Hmmm... Well it seems there's perhaps nothing
to do unless there is some need/desire to ensure the <address> has
specific values in which case you'd probably need that device def
callback specific to vbox similarly to the qemu device def callback.
One other thought - thinking about patch3 and formatdomain.html.in - you
should indicate there that model types are only supported and handled
for the vbox hypervisor only - there are other examples of that already.
For example,
<dd><span class="since">Since 3.9.0</span> for the vbox
driver, the
<code>ide</code> controller has an optional attribute
<code>model</code>, which is one of "piix3", "piix4", or
"ich6".</dd>
I assume if the model='xxx' isn't provided, then some default is chosen.
Sorry for the confusion -
John
>> diff --git a/docs/schemas/domaincommon.rng
>> b/docs/schemas/domaincommon.rng
>> index 4dbda6932..c3f1557f0 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1927,12 +1927,11 @@
>> <ref name="address"/>
>> </optional>
>> <choice>
>> - <!-- fdc/ide/sata/ccid have only the common attributes
>> -->
>> + <!-- fdc/sata/ccid have only the common attributes -->
>> <group>
>> <attribute name="type">
>> <choice>
>> <value>fdc</value>
>> - <value>ide</value>
>> <value>sata</value>
>> <value>ccid</value>
>> </choice>
>> @@ -1993,6 +1992,21 @@
>> </attribute>
>> </optional>
>> </group>
>> + <!-- ide has an optional attribute "model" -->
>> + <group>
>> + <attribute name="type">
>> + <value>ide</value>
>> + </attribute>
>> + <optional>
>> + <attribute name="model">
>> + <choice>
>> + <value>piix3</value>
>> + <value>piix4</value>
>> + <value>ich6</value>
>> + </choice>
>> + </attribute>
>> + </optional>
>> + </group>
>> <!-- pci has an optional attribute "model" -->
>> <group>
>> <attribute name="type">
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 54be9028d..493bf83ff 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,
>> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
>> "qemu-xhci",
>> "none")
>>
>> +VIR_ENUM_IMPL(virDomainControllerModelIDE,
>> VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
>> + "piix3",
>> + "piix4",
>> + "ich6")
>> +
>> VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
>> "mount",
>> "block",
>> @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const
>> virDomainControllerDef *def,
>> return virDomainControllerModelUSBTypeFromString(model);
>> else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>> return virDomainControllerModelPCITypeFromString(model);
>> + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
>> + return virDomainControllerModelIDETypeFromString(model);
>>
>> return -1;
>> }
>> @@ -9482,6 +9489,8 @@
>> virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
>> return virDomainControllerModelUSBTypeToString(model);
>> else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>> return virDomainControllerModelPCITypeToString(model);
>> + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
>> + return virDomainControllerModelIDETypeToString(model);
>>
>> return NULL;
>> }
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index a42efcfa6..d7f4c3f1e 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -748,6 +748,14 @@ typedef enum {
>> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
>> } virDomainControllerModelUSB;
>>
>> +typedef enum {
>> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3,
>> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4,
>> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6,
>> +
>> + VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST
>> +} virDomainControllerModelIDE;
>> +
>> # define IS_USB2_CONTROLLER(ctrl) \
>> (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
>> ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1
>> || \
>> @@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI)
>> VIR_ENUM_DECL(virDomainControllerPCIModelName)
>> VIR_ENUM_DECL(virDomainControllerModelSCSI)
>> VIR_ENUM_DECL(virDomainControllerModelUSB)
>> +VIR_ENUM_DECL(virDomainControllerModelIDE)
>> VIR_ENUM_DECL(virDomainFS)
>> VIR_ENUM_DECL(virDomainFSDriver)
>> VIR_ENUM_DECL(virDomainFSAccessMode)
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 9243c5591..616b14f82 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex;
>> virDomainControllerInsert;
>> virDomainControllerInsertPreAlloced;
>> virDomainControllerIsPSeriesPHB;
>> +virDomainControllerModelIDETypeFromString;
>> +virDomainControllerModelIDETypeToString;
>> virDomainControllerModelPCITypeToString;
>> virDomainControllerModelSCSITypeFromString;
>> virDomainControllerModelSCSITypeToString;
>>
>
> "Currently" you probably don't need to export the *IDEType{To|From}*
> functions since domain_conf is the only consumer; however, seeing how
> the *SCSIType{To|From}* has multiple/external consumers makes me
> wonder
> if more consumers are needed for IDE.
>
> In particular, in qemuBuildControllerDevStr and some assumptions in
> src/qemu/qemu_domain_address.c related to where controllers live on
> the
> bus for piix3 (FWIW: This was written before the above last two
> paragraphs - it's what prompted me to consider the needs).
>
> John
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
1:04 p.m.
New subject: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.
On Wed, Oct 18, 2017 at 12:23:24PM -0400, Dawid Zamirski wrote:
On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
>
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > From: Dawid Zamirski <dzamirski(a)datto.com>
> >
> > The optional values are 'piix3', 'piix4' or 'ich6'.
Those will be
> > needed to allow setting IDE controller model in VirtualBox driver.
> > ---
> > docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
> > src/conf/domain_conf.c | 9 +++++++++
> > src/conf/domain_conf.h | 9 +++++++++
> > src/libvirt_private.syms | 2 ++
> > 4 files changed, 36 insertions(+), 2 deletions(-)
> >
>
> Patches 2 & 3 should be combined and you'll need to provide an
> xml2xml
> example here, modifying tests/qemuxml2xmltest.c in order to add your
> test. Search for controller type='scsi' and model= being set in any
> of
> the tests/*/*.xml files. Then generate your test that would have
> controller type='ide' ... model='xxx' (just one example is fine).
>
I understand that the patch with test cases should be separated,
corrent?
> You may also need to alter virDomainControllerDefValidate to ensure
> perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> aren't lost if ->model is filled in.
>
That's clear, no problem...
> I am far from being an expert on IDE controllers and their existing
> assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> references you will find the existing ones. My assumption has been
> that
> the current model assumption is piix3 - so by adding piix4 and ich6
> you'll need to alter code in order to handle any assumptions those
> models have; otherwise, once code finds IDE it assumes piix3 and
> those
> assumptions may not work well for piix4 and ich6.
>
...though, I'm a little concerned with this part. While I'm somewhat
familiar with libvirt qemu driver and can confirm that it implies PIIX3
for IDE (at least libvirt does by checking if emulated machine is 440FX
although qemu itself seems to support PIIX4 as well [1]), I'm not so
sure I could easily determine "defaults" for other drivers like ESX
(especially this one being closed-source) - AFAIK those drivers
basically allow to attach "an IDE controller" without any specifics on
what particular model of the southbridge the hypervisor should emulate
QEMU 'pc' machine type provides a piix3 IDE controller built-in,
and we can't (at time this) override this default.
You can then add extra IDE controllers which can be piix3 or piix4,
but we don't have that wired up for QEMU driver yet. It looks like
your proposed XML changes would let us do that though, which is nice.
- I guess it's likely determined by the "machine" type
or hwVersion
(ESX) and I suppose vbox is an "oddball" here allowing to set IDE
controller model independently. Would it be acceptable to update each
driver to check that ->model == -1 and error out if it's not? In other
words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
everything else would accept just -1 - guess those could be enforced
by implementing virDomainDeviceDefValidateCallback in each driver.
vbox isn't really an odd-ball actually. vbox has as machine type of
piix3 by default if you look at the 'chipset' docs here:
https://www.virtualbox.org/manual/ch03.html#settings-motherboard
the IDE controller model setting looks like it overrides the model
of this built-in IDE controller. This is slightly more flexible
than what QEMU does, in that you can overrde the default. Both
QEMU and VBox ways work.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
3:49 p.m.
New subject: [libvirt] [PATCH 3/6] docs: Add info about ide model attribute.
From: Dawid Zamirski <dzamirski(a)datto.com>
---
docs/formatdomain.html.in | 3 +++
1 file changed, 3 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c0e3c2221..ddcc9f1b1 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3596,6 +3596,9 @@
<span class="since">Since 1.3.5</span>, USB controllers
accept a
<code>ports</code> attribute to configure how many devices can be
connected to the controller.</dd>
+ <dt><code>ide</code></dt>
+ <dd>An <code>ide</code> controller has an optional attribute
+ <code>model</code>, which is one of "piix3",
"piix4" or "ich6".</dd>
</dl>
<p>
--
2.14.2
2:46 p.m.
New subject: [libvirt] [PATCH 3/6] docs: Add info about ide model attribute.
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski(a)datto.com>
---
docs/formatdomain.html.in | 3 +++
1 file changed, 3 insertions(+)
This would be merged into patch 2...
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c0e3c2221..ddcc9f1b1 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3596,6 +3596,9 @@
<span class="since">Since 1.3.5</span>, USB controllers
accept a
<code>ports</code> attribute to configure how many devices can be
connected to the controller.</dd>
+ <dt><code>ide</code></dt>
+ <dd>An <code>ide</code> controller has an optional attribute
+ <code>model</code>, which is one of "piix3",
"piix4" or "ich6".</dd>
Once this is integrated, you'd need to add a <span
class="since">Since
x.x.x</span> tag in order to indicate which release the change went into.
John
</dl>
<p>
3:49 p.m.
New subject: [libvirt] [PATCH 4/6] vbox: Add more IStorageController API mappings.
From: Dawid Zamirski <dzamirski(a)datto.com>
This patch 'maps' additonal methods for VBOX API to the libvirt unified
vbox API to deal with IStorageController. The 'mapped' methods are:
* IStorageController->GetStorageControllerType()
* IStorageController->SetStorageControllerType()
* IMachine->GetStorageControllers()
---
src/vbox/vbox_common.h | 13 +++++++++++++
src/vbox/vbox_tmpl.c | 20 ++++++++++++++++++++
src/vbox/vbox_uniformed_api.h | 3 +++
3 files changed, 36 insertions(+)
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index c6da8929d..b08ad1e3e 100644
--- a/src/vbox/vbox_common.h
+++ b/src/vbox/vbox_common.h
@@ -235,6 +235,19 @@ enum StorageBus
StorageBus_SAS = 5
};
+enum StorageControllerType
+{
+ StorageControllerType_Null = 0,
+ StorageControllerType_LsiLogic = 1,
+ StorageControllerType_BusLogic = 2,
+ StorageControllerType_IntelAhci = 3,
+ StorageControllerType_PIIX3 = 4,
+ StorageControllerType_PIIX4 = 5,
+ StorageControllerType_ICH6 = 6,
+ StorageControllerType_I82078 = 7,
+ StorageControllerType_LsiLogicSas = 8
+};
+
enum AccessMode
{
AccessMode_ReadOnly = 1,
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index ac3b8fa00..6592cbd63 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -550,6 +550,11 @@ static void* _handleUSBGetDeviceFilters(IUSBCommon *USBCommon)
return USBCommon->vtbl->GetDeviceFilters;
}
+static void* _handleMachineGetStorageControllers(IMachine *machine)
+{
+ return machine->vtbl->GetStorageControllers;
+}
+
static void* _handleMachineGetMediumAttachments(IMachine *machine)
{
return machine->vtbl->GetMediumAttachments;
@@ -1919,6 +1924,18 @@ _storageControllerGetBus(IStorageController *storageController,
PRUint32 *bus)
return storageController->vtbl->GetBus(storageController, bus);
}
+static nsresult
+_storageControllerGetControllerType(IStorageController *storageController, PRUint32
*controllerType)
+{
+ return storageController->vtbl->GetControllerType(storageController,
controllerType);
+}
+
+static nsresult
+_storageControllerSetControllerType(IStorageController *storageController, PRUint32
controllerType)
+{
+ return storageController->vtbl->SetControllerType(storageController,
controllerType);
+}
+
static nsresult
_sharedFolderGetHostPath(ISharedFolder *sharedFolder, PRUnichar **hostPath)
{
@@ -2268,6 +2285,7 @@ static vboxUniformedArray _UArray = {
.handleGetMachines = _handleGetMachines,
.handleGetHardDisks = _handleGetHardDisks,
.handleUSBGetDeviceFilters = _handleUSBGetDeviceFilters,
+ .handleMachineGetStorageControllers = _handleMachineGetStorageControllers,
.handleMachineGetMediumAttachments = _handleMachineGetMediumAttachments,
.handleMachineGetSharedFolders = _handleMachineGetSharedFolders,
.handleSnapshotGetChildren = _handleSnapshotGetChildren,
@@ -2499,6 +2517,8 @@ static vboxUniformedIMediumAttachment _UIMediumAttachment = {
static vboxUniformedIStorageController _UIStorageController = {
.GetBus = _storageControllerGetBus,
+ .GetControllerType = _storageControllerGetControllerType,
+ .SetControllerType = _storageControllerSetControllerType,
};
static vboxUniformedISharedFolder _UISharedFolder = {
diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
index 2ccaf43e8..dc0b391b2 100644
--- a/src/vbox/vbox_uniformed_api.h
+++ b/src/vbox/vbox_uniformed_api.h
@@ -135,6 +135,7 @@ typedef struct {
void* (*handleGetMachines)(IVirtualBox *vboxObj);
void* (*handleGetHardDisks)(IVirtualBox *vboxObj);
void* (*handleUSBGetDeviceFilters)(IUSBCommon *USBCommon);
+ void* (*handleMachineGetStorageControllers)(IMachine *machine);
void* (*handleMachineGetMediumAttachments)(IMachine *machine);
void* (*handleMachineGetSharedFolders)(IMachine *machine);
void* (*handleSnapshotGetChildren)(ISnapshot *snapshot);
@@ -410,6 +411,8 @@ typedef struct {
/* Functions for IStorageController */
typedef struct {
nsresult (*GetBus)(IStorageController *storageController, PRUint32 *bus);
+ nsresult (*SetControllerType)(IStorageController *storageController, PRUint32
controllerType);
+ nsresult (*GetControllerType)(IStorageController *storageController, PRUint32
*controllerType);
} vboxUniformedIStorageController;
/* Functions for ISharedFolder */
--
2.14.2
2:46 p.m.
New subject: [libvirt] [PATCH 4/6] vbox: Add more IStorageController API mappings.
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski(a)datto.com>
This patch 'maps' additonal methods for VBOX API to the libvirt unified
"for the VBOX API"
additional
vbox API to deal with IStorageController. The 'mapped'
methods are:
* IStorageController->GetStorageControllerType()
* IStorageController->SetStorageControllerType()
* IMachine->GetStorageControllers()
---
src/vbox/vbox_common.h | 13 +++++++++++++
src/vbox/vbox_tmpl.c | 20 ++++++++++++++++++++
src/vbox/vbox_uniformed_api.h | 3 +++
3 files changed, 36 insertions(+)
This seems reasonable.... Even though there's some style differences
with how we generally like to see libvirt code - it follows the style of
other vbox code and to me that perhaps more important.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index c6da8929d..b08ad1e3e 100644
--- a/src/vbox/vbox_common.h
+++ b/src/vbox/vbox_common.h
@@ -235,6 +235,19 @@ enum StorageBus
StorageBus_SAS = 5
};
+enum StorageControllerType
+{
+ StorageControllerType_Null = 0,
+ StorageControllerType_LsiLogic = 1,
+ StorageControllerType_BusLogic = 2,
+ StorageControllerType_IntelAhci = 3,
+ StorageControllerType_PIIX3 = 4,
+ StorageControllerType_PIIX4 = 5,
+ StorageControllerType_ICH6 = 6,
+ StorageControllerType_I82078 = 7,
+ StorageControllerType_LsiLogicSas = 8
+};
+
enum AccessMode
{
AccessMode_ReadOnly = 1,
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index ac3b8fa00..6592cbd63 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -550,6 +550,11 @@ static void* _handleUSBGetDeviceFilters(IUSBCommon *USBCommon)
return USBCommon->vtbl->GetDeviceFilters;
}
+static void* _handleMachineGetStorageControllers(IMachine *machine)
+{
+ return machine->vtbl->GetStorageControllers;
+}
+
static void* _handleMachineGetMediumAttachments(IMachine *machine)
{
return machine->vtbl->GetMediumAttachments;
@@ -1919,6 +1924,18 @@ _storageControllerGetBus(IStorageController *storageController,
PRUint32 *bus)
return storageController->vtbl->GetBus(storageController, bus);
}
+static nsresult
+_storageControllerGetControllerType(IStorageController *storageController, PRUint32
*controllerType)
+{
+ return storageController->vtbl->GetControllerType(storageController,
controllerType);
+}
+
+static nsresult
+_storageControllerSetControllerType(IStorageController *storageController, PRUint32
controllerType)
+{
+ return storageController->vtbl->SetControllerType(storageController,
controllerType);
+}
+
static nsresult
_sharedFolderGetHostPath(ISharedFolder *sharedFolder, PRUnichar **hostPath)
{
@@ -2268,6 +2285,7 @@ static vboxUniformedArray _UArray = {
.handleGetMachines = _handleGetMachines,
.handleGetHardDisks = _handleGetHardDisks,
.handleUSBGetDeviceFilters = _handleUSBGetDeviceFilters,
+ .handleMachineGetStorageControllers = _handleMachineGetStorageControllers,
.handleMachineGetMediumAttachments = _handleMachineGetMediumAttachments,
.handleMachineGetSharedFolders = _handleMachineGetSharedFolders,
.handleSnapshotGetChildren = _handleSnapshotGetChildren,
@@ -2499,6 +2517,8 @@ static vboxUniformedIMediumAttachment _UIMediumAttachment = {
static vboxUniformedIStorageController _UIStorageController = {
.GetBus = _storageControllerGetBus,
+ .GetControllerType = _storageControllerGetControllerType,
+ .SetControllerType = _storageControllerSetControllerType,
};
static vboxUniformedISharedFolder _UISharedFolder = {
diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
index 2ccaf43e8..dc0b391b2 100644
--- a/src/vbox/vbox_uniformed_api.h
+++ b/src/vbox/vbox_uniformed_api.h
@@ -135,6 +135,7 @@ typedef struct {
void* (*handleGetMachines)(IVirtualBox *vboxObj);
void* (*handleGetHardDisks)(IVirtualBox *vboxObj);
void* (*handleUSBGetDeviceFilters)(IUSBCommon *USBCommon);
+ void* (*handleMachineGetStorageControllers)(IMachine *machine);
void* (*handleMachineGetMediumAttachments)(IMachine *machine);
void* (*handleMachineGetSharedFolders)(IMachine *machine);
void* (*handleSnapshotGetChildren)(ISnapshot *snapshot);
@@ -410,6 +411,8 @@ typedef struct {
/* Functions for IStorageController */
typedef struct {
nsresult (*GetBus)(IStorageController *storageController, PRUint32 *bus);
+ nsresult (*SetControllerType)(IStorageController *storageController, PRUint32
controllerType);
+ nsresult (*GetControllerType)(IStorageController *storageController, PRUint32
*controllerType);
} vboxUniformedIStorageController;
/* Functions for ISharedFolder */
3:49 p.m.
New subject: [libvirt] [PATCH 5/6] vbox: Process controller definitions from xml.
From: Dawid Zamirski <dzamirski(a)datto.com>
Until now the vbox driver was completely ignoring <controller> element
for storage in the XML definition. This patch adds support for
interpretting <controller> element for storage devices. With this the
following other changes were made to the whole storage attachment code:
* vboxAttachDrives no longer "computes" deviceSlot and devicePort
values based on the disk device name. This was causing the driver to
ignore the values set by <address> element of the <disk> device. If
that element is omitted in XML, the values produced by default by
virDomainDiskDefAssign address should work well.
* if any part of storage attachment code fails, i.e wherever we call
virReportError, we also fail the DefineXML caller rather than ignoring
those issues and moving on.
* the DefineXML cleanup part of the code was changed to make sure that
any critical failure in device attachment code does not leave any
partially defined VM behind.
* do not require disk source for removable drives so that "empty"
drives can be created for cdrom and floppy types.
---
src/vbox/vbox_common.c | 535 ++++++++++++++++++++++++++-----------------------
src/vbox/vbox_common.h | 8 +
src/vbox/vbox_tmpl.c | 43 ++--
3 files changed, 310 insertions(+), 276 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee37164..7645b29a0 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -324,6 +324,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
StorageBus_SCSI,
&maxPortPerInst[StorageBus_SCSI]);
+ gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
+ StorageBus_SAS,
+
&maxPortPerInst[StorageBus_SAS]);
gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
StorageBus_Floppy,
&maxPortPerInst[StorageBus_Floppy]);
@@ -337,6 +340,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
StorageBus_SCSI,
&maxSlotPerPort[StorageBus_SCSI]);
+ gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
+ StorageBus_SAS,
+
&maxSlotPerPort[StorageBus_SAS]);
gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
StorageBus_Floppy,
&maxSlotPerPort[StorageBus_Floppy]);
@@ -346,68 +352,6 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
return true;
}
-/**
- * function to get the StorageBus, Port number
- * and Device number for the given devicename
- * e.g: hda has StorageBus = IDE, port = 0,
- * device = 0
- *
- * @returns true on Success, false on failure.
- * @param deviceName Input device name
- * @param aMaxPortPerInst Input array of max port per device instance
- * @param aMaxSlotPerPort Input array of max slot per device port
- * @param storageBus Input storage bus type
- * @param deviceInst Output device instance number
- * @param devicePort Output port number
- * @param deviceSlot Output slot number
- *
- */
-static bool vboxGetDeviceDetails(const char *deviceName,
- PRUint32 *aMaxPortPerInst,
- PRUint32 *aMaxSlotPerPort,
- PRUint32 storageBus,
- PRInt32 *deviceInst,
- PRInt32 *devicePort,
- PRInt32 *deviceSlot)
-{
- int total = 0;
- PRUint32 maxPortPerInst = 0;
- PRUint32 maxSlotPerPort = 0;
-
- if (!deviceName ||
- !deviceInst ||
- !devicePort ||
- !deviceSlot ||
- !aMaxPortPerInst ||
- !aMaxSlotPerPort)
- return false;
-
- if ((storageBus < StorageBus_IDE) ||
- (storageBus > StorageBus_Floppy))
- return false;
-
- total = virDiskNameToIndex(deviceName);
-
- maxPortPerInst = aMaxPortPerInst[storageBus];
- maxSlotPerPort = aMaxSlotPerPort[storageBus];
-
- if (!maxPortPerInst ||
- !maxSlotPerPort ||
- (total < 0))
- return false;
-
- *deviceInst = total / (maxPortPerInst * maxSlotPerPort);
- *devicePort = (total % (maxPortPerInst * maxSlotPerPort)) / maxSlotPerPort;
- *deviceSlot = (total % (maxPortPerInst * maxSlotPerPort)) % maxSlotPerPort;
-
- VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, "
- "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u",
- deviceName, total, storageBus, *deviceInst, *devicePort,
- *deviceSlot, maxPortPerInst, maxSlotPerPort);
-
- return true;
-}
-
/**
* function to generate the name for medium,
* for e.g: hda, sda, etc
@@ -441,7 +385,7 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
return NULL;
if ((storageBus < StorageBus_IDE) ||
- (storageBus > StorageBus_Floppy))
+ (storageBus > StorageBus_SAS))
return NULL;
maxPortPerInst = aMaxPortPerInst[storageBus];
@@ -452,8 +396,9 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
if (storageBus == StorageBus_IDE) {
prefix = "hd";
- } else if ((storageBus == StorageBus_SATA) ||
- (storageBus == StorageBus_SCSI)) {
+ } else if (storageBus == StorageBus_SATA ||
+ storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
prefix = "sd";
} else if (storageBus == StorageBus_Floppy) {
prefix = "fd";
@@ -468,6 +413,124 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
return name;
}
+static int
+vboxSetStorageController(virDomainControllerDefPtr controller,
+ vboxDriverPtr data, IMachine *machine)
+{
+ PRUnichar *controllerName = NULL;
+ PRInt32 vboxModel = StorageControllerType_Null;
+ PRInt32 vboxBusType = StorageBus_Null;
+ IStorageController *vboxController = NULL;
+ nsresult rc = 0;
+ int ret = -1;
+
+ /* libvirt controller type => vbox bus type */
+ switch (controller->type) {
+ case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &controllerName);
+ vboxBusType = StorageBus_Floppy;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &controllerName);
+ vboxBusType = StorageBus_IDE;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &controllerName);
+ vboxBusType = StorageBus_SCSI;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &controllerName);
+ vboxBusType = StorageBus_SATA;
+
+ break;
+ }
+
+ /* libvirt scsi model => vbox scsi model */
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ switch (controller->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+ vboxModel = StorageControllerType_LsiLogic;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
+ vboxModel = StorageControllerType_BusLogic;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+ /* in vbox, lsisas has a dedicated SAS bus type with no model */
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &controllerName);
+ vboxBusType = StorageBus_SAS;
+
+ break;
+ }
+ }
+
+ /* libvirt ide model => vbox ide model */
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
+ switch (controller->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3:
+ vboxModel = StorageControllerType_PIIX3;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4:
+ vboxModel = StorageControllerType_PIIX4;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6:
+ vboxModel = StorageControllerType_ICH6;
+
+ break;
+ }
+ }
+
+ rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName,
+ vboxBusType, &vboxController);
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to add storage controller "
+ "(type: %s, index=%d), rc=%08x"),
+ virDomainControllerTypeToString(controller->type),
+ controller->idx, (unsigned) rc);
+ goto cleanup;
+ }
+
+ if (vboxModel != StorageControllerType_Null) {
+ rc = gVBoxAPI.UIStorageController.SetControllerType(vboxController,
+ vboxModel);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to change storage controller model, "
+ "rc=%08x"), (unsigned) rc);
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+ cleanup:
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_RELEASE(vboxController);
+
+ return ret;
+}
+
+static int
+vboxAttachStorageControllers(virDomainDefPtr def, vboxDriverPtr data,
+ IMachine *machine)
+{
+ size_t i;
+ for (i = 0; i < def->ncontrollers; i++) {
+ if (vboxSetStorageController(def->controllers[i], data, machine) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
static virDrvOpenStatus
vboxConnectOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
@@ -1017,209 +1080,168 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data,
}
}
-static void
+static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
+ int ret = 0;
size_t i;
nsresult rc = 0;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
PRUnichar *storageCtlName = NULL;
- bool error = false;
+ virDomainDiskDefPtr disk = NULL;
- /* get the max port/slots/etc for the given storage bus */
- error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
- maxSlotPerPort);
-
- /* add a storage controller for the mediums to be attached */
- /* this needs to change when multiple controller are supported for
- * ver > 3.1 */
- {
- IStorageController *storageCtl = NULL;
- PRUnichar *sName = NULL;
-
- VBOX_UTF8_TO_UTF16("IDE Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_IDE,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
-
- VBOX_UTF8_TO_UTF16("SATA Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_SATA,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
-
- VBOX_UTF8_TO_UTF16("SCSI Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_SCSI,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
-
- VBOX_UTF8_TO_UTF16("Floppy Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_Floppy,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
- }
-
- for (i = 0; i < def->ndisks && !error; i++) {
- const char *src = virDomainDiskGetSource(def->disks[i]);
- int type = virDomainDiskGetType(def->disks[i]);
- int format = virDomainDiskGetFormat(def->disks[i]);
-
- VIR_DEBUG("disk(%zu) type: %d", i, type);
- VIR_DEBUG("disk(%zu) device: %d", i, def->disks[i]->device);
- VIR_DEBUG("disk(%zu) bus: %d", i, def->disks[i]->bus);
- VIR_DEBUG("disk(%zu) src: %s", i, src);
- VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst);
- VIR_DEBUG("disk(%zu) driverName: %s", i,
- virDomainDiskGetDriver(def->disks[i]));
- VIR_DEBUG("disk(%zu) driverType: %s", i,
- virStorageFileFormatTypeToString(format));
- VIR_DEBUG("disk(%zu) cachemode: %d", i,
def->disks[i]->cachemode);
- VIR_DEBUG("disk(%zu) readonly: %s", i,
(def->disks[i]->src->readonly
- ? "True" : "False"));
- VIR_DEBUG("disk(%zu) shared: %s", i,
(def->disks[i]->src->shared
- ? "True" : "False"));
-
- if (type == VIR_STORAGE_TYPE_FILE && src) {
- IMedium *medium = NULL;
- vboxIID mediumUUID;
- PRUnichar *mediumFileUtf16 = NULL;
- PRUint32 storageBus = StorageBus_Null;
- PRUint32 deviceType = DeviceType_Null;
- PRUint32 accessMode = AccessMode_ReadOnly;
- PRInt32 deviceInst = 0;
- PRInt32 devicePort = 0;
- PRInt32 deviceSlot = 0;
+ for (i = 0; i < def->ndisks; i++) {
+ disk = def->disks[i];
+ const char *src = virDomainDiskGetSource(disk);
+ int type = virDomainDiskGetType(disk);
- VBOX_IID_INITIALIZE(&mediumUUID);
- VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
+ if (type != VIR_STORAGE_TYPE_FILE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unsupported storage type %s, the only supported
"
+ "type is %s"),
+ virStorageTypeToString(type),
+ virStorageTypeToString(VIR_STORAGE_TYPE_FILE));
+ return -1;
+ }
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- deviceType = DeviceType_HardDisk;
- accessMode = AccessMode_ReadWrite;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
- deviceType = DeviceType_DVD;
- accessMode = AccessMode_ReadOnly;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
- deviceType = DeviceType_Floppy;
- accessMode = AccessMode_ReadWrite;
- } else {
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
- }
+ if (!src && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Missing disk source file path"));
+ return -1;
+ }
- gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
- deviceType, accessMode, &medium);
+ IMedium *medium = NULL;
+ vboxIID mediumUUID;
+ PRUnichar *mediumFileUtf16 = NULL;
+ PRUint32 deviceType = DeviceType_Null;
+ PRUint32 accessMode = AccessMode_ReadOnly;
+ PRInt32 deviceSlot = disk->info.addr.drive.bus;
+ PRInt32 devicePort = disk->info.addr.drive.unit;
+ int model = -1;
+
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ deviceType = DeviceType_HardDisk;
+ accessMode = AccessMode_ReadWrite;
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ deviceType = DeviceType_DVD;
+ accessMode = AccessMode_ReadOnly;
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ deviceType = DeviceType_Floppy;
+ accessMode = AccessMode_ReadWrite;
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unsupported disk device type %s"),
+ virDomainDiskDeviceTypeToString(disk->device));
+ ret = -1;
+ goto cleanup;
+ }
- if (!medium) {
- PRUnichar *mediumEmpty = NULL;
+ if (src) {
+ VBOX_IID_INITIALIZE(&mediumUUID);
+ VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
- VBOX_UTF8_TO_UTF16("", &mediumEmpty);
+ rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
+ deviceType, accessMode,
&medium);
- rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
- mediumFileUtf16,
- deviceType, accessMode,
- &medium);
- VBOX_UTF16_FREE(mediumEmpty);
+ /* The following is not needed for vbox 4.2+ but older versions have
+ * distinct find and open operations where former looks in vbox media
+ * registry and latter at storage location. In 4.2+, the OpenMedium call
+ * takes care of both cases internally.
+ */
+ if (!medium) {
+ rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, mediumFileUtf16,
+ deviceType, accessMode,
&medium);
}
if (!medium) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to attach the following disk/dvd/floppy
"
- "to the machine: %s, rc=%08x"),
+ _("Failed to open the following disk/dvd/floppy
"
+ "image file: %s, rc=%08x"),
src, (unsigned)rc);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ ret = -1;
+ goto cleanup;
}
rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("can't get the uuid of the file to be attached
"
+ _("Can't get the uuid of the file to be attached
"
"as harddisk/dvd/floppy: %s, rc=%08x"),
src, (unsigned)rc);
- VBOX_MEDIUM_RELEASE(medium);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ ret = -1;
+ goto cleanup;
}
+ }
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (def->disks[i]->src->readonly) {
- gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
- VIR_DEBUG("setting harddisk to immutable");
- } else if (!def->disks[i]->src->readonly) {
- gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
- VIR_DEBUG("setting harddisk type to normal");
- }
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (disk->src->readonly) {
+ gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
+ } else if (!disk->src->readonly) {
+ gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
}
+ }
- if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
- VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
- storageBus = StorageBus_IDE;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) {
- VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
- storageBus = StorageBus_SATA;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
- VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
- storageBus = StorageBus_SCSI;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) {
- VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
- storageBus = StorageBus_Floppy;
- }
+ /* asssociate <disc> bus to controller */
- /* get the device details i.e instance, port and slot */
- if (!vboxGetDeviceDetails(def->disks[i]->dst,
- maxPortPerInst,
- maxSlotPerPort,
- storageBus,
- &deviceInst,
- &devicePort,
- &deviceSlot)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("can't get the port/slot number of "
- "harddisk/dvd/floppy to be attached: "
- "%s, rc=%08x"),
- src, (unsigned)rc);
- VBOX_MEDIUM_RELEASE(medium);
- vboxIIDUnalloc(&mediumUUID);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
- }
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &storageCtlName);
- /* attach the harddisk/dvd/Floppy to the storage controller */
- rc = gVBoxAPI.UIMachine.AttachDevice(machine,
- storageCtlName,
- devicePort,
- deviceSlot,
- deviceType,
- medium);
+ break;
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not attach the file as "
- "harddisk/dvd/floppy: %s, rc=%08x"),
- src, (unsigned)rc);
- } else {
- DEBUGIID("Attached HDD/DVD/Floppy with UUID",
&mediumUUID);
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &storageCtlName);
+
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName);
+
+ model = virDomainDeviceFindControllerModel(def, &disk->info,
+ VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+
+ /* if the model is lsisas1068, set vbox bus type to SAS */
+ if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ VBOX_UTF16_FREE(storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName);
}
- VBOX_MEDIUM_RELEASE(medium);
- vboxIIDUnalloc(&mediumUUID);
- VBOX_UTF16_FREE(mediumFileUtf16);
- VBOX_UTF16_FREE(storageCtlName);
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName);
+ devicePort = 0;
+ deviceSlot = disk->info.addr.drive.unit;
+
+ break;
}
+
+ /* attach the harddisk/dvd/Floppy to the storage controller */
+ rc = gVBoxAPI.UIMachine.AttachDevice(machine,
+ storageCtlName,
+ devicePort,
+ deviceSlot,
+ deviceType,
+ medium);
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not attach the file as "
+ "harddisk/dvd/floppy: %s, rc=%08x"),
+ src, (unsigned)rc);
+ ret = -1;
+ }
+
+ cleanup:
+ vboxIIDUnalloc(&mediumUUID);
+ VBOX_MEDIUM_RELEASE(medium);
+ VBOX_UTF16_FREE(mediumFileUtf16);
+ VBOX_UTF16_FREE(storageCtlName);
+
+ if (ret < 0)
+ break;
}
+
+ return ret;
}
static void
@@ -1853,6 +1875,7 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainPtr ret = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+ int success = 0;
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -1948,7 +1971,10 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
vboxSetBootDeviceOrder(def, data, machine);
- vboxAttachDrives(def, data, machine);
+ if (vboxAttachStorageControllers(def, data, machine) < 0)
+ goto cleanup;
+ if (vboxAttachDrives(def, data, machine) < 0)
+ goto cleanup;
vboxAttachSound(def, machine);
if (vboxAttachNetwork(def, data, machine) < 0)
goto cleanup;
@@ -1959,30 +1985,40 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
vboxAttachUSB(def, data, machine);
vboxAttachSharedFolder(def, data, machine);
- /* Save the machine settings made till now and close the
- * session. also free up the mchiid variable used.
- */
+ success = 1;
+
+ cleanup:
+ /* Save the machine settings made till now, even if incomplete */
rc = gVBoxAPI.UIMachine.SaveSettings(machine);
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(rc))
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed no saving settings, rc=%08x"), (unsigned)rc);
- goto cleanup;
+ _("failed to save VM settings, rc=%08x"),
+ (unsigned) rc);
+
+ if (success) {
+ ret = virGetDomain(conn, def->name, def->uuid, -1);
+ } else {
+ /* Unregister incompletely configured VM to not leave garbage behind */
+ gVBoxAPI.UISession.Close(data->vboxSession);
+ rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
+ if (NS_SUCCEEDED(rc)) {
+ gVBoxAPI.deleteConfig(machine);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("could not cleanup partial domain after failure "
+ "to define, rc=%08x"),
+ (unsigned) rc);
+ }
}
gVBoxAPI.UISession.Close(data->vboxSession);
vboxIIDUnalloc(&mchiid);
- ret = virGetDomain(conn, def->name, def->uuid, -1);
VBOX_RELEASE(machine);
virDomainDefFree(def);
return ret;
-
- cleanup:
- VBOX_RELEASE(machine);
- virDomainDefFree(def);
- return NULL;
}
static virDomainPtr
@@ -3057,8 +3093,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
bool error = false;
int diskCount = 0;
size_t i;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
+ PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {};
+ PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {};
def->ndisks = 0;
gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
@@ -3151,7 +3187,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index b08ad1e3e..23940fc63 100644
--- a/src/vbox/vbox_common.h
+++ b/src/vbox/vbox_common.h
@@ -326,6 +326,14 @@ enum HardDiskVariant
# define VBOX_E_INVALID_SESSION_STATE 0x80BB000B
# define VBOX_E_OBJECT_IN_USE 0x80BB000C
+/* VBOX storage controller name definitions */
+# define VBOX_CONTROLLER_IDE_NAME "IDE Controller"
+# define VBOX_CONTROLLER_FLOPPY_NAME "Floppy Controller"
+# define VBOX_CONTROLLER_SATA_NAME "SATA Controller"
+# define VBOX_CONTROLLER_SCSI_NAME "SCSI Controller"
+# define VBOX_CONTROLLER_SAS_NAME "SAS Controller"
+
+
/* Simplied definitions in vbox_CAPI_*.h */
typedef void const *PCVBOXXPCOM;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 6592cbd63..b7ced62dc 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -684,7 +684,9 @@ _virtualboxCreateHardDisk(IVirtualBox *vboxObj, PRUnichar *format,
#if VBOX_API_VERSION < 5000000
return vboxObj->vtbl->CreateHardDisk(vboxObj, format, location, medium);
#elif VBOX_API_VERSION >= 5000000 /*VBOX_API_VERSION >= 5000000*/
- return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
AccessMode_ReadWrite, DeviceType_HardDisk, medium);
+ return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
+ AccessMode_ReadWrite,
+ DeviceType_HardDisk, medium);
#endif /*VBOX_API_VERSION >= 5000000*/
}
@@ -696,37 +698,28 @@ _virtualboxRegisterMachine(IVirtualBox *vboxObj, IMachine *machine)
static nsresult
_virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location,
- PRUint32 deviceType ATTRIBUTE_UNUSED,
- PRUint32 accessMode ATTRIBUTE_UNUSED,
- IMedium **medium)
+ PRUint32 deviceType,
+ PRUint32 accessMode ATTRIBUTE_UNUSED, IMedium **medium)
{
#if VBOX_API_VERSION < 4002000
- return vboxObj->vtbl->FindMedium(vboxObj, location,
- deviceType, medium);
+ return vboxObj->vtbl->FindMedium(vboxObj, location, deviceType, medium);
#else /* VBOX_API_VERSION >= 4002000 */
- return vboxObj->vtbl->OpenMedium(vboxObj, location,
- deviceType, accessMode, PR_FALSE, medium);
+ return vboxObj->vtbl->OpenMedium(vboxObj, location, deviceType, accessMode,
+ PR_FALSE, medium);
#endif /* VBOX_API_VERSION >= 4002000 */
}
static nsresult
-_virtualboxOpenMedium(IVirtualBox *vboxObj ATTRIBUTE_UNUSED,
- PRUnichar *location ATTRIBUTE_UNUSED,
- PRUint32 deviceType ATTRIBUTE_UNUSED,
- PRUint32 accessMode ATTRIBUTE_UNUSED,
- IMedium **medium ATTRIBUTE_UNUSED)
+_virtualboxOpenMedium(IVirtualBox *vboxObj, PRUnichar *location,
+ PRUint32 deviceType, PRUint32 accessMode,
+ IMedium **medium)
{
#if VBOX_API_VERSION == 4000000
- return vboxObj->vtbl->OpenMedium(vboxObj,
- location,
- deviceType, accessMode,
+ return vboxObj->vtbl->OpenMedium(vboxObj, location, deviceType, accessMode,
medium);
#elif VBOX_API_VERSION >= 4001000
- return vboxObj->vtbl->OpenMedium(vboxObj,
- location,
- deviceType, accessMode,
- false,
- medium);
+ return vboxObj->vtbl->OpenMedium(vboxObj, location, deviceType, accessMode,
+ false, medium);
#endif
}
@@ -778,12 +771,8 @@ _machineGetStorageControllerByName(IMachine *machine, PRUnichar
*name,
}
static nsresult
-_machineAttachDevice(IMachine *machine ATTRIBUTE_UNUSED,
- PRUnichar *name ATTRIBUTE_UNUSED,
- PRInt32 controllerPort ATTRIBUTE_UNUSED,
- PRInt32 device ATTRIBUTE_UNUSED,
- PRUint32 type ATTRIBUTE_UNUSED,
- IMedium * medium ATTRIBUTE_UNUSED)
+_machineAttachDevice(IMachine *machine, PRUnichar *name, PRInt32 controllerPort,
+ PRInt32 device, PRUint32 type, IMedium * medium)
{
return machine->vtbl->AttachDevice(machine, name, controllerPort,
device, type, medium);
--
2.14.2
2:46 p.m.
New subject: [libvirt] [PATCH 5/6] vbox: Process controller definitions from xml.
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski(a)datto.com>
Until now the vbox driver was completely ignoring <controller> element
ignoring the <controller>
for storage in the XML definition. This patch adds support for
interpretting <controller> element for storage devices. With this the
interpreting the <controller>
following other changes were made to the whole storage attachment
code:
* vboxAttachDrives no longer "computes" deviceSlot and devicePort
values based on the disk device name. This was causing the driver to
ignore the values set by <address> element of the <disk> device. If
that element is omitted in XML, the values produced by default by
virDomainDiskDefAssign address should work well.
* if any part of storage attachment code fails, i.e wherever we call
virReportError, we also fail the DefineXML caller rather than ignoring
those issues and moving on.
This particular code (checking vboxAttachDrives status) probably could
have been pulled out and made into it's own patch.
* the DefineXML cleanup part of the code was changed to make sure
that
any critical failure in device attachment code does not leave any
partially defined VM behind.
* do not require disk source for removable drives so that "empty"
drives can be created for cdrom and floppy types.
What's "StorageBus_SAS" and why was it not mentioned? Seems to be
something that should be separated into a patch just before this one in
order to reduce the number of changes in this patch.
---
src/vbox/vbox_common.c | 535 ++++++++++++++++++++++++++-----------------------
src/vbox/vbox_common.h | 8 +
src/vbox/vbox_tmpl.c | 43 ++--
3 files changed, 310 insertions(+), 276 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee37164..7645b29a0 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -324,6 +324,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
StorageBus_SCSI,
&maxPortPerInst[StorageBus_SCSI]);
+ gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
+ StorageBus_SAS,
+
&maxPortPerInst[StorageBus_SAS]);
gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
StorageBus_Floppy,
&maxPortPerInst[StorageBus_Floppy]);
@@ -337,6 +340,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
StorageBus_SCSI,
&maxSlotPerPort[StorageBus_SCSI]);
+ gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
+ StorageBus_SAS,
+
&maxSlotPerPort[StorageBus_SAS]);
gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
StorageBus_Floppy,
&maxSlotPerPort[StorageBus_Floppy]);
@@ -346,68 +352,6 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
return true;
}
-/**
- * function to get the StorageBus, Port number
- * and Device number for the given devicename
- * e.g: hda has StorageBus = IDE, port = 0,
- * device = 0
- *
- * @returns true on Success, false on failure.
- * @param deviceName Input device name
- * @param aMaxPortPerInst Input array of max port per device instance
- * @param aMaxSlotPerPort Input array of max slot per device port
- * @param storageBus Input storage bus type
- * @param deviceInst Output device instance number
- * @param devicePort Output port number
- * @param deviceSlot Output slot number
- *
- */
-static bool vboxGetDeviceDetails(const char *deviceName,
- PRUint32 *aMaxPortPerInst,
- PRUint32 *aMaxSlotPerPort,
- PRUint32 storageBus,
- PRInt32 *deviceInst,
- PRInt32 *devicePort,
- PRInt32 *deviceSlot)
-{
- int total = 0;
- PRUint32 maxPortPerInst = 0;
- PRUint32 maxSlotPerPort = 0;
-
- if (!deviceName ||
- !deviceInst ||
- !devicePort ||
- !deviceSlot ||
- !aMaxPortPerInst ||
- !aMaxSlotPerPort)
- return false;
-
- if ((storageBus < StorageBus_IDE) ||
- (storageBus > StorageBus_Floppy))
NB: If you make that _SAS specific patch this would probably have to
change too at least for the one patch to be "complete"
- return false;
-
- total = virDiskNameToIndex(deviceName);
-
- maxPortPerInst = aMaxPortPerInst[storageBus];
- maxSlotPerPort = aMaxSlotPerPort[storageBus];
-
- if (!maxPortPerInst ||
- !maxSlotPerPort ||
- (total < 0))
- return false;
-
- *deviceInst = total / (maxPortPerInst * maxSlotPerPort);
- *devicePort = (total % (maxPortPerInst * maxSlotPerPort)) / maxSlotPerPort;
- *deviceSlot = (total % (maxPortPerInst * maxSlotPerPort)) % maxSlotPerPort;
-
- VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, "
- "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u",
- deviceName, total, storageBus, *deviceInst, *devicePort,
- *deviceSlot, maxPortPerInst, maxSlotPerPort);
-
- return true;
-}
-
/**
* function to generate the name for medium,
* for e.g: hda, sda, etc
@@ -441,7 +385,7 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
return NULL;
if ((storageBus < StorageBus_IDE) ||
- (storageBus > StorageBus_Floppy))
+ (storageBus > StorageBus_SAS))
return NULL;
maxPortPerInst = aMaxPortPerInst[storageBus];
@@ -452,8 +396,9 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
if (storageBus == StorageBus_IDE) {
prefix = "hd";
- } else if ((storageBus == StorageBus_SATA) ||
- (storageBus == StorageBus_SCSI)) {
+ } else if (storageBus == StorageBus_SATA ||
+ storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
prefix = "sd";
} else if (storageBus == StorageBus_Floppy) {
prefix = "fd";
@@ -468,6 +413,124 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
return name;
}
+static int
+vboxSetStorageController(virDomainControllerDefPtr controller,
+ vboxDriverPtr data, IMachine *machine)
+{
+ PRUnichar *controllerName = NULL;
+ PRInt32 vboxModel = StorageControllerType_Null;
+ PRInt32 vboxBusType = StorageBus_Null;
+ IStorageController *vboxController = NULL;
+ nsresult rc = 0;
+ int ret = -1;
+
+ /* libvirt controller type => vbox bus type */
+ switch (controller->type) {
If you typecast controller->type as (virDomainControllerType)
+ case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &controllerName);
+ vboxBusType = StorageBus_Floppy;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &controllerName);
+ vboxBusType = StorageBus_IDE;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &controllerName);
+ vboxBusType = StorageBus_SCSI;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &controllerName);
+ vboxBusType = StorageBus_SATA;
+
+ break;
Then here you have cases for when @controller is VIRTIO_SERIAL, CCID,
USB, PCI, or LAST. I would think for types other than LAST, you could
just return because they're not storage controllers (avoids having a
NULL controllerName later on). For LAST one would hope it's not
provided, but it's your call whether that's an error or ignore type case.
Doing this avoids the chance that someone adds a new controller and
doesn't address the vbox code.
+ }
+
+ /* libvirt scsi model => vbox scsi model */
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ switch (controller->model) {
similarly here, except use (virDomainControllerModelSCSI)
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+ vboxModel = StorageControllerType_LsiLogic;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
+ vboxModel = StorageControllerType_BusLogic;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+ /* in vbox, lsisas has a dedicated SAS bus type with no model */
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &controllerName);
+ vboxBusType = StorageBus_SAS;
+
+ break;
+ }
and here would be the I don't know/support that SCSI controller model
type logic or we don't need to set the vboxModel value.
As for LAST, that's probably an error - shouldn't happen, but the
compiler will complain if you don't add it to the switch() case's.
Again, doing this ensures someone doesn't add a new SCSI model type and
doesn't address the vbox code.
+ }
+
+ /* libvirt ide model => vbox ide model */
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
I'd use
} else if {
But this is OK
+ switch (controller->model) {
typecast to (virDomainControllerModelIDE)
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3:
+ vboxModel = StorageControllerType_PIIX3;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4:
+ vboxModel = StorageControllerType_PIIX4;
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6:
+ vboxModel = StorageControllerType_ICH6;
+
+ break;
+ }
Again logic for _LAST and again, this ensures that no one adds to the
enum without addressing logic here.
+ }
+
+ rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName,
+ vboxBusType, &vboxController);
BTW: Without adding those switches above this fails perhaps randomly? I
don't know what happens when one makes this call with a NULL
controllerName and/or a vboxBusType == StorageBus_Null
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to add storage controller "
+ "(type: %s, index=%d), rc=%08x"),
+ virDomainControllerTypeToString(controller->type),
+ controller->idx, (unsigned) rc);
+ goto cleanup;
+ }
+
+ if (vboxModel != StorageControllerType_Null) {
So this is only necessary for SCSI and IDE of certain types? Not a
problem, mostly curious.
+ rc =
gVBoxAPI.UIStorageController.SetControllerType(vboxController,
+ vboxModel);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to change storage controller model, "
+ "rc=%08x"), (unsigned) rc);
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+ cleanup:
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_RELEASE(vboxController);
+
+ return ret;
+}
+
+static int
+vboxAttachStorageControllers(virDomainDefPtr def, vboxDriverPtr data,
+ IMachine *machine)
+{
+ size_t i;
+ for (i = 0; i < def->ncontrollers; i++) {
You do realize the domain controller list has more than just storage
controllers... But I address that earlier by returning 0 if nothing
needs to be done... Unless you think something should be done for other c
+ if (vboxSetStorageController(def->controllers[i], data,
machine) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
FWIW: More recently in libvirt we prefer new functions added with 2
blank lines between each.
static virDrvOpenStatus
vboxConnectOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
@@ -1017,209 +1080,168 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr
data,
}
}
-static void
+static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
+ int ret = 0;
size_t i;
nsresult rc = 0;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
PRUnichar *storageCtlName = NULL;
- bool error = false;
+ virDomainDiskDefPtr disk = NULL;
- /* get the max port/slots/etc for the given storage bus */
- error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
- maxSlotPerPort);
-
- /* add a storage controller for the mediums to be attached */
- /* this needs to change when multiple controller are supported for
- * ver > 3.1 */
- {
- IStorageController *storageCtl = NULL;
- PRUnichar *sName = NULL;
-
- VBOX_UTF8_TO_UTF16("IDE Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_IDE,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
-
- VBOX_UTF8_TO_UTF16("SATA Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_SATA,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
-
- VBOX_UTF8_TO_UTF16("SCSI Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_SCSI,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
-
- VBOX_UTF8_TO_UTF16("Floppy Controller", &sName);
- gVBoxAPI.UIMachine.AddStorageController(machine,
- sName,
- StorageBus_Floppy,
- &storageCtl);
- VBOX_UTF16_FREE(sName);
- VBOX_RELEASE(storageCtl);
- }
-
- for (i = 0; i < def->ndisks && !error; i++) {
- const char *src = virDomainDiskGetSource(def->disks[i]);
- int type = virDomainDiskGetType(def->disks[i]);
- int format = virDomainDiskGetFormat(def->disks[i]);
-
- VIR_DEBUG("disk(%zu) type: %d", i, type);
- VIR_DEBUG("disk(%zu) device: %d", i,
def->disks[i]->device);
- VIR_DEBUG("disk(%zu) bus: %d", i, def->disks[i]->bus);
- VIR_DEBUG("disk(%zu) src: %s", i, src);
- VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst);
- VIR_DEBUG("disk(%zu) driverName: %s", i,
- virDomainDiskGetDriver(def->disks[i]));
- VIR_DEBUG("disk(%zu) driverType: %s", i,
- virStorageFileFormatTypeToString(format));
- VIR_DEBUG("disk(%zu) cachemode: %d", i,
def->disks[i]->cachemode);
- VIR_DEBUG("disk(%zu) readonly: %s", i,
(def->disks[i]->src->readonly
- ? "True" : "False"));
- VIR_DEBUG("disk(%zu) shared: %s", i,
(def->disks[i]->src->shared
- ? "True" : "False"));
Lots of potentially use DEBUG information being lost. IDC, but seems a
shame to not have it. Although it can be overkill when DEBUG is enabled
- it does help sometimes in debugging problems. Your call...
-
- if (type == VIR_STORAGE_TYPE_FILE && src) {
- IMedium *medium = NULL;
- vboxIID mediumUUID;
- PRUnichar *mediumFileUtf16 = NULL;
- PRUint32 storageBus = StorageBus_Null;
- PRUint32 deviceType = DeviceType_Null;
- PRUint32 accessMode = AccessMode_ReadOnly;
- PRInt32 deviceInst = 0;
- PRInt32 devicePort = 0;
- PRInt32 deviceSlot = 0;
+ for (i = 0; i < def->ndisks; i++) {
+ disk = def->disks[i];
+ const char *src = virDomainDiskGetSource(disk);
+ int type = virDomainDiskGetType(disk);
- VBOX_IID_INITIALIZE(&mediumUUID);
- VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
+ if (type != VIR_STORAGE_TYPE_FILE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unsupported storage type %s, the only supported
"
+ "type is %s"),
+ virStorageTypeToString(type),
+ virStorageTypeToString(VIR_STORAGE_TYPE_FILE));
+ return -1;
+ }
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- deviceType = DeviceType_HardDisk;
- accessMode = AccessMode_ReadWrite;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
- deviceType = DeviceType_DVD;
- accessMode = AccessMode_ReadOnly;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
- deviceType = DeviceType_Floppy;
- accessMode = AccessMode_ReadWrite;
- } else {
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
- }
+ if (!src && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
I think more typically there are checks against != *_CDROM || *_FLOPPY
although perhaps this check should go slightly later [1]
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
+ _("Missing disk source file path"));
+ return -1;
+ }
- gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
- deviceType, accessMode, &medium);
+ IMedium *medium = NULL;
+ vboxIID mediumUUID;
Since you call vboxIIDUnalloc later on this [2], perhaps this should be
initialized to NULL... Although, I see it only gets set when "if (src)",
so perhaps that becomes the key for Unalloc too.
+ PRUnichar *mediumFileUtf16 = NULL;
+ PRUint32 deviceType = DeviceType_Null;
+ PRUint32 accessMode = AccessMode_ReadOnly;
+ PRInt32 deviceSlot = disk->info.addr.drive.bus;
+ PRInt32 devicePort = disk->info.addr.drive.unit;
+ int model = -1;
We really prefer to see definitions grouped together at the top rather
than in the middle of code
+
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
[1] If you move that !src check here, then you're accomplishing the
same result as previously but making it more obvious (at least for me)
since _LUN isn't supported...
+ deviceType = DeviceType_HardDisk;
+ accessMode = AccessMode_ReadWrite;
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ deviceType = DeviceType_DVD;
+ accessMode = AccessMode_ReadOnly;
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ deviceType = DeviceType_Floppy;
+ accessMode = AccessMode_ReadWrite;
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unsupported disk device type %s"),
+ virDomainDiskDeviceTypeToString(disk->device));
+ ret = -1;
+ goto cleanup;
+ }
Also if you change this to a :
switch ((virDomainDiskDevice) disk->device) {
case VIR_DOMAIN_DISK_DEVICE_DISK:
...
case VIR_DOMAIN_DISK_DEVICE_LUN:
VIR_DOMAIN_DISK_DEVICE_LAST:
virReportError()...
...
}
Then you avoid the chance that someone adds a new virDomainDiskDevice
and doesn't address that in the vbox code.
- if (!medium) {
- PRUnichar *mediumEmpty = NULL;
+ if (src) {
+ VBOX_IID_INITIALIZE(&mediumUUID);
+ VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
- VBOX_UTF8_TO_UTF16("", &mediumEmpty);
+ rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
+ deviceType, accessMode,
&medium);
- rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
- mediumFileUtf16,
- deviceType, accessMode,
- &medium);
- VBOX_UTF16_FREE(mediumEmpty);
+ /* The following is not needed for vbox 4.2+ but older versions have
+ * distinct find and open operations where former looks in vbox media
+ * registry and latter at storage location. In 4.2+, the OpenMedium call
+ * takes care of both cases internally.
+ */
+ if (!medium) {
+ rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
mediumFileUtf16,
+ deviceType, accessMode,
&medium);
}
if (!medium) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to attach the following disk/dvd/floppy
"
- "to the machine: %s, rc=%08x"),
+ _("Failed to open the following disk/dvd/floppy
"
+ "image file: %s, rc=%08x"),
src, (unsigned)rc);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ ret = -1;
+ goto cleanup;
}
rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("can't get the uuid of the file to be attached
"
+ _("Can't get the uuid of the file to be attached
"
"as harddisk/dvd/floppy: %s, rc=%08x"),
src, (unsigned)rc);
- VBOX_MEDIUM_RELEASE(medium);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ ret = -1;
+ goto cleanup;
}
+ }
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (def->disks[i]->src->readonly) {
- gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
- VIR_DEBUG("setting harddisk to immutable");
- } else if (!def->disks[i]->src->readonly) {
- gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
- VIR_DEBUG("setting harddisk type to normal");
- }
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (disk->src->readonly) {
+ gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
+ } else if (!disk->src->readonly) {
+ gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
}
+ }
- if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
- VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
- storageBus = StorageBus_IDE;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) {
- VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
- storageBus = StorageBus_SATA;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
- VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
- storageBus = StorageBus_SCSI;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) {
- VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
- storageBus = StorageBus_Floppy;
- }
+ /* asssociate <disc> bus to controller */
<disk>
- /* get the device details i.e instance, port and slot */
- if (!vboxGetDeviceDetails(def->disks[i]->dst,
- maxPortPerInst,
- maxSlotPerPort,
- storageBus,
- &deviceInst,
- &devicePort,
- &deviceSlot)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("can't get the port/slot number of "
- "harddisk/dvd/floppy to be attached: "
- "%s, rc=%08x"),
- src, (unsigned)rc);
- VBOX_MEDIUM_RELEASE(medium);
- vboxIIDUnalloc(&mediumUUID);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
- }
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &storageCtlName);
- /* attach the harddisk/dvd/Floppy to the storage controller */
- rc = gVBoxAPI.UIMachine.AttachDevice(machine,
- storageCtlName,
- devicePort,
- deviceSlot,
- deviceType,
- medium);
+ break;
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not attach the file as "
- "harddisk/dvd/floppy: %s, rc=%08x"),
- src, (unsigned)rc);
- } else {
- DEBUGIID("Attached HDD/DVD/Floppy with UUID",
&mediumUUID);
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &storageCtlName);
+
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName);
+
+ model = virDomainDeviceFindControllerModel(def, &disk->info,
+
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+
+ /* if the model is lsisas1068, set vbox bus type to SAS */
+ if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ VBOX_UTF16_FREE(storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName);
}
- VBOX_MEDIUM_RELEASE(medium);
- vboxIIDUnalloc(&mediumUUID);
- VBOX_UTF16_FREE(mediumFileUtf16);
- VBOX_UTF16_FREE(storageCtlName);
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName);
+ devicePort = 0;
+ deviceSlot = disk->info.addr.drive.unit;
+
+ break;
}
+
+ /* attach the harddisk/dvd/Floppy to the storage controller */
+ rc = gVBoxAPI.UIMachine.AttachDevice(machine,
+ storageCtlName,
+ devicePort,
+ deviceSlot,
+ deviceType,
+ medium);
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not attach the file as "
+ "harddisk/dvd/floppy: %s, rc=%08x"),
+ src, (unsigned)rc);
+ ret = -1;
+ }
you could keep the "} else { DEBUGIID("Attached HDD/DVD/Floppy with
UUID", &mediumUUID); | " - your call
+
+ cleanup:
+ vboxIIDUnalloc(&mediumUUID);
[2] This only gets set on "if (src)", so you probably need that
condition here too - just to be safe...
+ VBOX_MEDIUM_RELEASE(medium);
+ VBOX_UTF16_FREE(mediumFileUtf16);
+ VBOX_UTF16_FREE(storageCtlName);
+
+ if (ret < 0)
+ break;
Not a normal way to have cleanup: label inside a for loop, but it does
avoid having duplicated logic...
}
+
+ return ret;
}
static void
@@ -1853,6 +1875,7 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainPtr ret = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+ int success = 0;
So this is more of a "bool machineReady = false"
I don't like @success, but naming is hard and whatever you pick will
cause someone else to say - why not use something else...
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -1948,7 +1971,10 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
vboxSetBootDeviceOrder(def, data, machine);
- vboxAttachDrives(def, data, machine);
+ if (vboxAttachStorageControllers(def, data, machine) < 0)
+ goto cleanup;
+ if (vboxAttachDrives(def, data, machine) < 0)
+ goto cleanup;
vboxAttachSound(def, machine);
if (vboxAttachNetwork(def, data, machine) < 0)
goto cleanup;
@@ -1959,30 +1985,40 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
vboxAttachUSB(def, data, machine);
vboxAttachSharedFolder(def, data, machine);
- /* Save the machine settings made till now and close the
- * session. also free up the mchiid variable used.
- */
+ success = 1;
+
+ cleanup:
+ /* Save the machine settings made till now, even if incomplete */
rc = gVBoxAPI.UIMachine.SaveSettings(machine);
- if (NS_FAILED(rc)) {
+ if (NS_FAILED(rc))
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed no saving settings, rc=%08x"),
(unsigned)rc);
- goto cleanup;
+ _("failed to save VM settings, rc=%08x"),
+ (unsigned) rc);
So if you fail to save the settings, then unlike previously you're going
to call virGetDomain() which doesn't seem right.
Is the (unsigned) typecast really necessary on a 08x ?
+
+ if (success) {
+ ret = virGetDomain(conn, def->name, def->uuid, -1);
+ } else {
+ /* Unregister incompletely configured VM to not leave garbage behind */
+ gVBoxAPI.UISession.Close(data->vboxSession);> + rc =
gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
+ if (NS_SUCCEEDED(rc)) {
+ gVBoxAPI.deleteConfig(machine);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("could not cleanup partial domain after failure
"
+ "to define, rc=%08x"),
+ (unsigned) rc);
+ }
}
gVBoxAPI.UISession.Close(data->vboxSession);
Prior logic allowed gVBoxAPI.UISession.Close(data->vboxSession); prior
to virGetDomain(conn, def->name, def->uuid, -1); - keeping this here
would mean in the } else { path from above we'd call this twice.
Is that right? Can/Should it move before the "if (success) {" above?
vboxIIDUnalloc(&mchiid);
- ret = virGetDomain(conn, def->name, def->uuid, -1);
VBOX_RELEASE(machine);
virDomainDefFree(def);
return ret;
-
- cleanup:
- VBOX_RELEASE(machine);
- virDomainDefFree(def);
- return NULL;
}
static virDomainPtr
@@ -3057,8 +3093,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
bool error = false;
int diskCount = 0;
size_t i;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
+ PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {};
+ PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {};
def->ndisks = 0;
gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
@@ -3151,7 +3187,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index b08ad1e3e..23940fc63 100644
--- a/src/vbox/vbox_common.h
+++ b/src/vbox/vbox_common.h
@@ -326,6 +326,14 @@ enum HardDiskVariant
# define VBOX_E_INVALID_SESSION_STATE 0x80BB000B
# define VBOX_E_OBJECT_IN_USE 0x80BB000C
+/* VBOX storage controller name definitions */
+# define VBOX_CONTROLLER_IDE_NAME "IDE Controller"
+# define VBOX_CONTROLLER_FLOPPY_NAME "Floppy Controller"
+# define VBOX_CONTROLLER_SATA_NAME "SATA Controller"
+# define VBOX_CONTROLLER_SCSI_NAME "SCSI Controller"
+# define VBOX_CONTROLLER_SAS_NAME "SAS Controller"
+
+
/* Simplied definitions in vbox_CAPI_*.h */
typedef void const *PCVBOXXPCOM;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 6592cbd63..b7ced62dc 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
I think this should be a separate patch prior to this one. You're
adjusting format and removing the ATTRIBUTE_UNUSED for arguments that
are actually used.
@@ -684,7 +684,9 @@ _virtualboxCreateHardDisk(IVirtualBox *vboxObj,
PRUnichar *format,
#if VBOX_API_VERSION < 5000000
return vboxObj->vtbl->CreateHardDisk(vboxObj, format, location, medium);
#elif VBOX_API_VERSION >= 5000000 /*VBOX_API_VERSION >= 5000000*/
- return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
AccessMode_ReadWrite, DeviceType_HardDisk, medium);
+ return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
+ AccessMode_ReadWrite,
+ DeviceType_HardDisk, medium);
#endif /*VBOX_API_VERSION >= 5000000*/
}
@@ -696,37 +698,28 @@ _virtualboxRegisterMachine(IVirtualBox *vboxObj, IMachine
*machine)
static nsresult
_virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location,
- PRUint32 deviceType ATTRIBUTE_UNUSED,
- PRUint32 accessMode ATTRIBUTE_UNUSED,
- IMedium **medium)
+ PRUint32 deviceType,
+ PRUint32 accessMode ATTRIBUTE_UNUSED, IMedium **medium)
Keep IMedium **medium on it's own line.
It's the preferred way at least for libvirt code on function args.
{
#if VBOX_API_VERSION < 4002000
- return vboxObj->vtbl->FindMedium(vboxObj, location,
- deviceType, medium);
+ return vboxObj->vtbl->FindMedium(vboxObj, location, deviceType, medium);
#else /* VBOX_API_VERSION >= 4002000 */
- return vboxObj->vtbl->OpenMedium(vboxObj, location,
- deviceType, accessMode, PR_FALSE, medium);
+ return vboxObj->vtbl->OpenMedium(vboxObj, location, deviceType, accessMode,
+ PR_FALSE, medium);
#endif /* VBOX_API_VERSION >= 4002000 */
}
static nsresult
-_virtualboxOpenMedium(IVirtualBox *vboxObj ATTRIBUTE_UNUSED,
- PRUnichar *location ATTRIBUTE_UNUSED,
- PRUint32 deviceType ATTRIBUTE_UNUSED,
- PRUint32 accessMode ATTRIBUTE_UNUSED,
- IMedium **medium ATTRIBUTE_UNUSED)
+_virtualboxOpenMedium(IVirtualBox *vboxObj, PRUnichar *location,
+ PRUint32 deviceType, PRUint32 accessMode,
+ IMedium **medium)
For this one - keep each argument on it's own line and just remove the
ATTRIBUTE_UNUSED
{
#if VBOX_API_VERSION == 4000000
- return vboxObj->vtbl->OpenMedium(vboxObj,
- location,
- deviceType, accessMode,
+ return vboxObj->vtbl->OpenMedium(vboxObj, location, deviceType, accessMode,
medium);
#elif VBOX_API_VERSION >= 4001000
- return vboxObj->vtbl->OpenMedium(vboxObj,
- location,
- deviceType, accessMode,
- false,
- medium);
+ return vboxObj->vtbl->OpenMedium(vboxObj, location, deviceType, accessMode,
+ false, medium);
#endif
}
@@ -778,12 +771,8 @@ _machineGetStorageControllerByName(IMachine *machine, PRUnichar
*name,
}
static nsresult
-_machineAttachDevice(IMachine *machine ATTRIBUTE_UNUSED,
- PRUnichar *name ATTRIBUTE_UNUSED,
- PRInt32 controllerPort ATTRIBUTE_UNUSED,
- PRInt32 device ATTRIBUTE_UNUSED,
- PRUint32 type ATTRIBUTE_UNUSED,
- IMedium * medium ATTRIBUTE_UNUSED)
+_machineAttachDevice(IMachine *machine, PRUnichar *name, PRInt32 controllerPort,
+ PRInt32 device, PRUint32 type, IMedium * medium)
Same - one line per argument is preferred
John
{
return machine->vtbl->AttachDevice(machine, name, controllerPort,
device, type, medium);
1:50 p.m.
New subject: [libvirt] [PATCH 5/6] vbox: Process controller definitions from xml.
On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski <dzamirski(a)datto.com>
>
Lots of potentially use DEBUG information being lost. IDC, but seems
a
shame to not have it. Although it can be overkill when DEBUG is
enabled
- it does help sometimes in debugging problems. Your call...
I've removed those because they basically print out the values of the
XML I just passed in and I know them from XML already. I've added a
more useful (at least for me) debug statement in (WIP) V2 that prints
the arguments passed to VBOX's AttachDevice method - those are
'computed' based on XML input by this very code so it's more helpful to
know what those final values are when debugging.
> -
> - if (type == VIR_STORAGE_TYPE_FILE && src) {
> - IMedium *medium = NULL;
> - vboxIID mediumUUID;
> - PRUnichar *mediumFileUtf16 = NULL;
> - PRUint32 storageBus = StorageBus_Null;
> - PRUint32 deviceType = DeviceType_Null;
> - PRUint32 accessMode = AccessMode_ReadOnly;
> - PRInt32 deviceInst = 0;
> - PRInt32 devicePort = 0;
> - PRInt32 deviceSlot = 0;
> + for (i = 0; i < def->ndisks; i++) {
> + disk = def->disks[i];
> + const char *src = virDomainDiskGetSource(disk);
> + int type = virDomainDiskGetType(disk);
>
> - VBOX_IID_INITIALIZE(&mediumUUID);
> - VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
> + if (type != VIR_STORAGE_TYPE_FILE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unsupported storage type %s, the
> only supported "
> + "type is %s"),
> + virStorageTypeToString(type),
> + virStorageTypeToString(VIR_STORAGE_TYPE
> _FILE));
> + return -1;
> + }
>
> - if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_DISK) {
> - deviceType = DeviceType_HardDisk;
> - accessMode = AccessMode_ReadWrite;
> - } else if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_CDROM) {
> - deviceType = DeviceType_DVD;
> - accessMode = AccessMode_ReadOnly;
> - } else if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> - deviceType = DeviceType_Floppy;
> - accessMode = AccessMode_ReadWrite;
> - } else {
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - continue;
> - }
> + if (!src && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
I think more typically there are checks against != *_CDROM ||
*_FLOPPY
although perhaps this check should go slightly later [1]
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Missing disk source file path"));
> + return -1;
> + }
>
> - gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj,
> mediumFileUtf16,
> - deviceType,
> accessMode, &medium);
> + IMedium *medium = NULL;
> + vboxIID mediumUUID;
Since you call vboxIIDUnalloc later on this [2], perhaps this should
be
initialized to NULL... Although, I see it only gets set when "if
(src)",
so perhaps that becomes the key for Unalloc too.
> + PRUnichar *mediumFileUtf16 = NULL;
> + PRUint32 deviceType = DeviceType_Null;
> + PRUint32 accessMode = AccessMode_ReadOnly;
> + PRInt32 deviceSlot = disk->info.addr.drive.bus;
> + PRInt32 devicePort = disk->info.addr.drive.unit;
> + int model = -1;
We really prefer to see definitions grouped together at the top
rather
than in the middle of code
> +
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
[1] If you move that !src check here, then you're accomplishing the
same result as previously but making it more obvious (at least for
me)
since _LUN isn't supported...
> + deviceType = DeviceType_HardDisk;
> + accessMode = AccessMode_ReadWrite;
> + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> + deviceType = DeviceType_DVD;
> + accessMode = AccessMode_ReadOnly;
> + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
> {
> + deviceType = DeviceType_Floppy;
> + accessMode = AccessMode_ReadWrite;
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unsupported disk device type %s"),
> + virDomainDiskDeviceTypeToString(disk-
> >device));
> + ret = -1;
> + goto cleanup;
> + }
Also if you change this to a :
switch ((virDomainDiskDevice) disk->device) {
case VIR_DOMAIN_DISK_DEVICE_DISK:
...
case VIR_DOMAIN_DISK_DEVICE_LUN:
VIR_DOMAIN_DISK_DEVICE_LAST:
virReportError()...
...
}
Then you avoid the chance that someone adds a new virDomainDiskDevice
and doesn't address that in the vbox code.
>
> - if (!medium) {
> - PRUnichar *mediumEmpty = NULL;
> + if (src) {
> + VBOX_IID_INITIALIZE(&mediumUUID);
> + VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
>
> - VBOX_UTF8_TO_UTF16("", &mediumEmpty);
> + rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj,
> mediumFileUtf16,
> + deviceType,
> accessMode, &medium);
>
> - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data-
> >vboxObj,
> - mediumFileUt
> f16,
> - deviceType,
> accessMode,
> - &medium);
> - VBOX_UTF16_FREE(mediumEmpty);
> + /* The following is not needed for vbox 4.2+ but older
> versions have
> + * distinct find and open operations where former
> looks in vbox media
> + * registry and latter at storage location. In 4.2+,
> the OpenMedium call
> + * takes care of both cases internally.
> + */
> + if (!medium) {
> + rc = gVBoxAPI.UIVirtualBox.OpenMedium(data-
> >vboxObj, mediumFileUtf16,
> + deviceType,
> accessMode, &medium);
> }
>
> if (!medium) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to attach the following
> disk/dvd/floppy "
> - "to the machine: %s, rc=%08x"),
> + _("Failed to open the following
> disk/dvd/floppy "
> + "image file: %s, rc=%08x"),
> src, (unsigned)rc);
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - continue;
> + ret = -1;
> + goto cleanup;
> }
>
> rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
> if (NS_FAILED(rc)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("can't get the uuid of the file
> to be attached "
> + _("Can't get the uuid of the file
> to be attached "
> "as harddisk/dvd/floppy: %s,
> rc=%08x"),
> src, (unsigned)rc);
> - VBOX_MEDIUM_RELEASE(medium);
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - continue;
> + ret = -1;
> + goto cleanup;
> }
> + }
>
> - if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_DISK) {
> - if (def->disks[i]->src->readonly) {
> - gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Immutable);
> - VIR_DEBUG("setting harddisk to immutable");
> - } else if (!def->disks[i]->src->readonly) {
> - gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Normal);
> - VIR_DEBUG("setting harddisk type to normal");
> - }
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> + if (disk->src->readonly) {
> + gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Immutable);
> + } else if (!disk->src->readonly) {
> + gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Normal);
> }
> + }
>
> - if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> - VBOX_UTF8_TO_UTF16("IDE Controller",
> &storageCtlName);
> - storageBus = StorageBus_IDE;
> - } else if (def->disks[i]->bus ==
> VIR_DOMAIN_DISK_BUS_SATA) {
> - VBOX_UTF8_TO_UTF16("SATA Controller",
> &storageCtlName);
> - storageBus = StorageBus_SATA;
> - } else if (def->disks[i]->bus ==
> VIR_DOMAIN_DISK_BUS_SCSI) {
> - VBOX_UTF8_TO_UTF16("SCSI Controller",
> &storageCtlName);
> - storageBus = StorageBus_SCSI;
> - } else if (def->disks[i]->bus ==
> VIR_DOMAIN_DISK_BUS_FDC) {
> - VBOX_UTF8_TO_UTF16("Floppy Controller",
> &storageCtlName);
> - storageBus = StorageBus_Floppy;
> - }
> + /* asssociate <disc> bus to controller */
<disk>
>
> - /* get the device details i.e instance, port and slot
> */
> - if (!vboxGetDeviceDetails(def->disks[i]->dst,
> - maxPortPerInst,
> - maxSlotPerPort,
> - storageBus,
> - &deviceInst,
> - &devicePort,
> - &deviceSlot)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("can't get the port/slot number
> of "
> - "harddisk/dvd/floppy to be
> attached: "
> - "%s, rc=%08x"),
> - src, (unsigned)rc);
> - VBOX_MEDIUM_RELEASE(medium);
> - vboxIIDUnalloc(&mediumUUID);
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - continue;
> - }
> + switch (disk->bus) {
> + case VIR_DOMAIN_DISK_BUS_IDE:
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> &storageCtlName);
>
> - /* attach the harddisk/dvd/Floppy to the storage
> controller */
> - rc = gVBoxAPI.UIMachine.AttachDevice(machine,
> - storageCtlName,
> - devicePort,
> - deviceSlot,
> - deviceType,
> - medium);
> + break;
>
> - if (NS_FAILED(rc)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("could not attach the file as "
> - "harddisk/dvd/floppy: %s,
> rc=%08x"),
> - src, (unsigned)rc);
> - } else {
> - DEBUGIID("Attached HDD/DVD/Floppy with UUID",
> &mediumUUID);
> + case VIR_DOMAIN_DISK_BUS_SATA:
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> &storageCtlName);
> +
> + break;
> +
> + case VIR_DOMAIN_DISK_BUS_SCSI:
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> &storageCtlName);
> +
> + model = virDomainDeviceFindControllerModel(def, &disk-
> >info,
> + VIR_DOMAIN_
> CONTROLLER_TYPE_SCSI);
> +
> + /* if the model is lsisas1068, set vbox bus type to
> SAS */
> + if (model ==
> VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> + VBOX_UTF16_FREE(storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> &storageCtlName);
> }
>
> - VBOX_MEDIUM_RELEASE(medium);
> - vboxIIDUnalloc(&mediumUUID);
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - VBOX_UTF16_FREE(storageCtlName);
> + break;
> +
> + case VIR_DOMAIN_DISK_BUS_FDC:
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> &storageCtlName);
> + devicePort = 0;
> + deviceSlot = disk->info.addr.drive.unit;
> +
> + break;
> }
> +
> + /* attach the harddisk/dvd/Floppy to the storage
> controller */
> + rc = gVBoxAPI.UIMachine.AttachDevice(machine,
> + storageCtlName,
> + devicePort,
> + deviceSlot,
> + deviceType,
> + medium);
> +
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not attach the file as "
> + "harddisk/dvd/floppy: %s, rc=%08x"),
> + src, (unsigned)rc);
> + ret = -1;
> + }
you could keep the "} else { DEBUGIID("Attached HDD/DVD/Floppy with
UUID", &mediumUUID); | " - your call
> +
> + cleanup:
> + vboxIIDUnalloc(&mediumUUID);
[2] This only gets set on "if (src)", so you probably need that
condition here too - just to be safe...
> + VBOX_MEDIUM_RELEASE(medium);
> + VBOX_UTF16_FREE(mediumFileUtf16);
> + VBOX_UTF16_FREE(storageCtlName);
> +
> + if (ret < 0)
> + break;
Not a normal way to have cleanup: label inside a for loop, but it
does
avoid having duplicated logic...
> }
> +
> + return ret;
> }
>
> static void
> @@ -1853,6 +1875,7 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virDomainPtr ret = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> + int success = 0;
So this is more of a "bool machineReady = false"
I don't like @success, but naming is hard and whatever you pick will
cause someone else to say - why not use something else...
>
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>
> @@ -1948,7 +1971,10 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
> gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
>
> vboxSetBootDeviceOrder(def, data, machine);
> - vboxAttachDrives(def, data, machine);
> + if (vboxAttachStorageControllers(def, data, machine) < 0)
> + goto cleanup;
> + if (vboxAttachDrives(def, data, machine) < 0)
> + goto cleanup;
> vboxAttachSound(def, machine);
> if (vboxAttachNetwork(def, data, machine) < 0)
> goto cleanup;
> @@ -1959,30 +1985,40 @@ vboxDomainDefineXMLFlags(virConnectPtr
> conn, const char *xml, unsigned int flags
> vboxAttachUSB(def, data, machine);
> vboxAttachSharedFolder(def, data, machine);
>
> - /* Save the machine settings made till now and close the
> - * session. also free up the mchiid variable used.
> - */
> + success = 1;
> +
> + cleanup:
> + /* Save the machine settings made till now, even if incomplete
> */
> rc = gVBoxAPI.UIMachine.SaveSettings(machine);
> - if (NS_FAILED(rc)) {
> + if (NS_FAILED(rc))
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed no saving settings, rc=%08x"),
> (unsigned)rc);
> - goto cleanup;
> + _("failed to save VM settings, rc=%08x"),
> + (unsigned) rc);
So if you fail to save the settings, then unlike previously you're
going
to call virGetDomain() which doesn't seem right.
Is the (unsigned) typecast really necessary on a 08x ?
> +
> + if (success) {
> + ret = virGetDomain(conn, def->name, def->uuid, -1);
> + } else {
> + /* Unregister incompletely configured VM to not leave
> garbage behind */
> + gVBoxAPI.UISession.Close(data->vboxSession);> + rc
> = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
> + if (NS_SUCCEEDED(rc)) {
> + gVBoxAPI.deleteConfig(machine);
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not cleanup partial domain
> after failure "
> + "to define, rc=%08x"),
> + (unsigned) rc);
> + }
> }
>
> gVBoxAPI.UISession.Close(data->vboxSession);
Prior logic allowed gVBoxAPI.UISession.Close(data->vboxSession);
prior
to virGetDomain(conn, def->name, def->uuid, -1); - keeping this here
would mean in the } else { path from above we'd call this twice.
Is that right? Can/Should it move before the "if (success) {" above?
> vboxIIDUnalloc(&mchiid);
>
> - ret = virGetDomain(conn, def->name, def->uuid, -1);
> VBOX_RELEASE(machine);
>
> virDomainDefFree(def);
>
> return ret;
> -
> - cleanup:
> - VBOX_RELEASE(machine);
> - virDomainDefFree(def);
> - return NULL;
> }
>
> static virDomainPtr
> @@ -3057,8 +3093,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> bool error = false;
> int diskCount = 0;
> size_t i;
> - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> + PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {};
> + PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {};
>
> def->ndisks = 0;
> gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
> @@ -3151,7 +3187,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
> } else if (storageBus == StorageBus_SATA) {
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
> - } else if (storageBus == StorageBus_SCSI) {
> + } else if (storageBus == StorageBus_SCSI ||
> + storageBus == StorageBus_SAS) {
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> } else if (storageBus == StorageBus_Floppy) {
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
> diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> index b08ad1e3e..23940fc63 100644
> --- a/src/vbox/vbox_common.h
> +++ b/src/vbox/vbox_common.h
> @@ -326,6 +326,14 @@ enum HardDiskVariant
> # define VBOX_E_INVALID_SESSION_STATE 0x80BB000B
> # define VBOX_E_OBJECT_IN_USE 0x80BB000C
>
> +/* VBOX storage controller name definitions */
> +# define VBOX_CONTROLLER_IDE_NAME "IDE Controller"
> +# define VBOX_CONTROLLER_FLOPPY_NAME "Floppy Controller"
> +# define VBOX_CONTROLLER_SATA_NAME "SATA Controller"
> +# define VBOX_CONTROLLER_SCSI_NAME "SCSI Controller"
> +# define VBOX_CONTROLLER_SAS_NAME "SAS Controller"
> +
> +
> /* Simplied definitions in vbox_CAPI_*.h */
>
> typedef void const *PCVBOXXPCOM;
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 6592cbd63..b7ced62dc 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
I think this should be a separate patch prior to this one. You're
adjusting format and removing the ATTRIBUTE_UNUSED for arguments that
are actually used.
> @@ -684,7 +684,9 @@ _virtualboxCreateHardDisk(IVirtualBox *vboxObj,
> PRUnichar *format,
> #if VBOX_API_VERSION < 5000000
> return vboxObj->vtbl->CreateHardDisk(vboxObj, format,
> location, medium);
> #elif VBOX_API_VERSION >= 5000000 /*VBOX_API_VERSION >= 5000000*/
> - return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
> AccessMode_ReadWrite, DeviceType_HardDisk, medium);
> + return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
> + AccessMode_ReadWrite,
> + DeviceType_HardDisk,
> medium);
> #endif /*VBOX_API_VERSION >= 5000000*/
> }
>
> @@ -696,37 +698,28 @@ _virtualboxRegisterMachine(IVirtualBox
> *vboxObj, IMachine *machine)
>
> static nsresult
> _virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location,
> - PRUint32 deviceType ATTRIBUTE_UNUSED,
> - PRUint32 accessMode ATTRIBUTE_UNUSED,
> - IMedium **medium)
> + PRUint32 deviceType,
> + PRUint32 accessMode ATTRIBUTE_UNUSED,
> IMedium **medium)
Keep IMedium **medium on it's own line.
It's the preferred way at least for libvirt code on function args.
> {
> #if VBOX_API_VERSION < 4002000
> - return vboxObj->vtbl->FindMedium(vboxObj, location,
> - deviceType, medium);
> + return vboxObj->vtbl->FindMedium(vboxObj, location,
> deviceType, medium);
> #else /* VBOX_API_VERSION >= 4002000 */
> - return vboxObj->vtbl->OpenMedium(vboxObj, location,
> - deviceType, accessMode,
> PR_FALSE, medium);
> + return vboxObj->vtbl->OpenMedium(vboxObj, location,
> deviceType, accessMode,
> + PR_FALSE, medium);
> #endif /* VBOX_API_VERSION >= 4002000 */
> }
>
> static nsresult
> -_virtualboxOpenMedium(IVirtualBox *vboxObj ATTRIBUTE_UNUSED,
> - PRUnichar *location ATTRIBUTE_UNUSED,
> - PRUint32 deviceType ATTRIBUTE_UNUSED,
> - PRUint32 accessMode ATTRIBUTE_UNUSED,
> - IMedium **medium ATTRIBUTE_UNUSED)
> +_virtualboxOpenMedium(IVirtualBox *vboxObj, PRUnichar *location,
> + PRUint32 deviceType, PRUint32 accessMode,
> + IMedium **medium)
For this one - keep each argument on it's own line and just remove
the
ATTRIBUTE_UNUSED
> {
> #if VBOX_API_VERSION == 4000000
> - return vboxObj->vtbl->OpenMedium(vboxObj,
> - location,
> - deviceType, accessMode,
> + return vboxObj->vtbl->OpenMedium(vboxObj, location,
> deviceType, accessMode,
> medium);
> #elif VBOX_API_VERSION >= 4001000
> - return vboxObj->vtbl->OpenMedium(vboxObj,
> - location,
> - deviceType, accessMode,
> - false,
> - medium);
> + return vboxObj->vtbl->OpenMedium(vboxObj, location,
> deviceType, accessMode,
> + false, medium);
> #endif
> }
>
> @@ -778,12 +771,8 @@ _machineGetStorageControllerByName(IMachine
> *machine, PRUnichar *name,
> }
>
> static nsresult
> -_machineAttachDevice(IMachine *machine ATTRIBUTE_UNUSED,
> - PRUnichar *name ATTRIBUTE_UNUSED,
> - PRInt32 controllerPort ATTRIBUTE_UNUSED,
> - PRInt32 device ATTRIBUTE_UNUSED,
> - PRUint32 type ATTRIBUTE_UNUSED,
> - IMedium * medium ATTRIBUTE_UNUSED)
> +_machineAttachDevice(IMachine *machine, PRUnichar *name, PRInt32
> controllerPort,
> + PRInt32 device, PRUint32 type, IMedium *
> medium)
Same - one line per argument is preferred
John
> {
> return machine->vtbl->AttachDevice(machine, name,
> controllerPort,
> device, type, medium);
>
3:49 p.m.
New subject: [libvirt] [PATCH 6/6] vbox: Update XML dump of storage devices.
From: Dawid Zamirski <dzamirski(a)datto.com>
* added vboxDumpStorageControllers
* replaced vboxDumpIDEHDDs with vboxDumpDisks which has been refectored
quite a bit to handle removable devices, the new SAS controller
support etc.
* align the logic in vboxSnapshotGetReadWriteDisks and
vboxSnapshotGetReadOnlyDisks to more closely resemble vboxDumpDisks.
* vboxGenerateMediumName was simplified to no longer "compute" the
device name value as deviePort/deviceSlot and their upper bound
values are no longer enough to do this accurately due to the added
SAS bus support which does not "map" directly into libvirt XML
semantics
* vboxGetMaxPortSlotValues is now removed as it's no longer used
---
src/vbox/vbox_common.c | 691 ++++++++++++++++++++++++++++---------------------
1 file changed, 393 insertions(+), 298 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 7645b29a0..dd876645a 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -290,126 +290,44 @@ static int openSessionForMachine(vboxDriverPtr data, const unsigned
char *dom_uu
return 0;
}
-/**
- * function to get the values for max port per
- * instance and max slots per port for the devices
- *
- * @returns true on Success, false on failure.
- * @param vbox Input IVirtualBox pointer
- * @param maxPortPerInst Output array of max port per instance
- * @param maxSlotPerPort Output array of max slot per port
- *
- */
-
-static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
- PRUint32 *maxPortPerInst,
- PRUint32 *maxSlotPerPort)
-{
- ISystemProperties *sysProps = NULL;
-
- if (!vbox)
- return false;
-
- gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, &sysProps);
-
- if (!sysProps)
- return false;
-
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_IDE,
-
&maxPortPerInst[StorageBus_IDE]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SATA,
-
&maxPortPerInst[StorageBus_SATA]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SCSI,
-
&maxPortPerInst[StorageBus_SCSI]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SAS,
-
&maxPortPerInst[StorageBus_SAS]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_Floppy,
-
&maxPortPerInst[StorageBus_Floppy]);
-
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_IDE,
-
&maxSlotPerPort[StorageBus_IDE]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_SATA,
-
&maxSlotPerPort[StorageBus_SATA]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_SCSI,
-
&maxSlotPerPort[StorageBus_SCSI]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_SAS,
-
&maxSlotPerPort[StorageBus_SAS]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_Floppy,
-
&maxSlotPerPort[StorageBus_Floppy]);
-
- VBOX_RELEASE(sysProps);
-
- return true;
-}
-
/**
* function to generate the name for medium,
* for e.g: hda, sda, etc
*
* @returns null terminated string with device name or NULL
* for failures
- * @param conn Input Connection Pointer
* @param storageBus Input storage bus type
- * @param deviceInst Input device instance number
* @param devicePort Input port number
* @param deviceSlot Input slot number
- * @param aMaxPortPerInst Input array of max port per device instance
- * @param aMaxSlotPerPort Input array of max slot per device port
- *
+ * @param sdCount Running total of disk devices with "sd" prefix
*/
-static char *vboxGenerateMediumName(PRUint32 storageBus,
- PRInt32 deviceInst,
- PRInt32 devicePort,
- PRInt32 deviceSlot,
- PRUint32 *aMaxPortPerInst,
- PRUint32 *aMaxSlotPerPort)
+static char *
+vboxGenerateMediumName(PRUint32 storageBus, PRInt32 devicePort,
+ PRInt32 deviceSlot, size_t sdCount)
{
const char *prefix = NULL;
char *name = NULL;
int total = 0;
- PRUint32 maxPortPerInst = 0;
- PRUint32 maxSlotPerPort = 0;
-
- if (!aMaxPortPerInst ||
- !aMaxSlotPerPort)
- return NULL;
if ((storageBus < StorageBus_IDE) ||
(storageBus > StorageBus_SAS))
return NULL;
- maxPortPerInst = aMaxPortPerInst[storageBus];
- maxSlotPerPort = aMaxSlotPerPort[storageBus];
- total = (deviceInst * maxPortPerInst * maxSlotPerPort)
- + (devicePort * maxSlotPerPort)
- + deviceSlot;
-
if (storageBus == StorageBus_IDE) {
prefix = "hd";
+ total = devicePort * 2 + deviceSlot;
} else if (storageBus == StorageBus_SATA ||
storageBus == StorageBus_SCSI ||
storageBus == StorageBus_SAS) {
prefix = "sd";
+ total = sdCount;
} else if (storageBus == StorageBus_Floppy) {
+ total = deviceSlot;
prefix = "fd";
}
name = virIndexToDiskName(total, prefix);
- VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, "
- "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u",
- NULLSTR(name), total, storageBus, deviceInst, devicePort,
- deviceSlot, maxPortPerInst, maxSlotPerPort);
return name;
}
@@ -3086,162 +3004,300 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr
def, IMachine *mach
}
static void
-vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
+vboxDumpStorageControllers(virDomainDefPtr def,
+ vboxDriverPtr data ATTRIBUTE_UNUSED,
+ IMachine *machine)
{
- /* dump IDE hdds if present */
- vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
- bool error = false;
- int diskCount = 0;
- size_t i;
- PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {};
+ vboxArray storageControllers = VBOX_ARRAY_INITIALIZER;
+ virDomainControllerDefPtr cont = NULL;
+ size_t i = 0;
- def->ndisks = 0;
- gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
-
gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+ gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine,
+ gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));
- /* get the number of attachments */
- for (i = 0; i < mediumAttachments.count; i++) {
- IMediumAttachment *imediumattach = mediumAttachments.items[i];
- if (imediumattach) {
- IMedium *medium = NULL;
+ for (i = 0; i < storageControllers.count; i++) {
+ IStorageController *controller = storageControllers.items[i];
+ PRUint32 storageBus = StorageBus_Null;
+ PRUint32 controllerType = StorageControllerType_Null;
+
+ gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+ gVBoxAPI.UIStorageController.GetControllerType(controller,
+ &controllerType);
+
+ switch (storageBus) {
+ case StorageBus_IDE:
+ {
+ int model = -1;
+ switch (controllerType) {
+ case StorageControllerType_PIIX3:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3;
+ break;
+ case StorageControllerType_PIIX4:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4;
+ break;
+ case StorageControllerType_ICH6:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6;
+ break;
+ }
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
- if (medium) {
- def->ndisks++;
- VBOX_RELEASE(medium);
+ cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_IDE,
+ -1, model);
+
+ break;
+ }
+ case StorageBus_SCSI:
+ {
+ int model = -1;
+ switch (controllerType) {
+ case StorageControllerType_BusLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC;
+ break;
+ case StorageControllerType_LsiLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+ break;
}
+
+ cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+ -1, model);
+
+ break;
+ }
+ case StorageBus_SAS:
+ cont = virDomainDefAddController(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_SCSI, -1,
+
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068);
+ break;
+ case StorageBus_SATA:
+ cont = virDomainDefAddController(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_SATA, -1, -1);
+ break;
+ case StorageBus_Floppy:
+ cont = virDomainDefAddController(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_FDC, -1, -1);
+ break;
}
}
- /* Allocate mem, if fails return error */
- if (VIR_ALLOC_N(def->disks, def->ndisks) >= 0) {
- for (i = 0; i < def->ndisks; i++) {
- virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
- if (!disk) {
- error = true;
- break;
+ if (!cont) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to add controller definition"));
+ }
+}
+
+static void
+vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
+{
+ vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
+ IMediumAttachment *mediumattach = NULL;
+ IMedium *medium = NULL;
+ nsresult rc;
+ size_t sdCount = 0, i, j;
+
+ def->ndisks = 0;
+ rc = gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
+ gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not get machine's medium attachments"));
+ goto error;
+ }
+
+ /* get the number of attachments */
+ for (i = 0; i < mediumAttachments.count; i++) {
+ mediumattach = mediumAttachments.items[i];
+ if (mediumattach) {
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumattach, &medium);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium"));
+ goto error;
}
- def->disks[i] = disk;
+
+ def->ndisks++;
+ VBOX_RELEASE(medium);
}
- } else {
- error = true;
}
- if (!error)
- error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
maxSlotPerPort);
+ if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
+ goto error;
+
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+ if (!disk)
+ goto error;
+
+ def->disks[i] = disk;
+ }
/* get the attachment details here */
- for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks
&& !error; i++) {
- IMediumAttachment *imediumattach = mediumAttachments.items[i];
+ for (i = 0; i < mediumAttachments.count; i++) {
+ mediumattach = mediumAttachments.items[i];
IStorageController *storageController = NULL;
PRUnichar *storageControllerName = NULL;
PRUint32 deviceType = DeviceType_Null;
PRUint32 storageBus = StorageBus_Null;
PRBool readOnly = PR_FALSE;
- IMedium *medium = NULL;
PRUnichar *mediumLocUtf16 = NULL;
char *mediumLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
+ virDomainDiskDefPtr disk = def->disks[i];
- if (!imediumattach)
- continue;
+ if (!mediumattach)
+ goto skip;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
- if (!medium)
- continue;
+ rc = gVBoxAPI.UIMediumAttachment.GetController(mediumattach,
+ &storageControllerName);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get storage controller name for medium "
+ "attachment, rc=%08x", (unsigned) rc);
+ goto skip;
+ }
- gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
- if (!storageControllerName) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ storageControllerName,
+ &storageController);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get storage controller for medium attachment,
"
+ "rc=%08x", (unsigned) rc);
+ goto skip;
}
- gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
- storageControllerName,
- &storageController);
- VBOX_UTF16_FREE(storageControllerName);
- if (!storageController) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController,
+ &storageBus);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get storage controller bus type, rc=%08x",
+ (unsigned) rc);
+ goto skip;
}
- gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
- VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
- VBOX_UTF16_FREE(mediumLocUtf16);
- ignore_value(virDomainDiskSetSource(def->disks[diskCount],
- mediumLocUtf8));
- VBOX_UTF8_FREE(mediumLocUtf8);
+ rc = gVBoxAPI.UIMediumAttachment.GetType(mediumattach, &deviceType);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get device type for medium attachment,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
- if (!virDomainDiskGetSource(def->disks[diskCount])) {
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
- error = true;
- break;
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumattach, &devicePort);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get device port for medium attachment,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumattach, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get device slot for medium attachment,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
}
- gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumattach, &medium);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get medium for medium attachment, rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ /* removable drives might not have mediums */
+ if (medium) {
+ rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Clould not get storage location for medium,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Clould not get read-only status for medium,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+ if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+ VIR_WARN("Could not set disk source for medium %s",
+ mediumLocUtf8);
+ goto skip;
+ }
+ }
+
+ disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ disk->info.addr.drive.bus = 0;
+ disk->info.addr.drive.unit = devicePort;
+
+ disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
+ sdCount);
if (storageBus == StorageBus_IDE) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
+ disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
+ disk->info.addr.drive.bus = devicePort; /* primary, secondary */
+ disk->info.addr.drive.unit = deviceSlot; /* master, slave */
+ disk->dst = virIndexToDiskName(devicePort * 2 + deviceSlot,
"hd");
+ disk->info.addr.drive.controller =
+ virDomainControllerFindByType(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_IDE);
} else if (storageBus == StorageBus_SATA) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
+ disk->info.addr.drive.controller =
+ virDomainControllerFindByType(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_SATA);
+ sdCount++;
} else if (storageBus == StorageBus_SCSI ||
storageBus == StorageBus_SAS) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ sdCount++;
+ /* find scsi controller index with matching model for vbox bus type */
+ for (j = 0; j < def->ncontrollers; j++) {
+ virDomainControllerDefPtr ctrl = def->controllers[j];
+
+ if (ctrl->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+ continue;
+
+ if (storageBus == StorageBus_SAS &&
+ ctrl->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ disk->info.addr.drive.controller = ctrl->idx;
+ } else if (storageBus == StorageBus_SCSI &&
+ ctrl->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068)
{
+ disk->info.addr.drive.controller = ctrl->idx;
+ }
+ }
} else if (storageBus == StorageBus_Floppy) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
+ disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+ disk->info.addr.drive.unit = deviceSlot;
}
- gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
if (deviceType == DeviceType_HardDisk)
- def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK;
+ disk->device = VIR_DOMAIN_DISK_DEVICE_DISK;
else if (deviceType == DeviceType_Floppy)
- def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
+ disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
else if (deviceType == DeviceType_DVD)
- def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
-
- gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
- gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
- def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
- deviceInst,
- devicePort,
- deviceSlot,
- maxPortPerInst,
- maxSlotPerPort);
- if (!def->disks[diskCount]->dst) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not generate medium name for the disk "
- "at: controller instance:%u, port:%d, slot:%d"),
- deviceInst, devicePort, deviceSlot);
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
- error = true;
- break;
- }
+ disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
- gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
if (readOnly == PR_TRUE)
- def->disks[diskCount]->src->readonly = true;
+ disk->src->readonly = true;
- virDomainDiskSetType(def->disks[diskCount],
- VIR_STORAGE_TYPE_FILE);
+ virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
+ skip:
+ VBOX_UTF16_FREE(storageControllerName);
+ VBOX_UTF8_FREE(mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
VBOX_RELEASE(medium);
VBOX_RELEASE(storageController);
- diskCount++;
}
gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
- /* cleanup on error */
- if (error) {
- for (i = 0; i < def->ndisks; i++)
- VIR_FREE(def->disks[i]);
- VIR_FREE(def->disks);
- def->ndisks = 0;
- }
+ return;
+
+ error:
+ for (i = 0; i < def->ndisks; i++)
+ VIR_FREE(def->disks[i]);
+ VIR_FREE(def->disks);
+ def->ndisks = 0;
+ gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
}
static int
@@ -3934,8 +3990,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
if (vboxDumpDisplay(def, data, machine) < 0)
goto cleanup;
- vboxDumpIDEHDDs(def, data, machine);
-
+ vboxDumpStorageControllers(def, data, machine);
+ vboxDumpDisks(def, data, machine);
vboxDumpSharedFolders(def, data, machine);
vboxDumpNetwork(def, data, machine, networkAdapterCount);
vboxDumpAudio(def, data, machine);
@@ -5490,8 +5546,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data,
return snapshot;
}
-static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
- virDomainSnapshotPtr snapshot)
+static int
+vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
+ virDomainSnapshotPtr snapshot)
{
virDomainPtr dom = snapshot->domain;
vboxDriverPtr data = dom->conn->privateData;
@@ -5500,9 +5557,7 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
ISnapshot *snap = NULL;
IMachine *snapMachine = NULL;
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
- int diskCount = 0;
+ size_t diskCount = 0, sdCount = 0;
nsresult rc;
vboxIID snapIid;
char *snapshotUuidStr = NULL;
@@ -5571,9 +5626,6 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
goto cleanup;
}
- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort))
- goto cleanup;
-
/* get the attachment details here */
for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
IStorageController *storageController = NULL;
@@ -5583,7 +5635,6 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
IMedium *disk = NULL;
PRUnichar *childLocUtf16 = NULL;
char *childLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
vboxArray children = VBOX_ARRAY_INITIALIZER;
@@ -5592,31 +5643,76 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
void *handle;
size_t j = 0;
size_t k = 0;
+
if (!imediumattach)
continue;
- rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+
+ rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
+ &storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium"));
+ _("cannot get controller"));
goto cleanup;
}
- if (!disk)
+
+ if (!storageControllerName)
continue;
- rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
+
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ storageControllerName,
+ &storageController);
+ VBOX_UTF16_FREE(storageControllerName);
+ if (!storageController)
+ continue;
+
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get controller"));
+ _("cannot get storage controller bus"));
+ VBOX_RELEASE(storageController);
goto cleanup;
}
- if (!storageControllerName) {
- VBOX_RELEASE(disk);
- continue;
+ rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment type"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment type"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
}
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment device"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+
+ /* skip removable disk */
+ if (!disk)
+ goto skip;
+
handle = gVBoxAPI.UArray.handleMediumGetChildren(disk);
rc = gVBoxAPI.UArray.vboxArrayGet(&children, disk, handle);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get children disk"));
+ VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
handle = gVBoxAPI.UArray.handleMediumGetSnapshotIds(disk);
@@ -5625,8 +5721,11 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get snapshot ids"));
+ VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
+
for (j = 0; j < children.count; ++j) {
IMedium *child = children.items[j];
for (k = 0; k < snapshotIids.count; ++k) {
@@ -5634,18 +5733,13 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
char *diskSnapIdStr = NULL;
VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr);
if (STREQ(diskSnapIdStr, snapshotUuidStr)) {
- rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
-
storageControllerName,
-
&storageController);
- VBOX_UTF16_FREE(storageControllerName);
- if (!storageController) {
- VBOX_RELEASE(child);
- break;
- }
rc = gVBoxAPI.UIMedium.GetLocation(child, &childLocUtf16);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get disk location"));
+ VBOX_RELEASE(child);
+ VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8);
@@ -5653,52 +5747,37 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
if (VIR_STRDUP(def->disks[diskCount].src->path, childLocUtf8)
< 0) {
VBOX_RELEASE(child);
VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
VBOX_UTF8_FREE(childLocUtf8);
- rc = gVBoxAPI.UIStorageController.GetBus(storageController,
&storageBus);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller
bus"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach,
&deviceType);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment
type"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach,
&devicePort);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment
type"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach,
&deviceSlot);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment
device"));
- goto cleanup;
- }
def->disks[diskCount].src->type = VIR_STORAGE_TYPE_FILE;
def->disks[diskCount].name = vboxGenerateMediumName(storageBus,
- deviceInst,
devicePort,
deviceSlot,
- maxPortPerInst,
- maxSlotPerPort);
+ sdCount);
}
VBOX_UTF8_FREE(diskSnapIdStr);
}
}
+
+ diskCount++;
+
+ skip:
VBOX_RELEASE(storageController);
VBOX_RELEASE(disk);
- diskCount++;
+
+ if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+
}
+
gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
ret = 0;
+
cleanup:
if (ret < 0) {
for (i = 0; i < def->ndisks; i++)
@@ -5710,9 +5789,9 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
return ret;
}
-static
-int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
- virDomainSnapshotDefPtr def)
+static int
+vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def,
+ virDomainSnapshotPtr snapshot)
{
virDomainPtr dom = snapshot->domain;
vboxDriverPtr data = dom->conn->privateData;
@@ -5725,9 +5804,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
nsresult rc;
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
size_t i = 0;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
- int diskCount = 0;
+ size_t diskCount = 0, sdCount = 0;
int ret = -1;
if (!data->vboxObj)
@@ -5790,9 +5867,6 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
goto cleanup;
}
- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort))
- goto cleanup;
-
/* get the attachment details here */
for (i = 0; i < mediumAttachments.count && diskCount <
def->dom->ndisks; i++) {
PRUnichar *storageControllerName = NULL;
@@ -5805,16 +5879,17 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
IMediumAttachment *imediumattach = mediumAttachments.items[i];
+
if (!imediumattach)
continue;
+
rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get medium"));
goto cleanup;
}
- if (!disk)
- continue;
+
rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -5834,41 +5909,85 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
VBOX_UTF16_FREE(storageControllerName);
if (!storageController)
continue;
- rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16);
+
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get disk location"));
+ _("cannot get storage controller bus"));
goto cleanup;
}
- VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
- VBOX_UTF16_FREE(mediumLocUtf16);
- if (VIR_STRDUP(def->dom->disks[diskCount]->src->path, mediumLocUtf8)
< 0)
+
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment port"));
goto cleanup;
+ }
- VBOX_UTF8_FREE(mediumLocUtf8);
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get device"));
+ goto cleanup;
+ }
- rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+
+ rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller bus"));
+ _("cannot get medium attachment type"));
goto cleanup;
}
+
+ if (disk) {
+ rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get disk location"));
+ goto cleanup;
+ }
+ VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
+ if (VIR_STRDUP(def->dom->disks[diskCount]->src->path,
mediumLocUtf8) < 0)
+ goto cleanup;
+
+ VBOX_UTF8_FREE(mediumLocUtf8);
+
+ rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get read only attribute"));
+ goto cleanup;
+ }
+ } else {
+ /* for removable drives, bump sdCount according to type so that
+ * device names are properly generated, and then skip it
+ */
+ if (storageBus == StorageBus_SATA ||
+ storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+
+ continue;
+ }
+
+ def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
+ devicePort,
+ deviceSlot,
+ sdCount);
if (storageBus == StorageBus_IDE) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ sdCount++;
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ sdCount++;
} else if (storageBus == StorageBus_Floppy) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
}
- rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment type"));
- goto cleanup;
- }
if (deviceType == DeviceType_HardDisk)
def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK;
else if (deviceType == DeviceType_Floppy)
@@ -5876,33 +5995,9 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
else if (deviceType == DeviceType_DVD)
def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
- rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment port"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get device"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get read only attribute"));
- goto cleanup;
- }
if (readOnly == PR_TRUE)
def->dom->disks[diskCount]->src->readonly = true;
def->dom->disks[diskCount]->src->type = VIR_STORAGE_TYPE_FILE;
- def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
- deviceInst,
- devicePort,
- deviceSlot,
- maxPortPerInst,
- maxSlotPerPort);
if (!def->dom->disks[diskCount]->dst) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not generate medium name for the disk "
@@ -5995,7 +6090,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr
snapshot,
if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0)
VIR_DEBUG("Could not get read write disks for snapshot");
- if (vboxSnapshotGetReadOnlyDisks(snapshot, def) < 0)
+ if (vboxSnapshotGetReadOnlyDisks(def, snapshot) < 0)
VIR_DEBUG("Could not get Readonly disks for snapshot");
}
--
2.14.2
2:47 p.m.
New subject: [libvirt] [PATCH 6/6] vbox: Update XML dump of storage devices.
On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski(a)datto.com>
* added vboxDumpStorageControllers
* replaced vboxDumpIDEHDDs with vboxDumpDisks which has been refectored
quite a bit to handle removable devices, the new SAS controller
support etc.
* align the logic in vboxSnapshotGetReadWriteDisks and
vboxSnapshotGetReadOnlyDisks to more closely resemble vboxDumpDisks.
* vboxGenerateMediumName was simplified to no longer "compute" the
device name value as deviePort/deviceSlot and their upper bound
values are no longer enough to do this accurately due to the added
SAS bus support which does not "map" directly into libvirt XML
semantics
* vboxGetMaxPortSlotValues is now removed as it's no longer used
---
src/vbox/vbox_common.c | 691 ++++++++++++++++++++++++++++---------------------
1 file changed, 393 insertions(+), 298 deletions(-)
Nny way to break this up into smaller more reviewably manageable chunks?
Certainly as I pointed out before, separating out the SAS into its own
patch could include whatever changes would happen here to support it.
It's too bad vboxDumpIDEHDDs couldn't be more easily split using the
current logic then updates applied to use the controller fetches.
I've kind of lost steam and think there's enough up to this point that
needs to be redone that I'll wait for the followup to look more in
depth. I noted a couple of things below.
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 7645b29a0..dd876645a 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -290,126 +290,44 @@ static int openSessionForMachine(vboxDriverPtr data, const
unsigned char *dom_uu
return 0;
}
-/**
- * function to get the values for max port per
- * instance and max slots per port for the devices
- *
- * @returns true on Success, false on failure.
- * @param vbox Input IVirtualBox pointer
- * @param maxPortPerInst Output array of max port per instance
- * @param maxSlotPerPort Output array of max slot per port
- *
- */
-
-static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
- PRUint32 *maxPortPerInst,
- PRUint32 *maxSlotPerPort)
-{
- ISystemProperties *sysProps = NULL;
-
- if (!vbox)
- return false;
-
- gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, &sysProps);
-
- if (!sysProps)
- return false;
-
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_IDE,
-
&maxPortPerInst[StorageBus_IDE]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SATA,
-
&maxPortPerInst[StorageBus_SATA]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SCSI,
-
&maxPortPerInst[StorageBus_SCSI]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SAS,
-
&maxPortPerInst[StorageBus_SAS]);
- gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_Floppy,
-
&maxPortPerInst[StorageBus_Floppy]);
-
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_IDE,
-
&maxSlotPerPort[StorageBus_IDE]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_SATA,
-
&maxSlotPerPort[StorageBus_SATA]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_SCSI,
-
&maxSlotPerPort[StorageBus_SCSI]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_SAS,
-
&maxSlotPerPort[StorageBus_SAS]);
- gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
- StorageBus_Floppy,
-
&maxSlotPerPort[StorageBus_Floppy]);
-
- VBOX_RELEASE(sysProps);
-
- return true;
-}
-
/**
* function to generate the name for medium,
* for e.g: hda, sda, etc
*
* @returns null terminated string with device name or NULL
* for failures
- * @param conn Input Connection Pointer
* @param storageBus Input storage bus type
- * @param deviceInst Input device instance number
* @param devicePort Input port number
* @param deviceSlot Input slot number
- * @param aMaxPortPerInst Input array of max port per device instance
- * @param aMaxSlotPerPort Input array of max slot per device port
- *
+ * @param sdCount Running total of disk devices with "sd"
prefix
*/
-static char *vboxGenerateMediumName(PRUint32 storageBus,
- PRInt32 deviceInst,
- PRInt32 devicePort,
- PRInt32 deviceSlot,
- PRUint32 *aMaxPortPerInst,
- PRUint32 *aMaxSlotPerPort)
+static char *
+vboxGenerateMediumName(PRUint32 storageBus, PRInt32 devicePort,
+ PRInt32 deviceSlot, size_t sdCount)
As w/ previous patch - one argument per line is the normal libvirt
preference
{
const char *prefix = NULL;
char *name = NULL;
int total = 0;
- PRUint32 maxPortPerInst = 0;
- PRUint32 maxSlotPerPort = 0;
-
- if (!aMaxPortPerInst ||
- !aMaxSlotPerPort)
- return NULL;
if ((storageBus < StorageBus_IDE) ||
(storageBus > StorageBus_SAS))
return NULL;
- maxPortPerInst = aMaxPortPerInst[storageBus];
- maxSlotPerPort = aMaxSlotPerPort[storageBus];
- total = (deviceInst * maxPortPerInst * maxSlotPerPort)
- + (devicePort * maxSlotPerPort)
- + deviceSlot;
-
if (storageBus == StorageBus_IDE) {
prefix = "hd";
+ total = devicePort * 2 + deviceSlot;
} else if (storageBus == StorageBus_SATA ||
storageBus == StorageBus_SCSI ||
storageBus == StorageBus_SAS) {
prefix = "sd";
+ total = sdCount;
} else if (storageBus == StorageBus_Floppy) {
+ total = deviceSlot;
prefix = "fd";
}
name = virIndexToDiskName(total, prefix);
- VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, "
- "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u",
- NULLSTR(name), total, storageBus, deviceInst, devicePort,
- deviceSlot, maxPortPerInst, maxSlotPerPort);
return name;
}
@@ -3086,162 +3004,300 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr
def, IMachine *mach
}
static void
-vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
+vboxDumpStorageControllers(virDomainDefPtr def,
+ vboxDriverPtr data ATTRIBUTE_UNUSED,
Is this ever going to be used? Why pass it then to a newly defined
function? Is it because all the callers pass it whether or not it's
used? Seems strange, especially considering it's the driver pointer.
+ IMachine *machine)
{
- /* dump IDE hdds if present */
- vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
- bool error = false;
- int diskCount = 0;
- size_t i;
- PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {};
+ vboxArray storageControllers = VBOX_ARRAY_INITIALIZER;
+ virDomainControllerDefPtr cont = NULL;
+ size_t i = 0;
- def->ndisks = 0;
- gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
-
gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+ gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine,
+ gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));
- /* get the number of attachments */
- for (i = 0; i < mediumAttachments.count; i++) {
- IMediumAttachment *imediumattach = mediumAttachments.items[i];
- if (imediumattach) {
- IMedium *medium = NULL;
+ for (i = 0; i < storageControllers.count; i++) {
+ IStorageController *controller = storageControllers.items[i];
+ PRUint32 storageBus = StorageBus_Null;
+ PRUint32 controllerType = StorageControllerType_Null;
+
+ gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+ gVBoxAPI.UIStorageController.GetControllerType(controller,
+ &controllerType);
+
+ switch (storageBus) {
+ case StorageBus_IDE:
+ {
+ int model = -1;
+ switch (controllerType) {
+ case StorageControllerType_PIIX3:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3;
+ break;
+ case StorageControllerType_PIIX4:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4;
+ break;
+ case StorageControllerType_ICH6:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6;
+ break;
+ }
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
- if (medium) {
- def->ndisks++;
- VBOX_RELEASE(medium);
+ cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_IDE,
+ -1, model);
+
+ break;
+ }
+ case StorageBus_SCSI:
+ {
+ int model = -1;
+ switch (controllerType) {
+ case StorageControllerType_BusLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC;
+ break;
+ case StorageControllerType_LsiLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+ break;
}
+
+ cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+ -1, model);
+
+ break;
+ }
+ case StorageBus_SAS:
+ cont = virDomainDefAddController(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_SCSI, -1,
+
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068);
+ break;
+ case StorageBus_SATA:
+ cont = virDomainDefAddController(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_SATA, -1, -1);
+ break;
+ case StorageBus_Floppy:
+ cont = virDomainDefAddController(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_FDC, -1, -1);
+ break;
}
}
- /* Allocate mem, if fails return error */
- if (VIR_ALLOC_N(def->disks, def->ndisks) >= 0) {
- for (i = 0; i < def->ndisks; i++) {
- virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
- if (!disk) {
- error = true;
- break;
+ if (!cont) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to add controller definition"));
+ }
+}
+
+static void
+vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
One argument per line
+{
+ vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
+ IMediumAttachment *mediumattach = NULL;
+ IMedium *medium = NULL;
+ nsresult rc;
+ size_t sdCount = 0, i, j;
+
+ def->ndisks = 0;> + rc =
gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
+ gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not get machine's medium attachments"));
+ goto error;
+ }
+
+ /* get the number of attachments */
+ for (i = 0; i < mediumAttachments.count; i++) {
+ mediumattach = mediumAttachments.items[i];
+ if (mediumattach) {
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumattach, &medium);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium"));
+ goto error;
}
- def->disks[i] = disk;
+
+ def->ndisks++;
+ VBOX_RELEASE(medium);
}
- } else {
- error = true;
}
- if (!error)
- error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
maxSlotPerPort);
+ if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
+ goto error;
+
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+ if (!disk)
+ goto error;
+
+ def->disks[i] = disk;
+ }
/* get the attachment details here */
- for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks
&& !error; i++) {
- IMediumAttachment *imediumattach = mediumAttachments.items[i];
+ for (i = 0; i < mediumAttachments.count; i++) {
+ mediumattach = mediumAttachments.items[i];
IStorageController *storageController = NULL;
PRUnichar *storageControllerName = NULL;
PRUint32 deviceType = DeviceType_Null;
PRUint32 storageBus = StorageBus_Null;
PRBool readOnly = PR_FALSE;
- IMedium *medium = NULL;
PRUnichar *mediumLocUtf16 = NULL;
char *mediumLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
+ virDomainDiskDefPtr disk = def->disks[i];
- if (!imediumattach)
- continue;
+ if (!mediumattach)
+ goto skip;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
- if (!medium)
- continue;
+ rc = gVBoxAPI.UIMediumAttachment.GetController(mediumattach,
+ &storageControllerName);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get storage controller name for medium "
+ "attachment, rc=%08x", (unsigned) rc);
+ goto skip;
+ }
- gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
- if (!storageControllerName) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ storageControllerName,
+ &storageController);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get storage controller for medium attachment,
"
+ "rc=%08x", (unsigned) rc);
+ goto skip;
}
- gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
- storageControllerName,
- &storageController);
- VBOX_UTF16_FREE(storageControllerName);
- if (!storageController) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController,
+ &storageBus);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get storage controller bus type, rc=%08x",
+ (unsigned) rc);
+ goto skip;
}
- gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
- VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
- VBOX_UTF16_FREE(mediumLocUtf16);
- ignore_value(virDomainDiskSetSource(def->disks[diskCount],
- mediumLocUtf8));
- VBOX_UTF8_FREE(mediumLocUtf8);
+ rc = gVBoxAPI.UIMediumAttachment.GetType(mediumattach, &deviceType);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get device type for medium attachment,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
- if (!virDomainDiskGetSource(def->disks[diskCount])) {
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
- error = true;
- break;
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumattach, &devicePort);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get device port for medium attachment,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumattach, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get device slot for medium attachment,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
}
- gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumattach, &medium);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Could not get medium for medium attachment, rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ /* removable drives might not have mediums */
+ if (medium) {
+ rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Clould not get storage location for medium,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+ if (NS_FAILED(rc)) {
+ VIR_WARN("Clould not get read-only status for medium,
rc=%08x",
+ (unsigned) rc);
+ goto skip;
+ }
+
+ VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+ if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+ VIR_WARN("Could not set disk source for medium %s",
+ mediumLocUtf8);
+ goto skip;
+ }
+ }
+
+ disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ disk->info.addr.drive.bus = 0;
+ disk->info.addr.drive.unit = devicePort;
+
+ disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
+ sdCount);
if (storageBus == StorageBus_IDE) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
+ disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
+ disk->info.addr.drive.bus = devicePort; /* primary, secondary */
+ disk->info.addr.drive.unit = deviceSlot; /* master, slave */
+ disk->dst = virIndexToDiskName(devicePort * 2 + deviceSlot,
"hd");
+ disk->info.addr.drive.controller =
+ virDomainControllerFindByType(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_IDE);
} else if (storageBus == StorageBus_SATA) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
+ disk->info.addr.drive.controller =
+ virDomainControllerFindByType(def,
+ VIR_DOMAIN_CONTROLLER_TYPE_SATA);
+ sdCount++;
} else if (storageBus == StorageBus_SCSI ||
storageBus == StorageBus_SAS) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ sdCount++;
+ /* find scsi controller index with matching model for vbox bus type */
+ for (j = 0; j < def->ncontrollers; j++) {
+ virDomainControllerDefPtr ctrl = def->controllers[j];
+
+ if (ctrl->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+ continue;
+
+ if (storageBus == StorageBus_SAS &&
+ ctrl->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ disk->info.addr.drive.controller = ctrl->idx;
+ } else if (storageBus == StorageBus_SCSI &&
+ ctrl->model !=
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ disk->info.addr.drive.controller = ctrl->idx;
+ }
+ }
} else if (storageBus == StorageBus_Floppy) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
+ disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+ disk->info.addr.drive.unit = deviceSlot;
}
- gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
if (deviceType == DeviceType_HardDisk)
- def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK;
+ disk->device = VIR_DOMAIN_DISK_DEVICE_DISK;
else if (deviceType == DeviceType_Floppy)
- def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
+ disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
else if (deviceType == DeviceType_DVD)
- def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
-
- gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
- gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
- def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
- deviceInst,
- devicePort,
- deviceSlot,
- maxPortPerInst,
- maxSlotPerPort);
- if (!def->disks[diskCount]->dst) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not generate medium name for the disk "
- "at: controller instance:%u, port:%d, slot:%d"),
- deviceInst, devicePort, deviceSlot);
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
- error = true;
- break;
- }
+ disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
- gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
if (readOnly == PR_TRUE)
- def->disks[diskCount]->src->readonly = true;
+ disk->src->readonly = true;
- virDomainDiskSetType(def->disks[diskCount],
- VIR_STORAGE_TYPE_FILE);
+ virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
+ skip:
+ VBOX_UTF16_FREE(storageControllerName);
+ VBOX_UTF8_FREE(mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
VBOX_RELEASE(medium);
VBOX_RELEASE(storageController);
- diskCount++;
}
gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
- /* cleanup on error */
- if (error) {
- for (i = 0; i < def->ndisks; i++)
- VIR_FREE(def->disks[i]);
- VIR_FREE(def->disks);
- def->ndisks = 0;
- }
+ return;
+
+ error:
+ for (i = 0; i < def->ndisks; i++)
+ VIR_FREE(def->disks[i]);
+ VIR_FREE(def->disks);
+ def->ndisks = 0;
+ gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
}
static int
@@ -3934,8 +3990,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
if (vboxDumpDisplay(def, data, machine) < 0)
goto cleanup;
- vboxDumpIDEHDDs(def, data, machine);
-
+ vboxDumpStorageControllers(def, data, machine);
+ vboxDumpDisks(def, data, machine);
As an aside - it's really strange to me how GetXMLDesc works for a
running machine here - querying the live system to get everything... I'm
so used to the QEMU model to save the live definition...
John
vboxDumpSharedFolders(def, data, machine);
vboxDumpNetwork(def, data, machine, networkAdapterCount);
vboxDumpAudio(def, data, machine);
@@ -5490,8 +5546,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data,
return snapshot;
}
-static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
- virDomainSnapshotPtr snapshot)
+static int
+vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
+ virDomainSnapshotPtr snapshot)
{
virDomainPtr dom = snapshot->domain;
vboxDriverPtr data = dom->conn->privateData;
@@ -5500,9 +5557,7 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
ISnapshot *snap = NULL;
IMachine *snapMachine = NULL;
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
- int diskCount = 0;
+ size_t diskCount = 0, sdCount = 0;
nsresult rc;
vboxIID snapIid;
char *snapshotUuidStr = NULL;
@@ -5571,9 +5626,6 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
goto cleanup;
}
- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort))
- goto cleanup;
-
/* get the attachment details here */
for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
IStorageController *storageController = NULL;
@@ -5583,7 +5635,6 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
IMedium *disk = NULL;
PRUnichar *childLocUtf16 = NULL;
char *childLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
vboxArray children = VBOX_ARRAY_INITIALIZER;
@@ -5592,31 +5643,76 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
void *handle;
size_t j = 0;
size_t k = 0;
+
if (!imediumattach)
continue;
- rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+
+ rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
+ &storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium"));
+ _("cannot get controller"));
goto cleanup;
}
- if (!disk)
+
+ if (!storageControllerName)
continue;
- rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
+
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ storageControllerName,
+ &storageController);
+ VBOX_UTF16_FREE(storageControllerName);
+ if (!storageController)
+ continue;
+
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get controller"));
+ _("cannot get storage controller bus"));
+ VBOX_RELEASE(storageController);
goto cleanup;
}
- if (!storageControllerName) {
- VBOX_RELEASE(disk);
- continue;
+ rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment type"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment type"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
}
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment device"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+
+ /* skip removable disk */
+ if (!disk)
+ goto skip;
+
handle = gVBoxAPI.UArray.handleMediumGetChildren(disk);
rc = gVBoxAPI.UArray.vboxArrayGet(&children, disk, handle);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get children disk"));
+ VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
handle = gVBoxAPI.UArray.handleMediumGetSnapshotIds(disk);
@@ -5625,8 +5721,11 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get snapshot ids"));
+ VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
+
for (j = 0; j < children.count; ++j) {
IMedium *child = children.items[j];
for (k = 0; k < snapshotIids.count; ++k) {
@@ -5634,18 +5733,13 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
char *diskSnapIdStr = NULL;
VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr);
if (STREQ(diskSnapIdStr, snapshotUuidStr)) {
- rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
-
storageControllerName,
-
&storageController);
- VBOX_UTF16_FREE(storageControllerName);
- if (!storageController) {
- VBOX_RELEASE(child);
- break;
- }
rc = gVBoxAPI.UIMedium.GetLocation(child, &childLocUtf16);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get disk location"));
+ VBOX_RELEASE(child);
+ VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8);
@@ -5653,52 +5747,37 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
if (VIR_STRDUP(def->disks[diskCount].src->path, childLocUtf8)
< 0) {
VBOX_RELEASE(child);
VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
goto cleanup;
}
VBOX_UTF8_FREE(childLocUtf8);
- rc = gVBoxAPI.UIStorageController.GetBus(storageController,
&storageBus);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller
bus"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach,
&deviceType);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment
type"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach,
&devicePort);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment
type"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach,
&deviceSlot);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment
device"));
- goto cleanup;
- }
def->disks[diskCount].src->type = VIR_STORAGE_TYPE_FILE;
def->disks[diskCount].name = vboxGenerateMediumName(storageBus,
- deviceInst,
devicePort,
deviceSlot,
- maxPortPerInst,
-
maxSlotPerPort);
+ sdCount);
}
VBOX_UTF8_FREE(diskSnapIdStr);
}
}
+
+ diskCount++;
+
+ skip:
VBOX_RELEASE(storageController);
VBOX_RELEASE(disk);
- diskCount++;
+
+ if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+
}
+
gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
ret = 0;
+
cleanup:
if (ret < 0) {
for (i = 0; i < def->ndisks; i++)
@@ -5710,9 +5789,9 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
return ret;
}
-static
-int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
- virDomainSnapshotDefPtr def)
+static int
+vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def,
+ virDomainSnapshotPtr snapshot)
{
virDomainPtr dom = snapshot->domain;
vboxDriverPtr data = dom->conn->privateData;
@@ -5725,9 +5804,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
nsresult rc;
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
size_t i = 0;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
- int diskCount = 0;
+ size_t diskCount = 0, sdCount = 0;
int ret = -1;
if (!data->vboxObj)
@@ -5790,9 +5867,6 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
goto cleanup;
}
- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort))
- goto cleanup;
-
/* get the attachment details here */
for (i = 0; i < mediumAttachments.count && diskCount <
def->dom->ndisks; i++) {
PRUnichar *storageControllerName = NULL;
@@ -5805,16 +5879,17 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
IMediumAttachment *imediumattach = mediumAttachments.items[i];
+
if (!imediumattach)
continue;
+
rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot get medium"));
goto cleanup;
}
- if (!disk)
- continue;
+
rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -5834,41 +5909,85 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
VBOX_UTF16_FREE(storageControllerName);
if (!storageController)
continue;
- rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16);
+
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get disk location"));
+ _("cannot get storage controller bus"));
goto cleanup;
}
- VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
- VBOX_UTF16_FREE(mediumLocUtf16);
- if (VIR_STRDUP(def->dom->disks[diskCount]->src->path, mediumLocUtf8)
< 0)
+
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get medium attachment port"));
goto cleanup;
+ }
- VBOX_UTF8_FREE(mediumLocUtf8);
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get device"));
+ goto cleanup;
+ }
- rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+
+ rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller bus"));
+ _("cannot get medium attachment type"));
goto cleanup;
}
+
+ if (disk) {
+ rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get disk location"));
+ goto cleanup;
+ }
+ VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
+ if (VIR_STRDUP(def->dom->disks[diskCount]->src->path,
mediumLocUtf8) < 0)
+ goto cleanup;
+
+ VBOX_UTF8_FREE(mediumLocUtf8);
+
+ rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot get read only attribute"));
+ goto cleanup;
+ }
+ } else {
+ /* for removable drives, bump sdCount according to type so that
+ * device names are properly generated, and then skip it
+ */
+ if (storageBus == StorageBus_SATA ||
+ storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+
+ continue;
+ }
+
+ def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
+ devicePort,
+ deviceSlot,
+ sdCount);
if (storageBus == StorageBus_IDE) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ sdCount++;
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ sdCount++;
} else if (storageBus == StorageBus_Floppy) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
}
- rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment type"));
- goto cleanup;
- }
if (deviceType == DeviceType_HardDisk)
def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK;
else if (deviceType == DeviceType_Floppy)
@@ -5876,33 +5995,9 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
else if (deviceType == DeviceType_DVD)
def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
- rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium attachment port"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get device"));
- goto cleanup;
- }
- rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get read only attribute"));
- goto cleanup;
- }
if (readOnly == PR_TRUE)
def->dom->disks[diskCount]->src->readonly = true;
def->dom->disks[diskCount]->src->type = VIR_STORAGE_TYPE_FILE;
- def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
- deviceInst,
- devicePort,
- deviceSlot,
- maxPortPerInst,
- maxSlotPerPort);
if (!def->dom->disks[diskCount]->dst) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not generate medium name for the disk "
@@ -5995,7 +6090,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr
snapshot,
if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0)
VIR_DEBUG("Could not get read write disks for snapshot");
- if (vboxSnapshotGetReadOnlyDisks(snapshot, def) < 0)
+ if (vboxSnapshotGetReadOnlyDisks(def, snapshot) < 0)
VIR_DEBUG("Could not get Readonly disks for snapshot");
}
2591
days inactive
2601
days old
18 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Dawid Zamirski -
John Ferlan