2:35 p.m.
From: Dawid Zamirski <dzrudy(a)gmail.com>
v1: https://www.redhat.com/archives/libvir-list/2017-October/msg00381.html
Changes since v1:
* split out the original patches into smaller ones, with each bugfix or
cleanup task is in its own isolated and compilable patch
* make sure switch statements are type casted and all cases are handled
* fixed implementation of partial VM cleanup code and isolated into
separate patch (patch #3)
* squashed doc and rng updates into a single patch (#9), updated doc
wording as suggested
* addressed other minor issues (add debug statements, code formatting,
better comments, commit message spelling etc)
Dawid Zamirski (15):
vbox: Update ATTRIBUTE_UNUSED usage
vbox: Close media when undefining domains
vbox: Cleanup partially-defined VM on failure
vbox: vboxAttachDrives now relies on address info
vbox: Cleanup vboxAttachDrives implementation
vbox: Errors in vboxAttachDrives are now critical
vbox: Support empty removable drives.
vbox: Add more IStorageController API mappings
domain: Allow 'model' attribute for ide controller
vbox: Process <controller> element in domain XML
vbox: Add vboxDumpStorageControllers
vbox: Correctly generate drive name in dumpxml
vbox: Cleanup vboxDumpDisks implementation
vbox: Process empty removable disks in dumpxml
vbox: Generate disk address element in dumpxml
docs/formatdomain.html.in | 4 +
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 | 1422 +++++++++++++++++++++++++----------------
src/vbox/vbox_common.h | 21 +
src/vbox/vbox_tmpl.c | 93 ++-
src/vbox/vbox_uniformed_api.h | 3 +
9 files changed, 983 insertions(+), 598 deletions(-)
--
2.14.2
2:35 p.m.
New subject: [libvirt] [PATCH v2 01/15] vbox: Update ATTRIBUTE_UNUSED usage
Since the removal of VBOX <= 3x, the function arguments are actually
used so they should not be marked with ATTRIBUTE_UNUSED anymore.
---
src/vbox/vbox_tmpl.c | 49 +++++++++++++++++++++++--------------------------
1 file changed, 23 insertions(+), 26 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index dffeabde0..4aa5e9a63 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -665,7 +665,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*/
}
@@ -676,38 +678,33 @@ _virtualboxRegisterMachine(IVirtualBox *vboxObj, IMachine *machine)
}
static nsresult
-_virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location,
- PRUint32 deviceType ATTRIBUTE_UNUSED,
+_virtualboxFindHardDisk(IVirtualBox *vboxObj,
+ PRUnichar *location,
+ 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
}
@@ -759,12 +756,12 @@ _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
8:31 a.m.
New subject: [libvirt] [PATCH v2 01/15] vbox: Update ATTRIBUTE_UNUSED usage
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Since the removal of VBOX <= 3x, the function arguments are
actually
used so they should not be marked with ATTRIBUTE_UNUSED anymore.
---
src/vbox/vbox_tmpl.c | 49 +++++++++++++++++++++++--------------------------
1 file changed, 23 insertions(+), 26 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[I can push this before a v3 would be necessary - more later]
2:35 p.m.
New subject: [libvirt] [PATCH v2 02/15] vbox: Close media when undefining domains
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 4aa5e9a63..2679b60f7 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 */
+ ignore_value(medium->vtbl->Close(medium));
+ }
+
+ cleanup:
vboxArrayUnalloc(&media);
return rc;
}
--
2.14.2
8:32 a.m.
New subject: [libvirt] [PATCH v2 02/15] vbox: Close media when undefining domains
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
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(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[I can push this before a v3 would be necessary - more later]
2:35 p.m.
New subject: [libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure
Since the VBOX API requires to register an initial VM before proceeding
to attach any remaining devices to it, any failure to attach such
devices should result in automatic cleanup of the initially registered
VM so that the state of VBOX registry remains clean without any leftover
"aborted" VMs in it. Failure to cleanup of such partial VMs results in a
warning log so that actual define error stays on the top of the error
stack.
---
src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee37164..812c940e6 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1853,6 +1853,8 @@ 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;
+ bool machineReady = false;
+
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
if (!data->vboxObj)
return ret;
- VBOX_IID_INITIALIZE(&mchiid);
if (!(def = virDomainDefParseString(xml, data->caps, data->xmlopt,
NULL, parse_flags))) {
- goto cleanup;
+ return ret;
}
+ VBOX_IID_INITIALIZE(&mchiid);
virUUIDFormat(def->uuid, uuidstr);
rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr);
@@ -1959,30 +1961,41 @@ 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.
+ machineReady = true;
+
+ cleanup:
+ /* Save the machine settings made till now, even when jumped here on error,
+ * as otherwise unregister won't cleanup properly. For example, it won't
+ * close media that were partially attached. The VBOX SDK docs say that
+ * unregister implicitly calls saveSettings but evidently it's not so...
*/
rc = gVBoxAPI.UIMachine.SaveSettings(machine);
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"), rc);
+ machineReady = false;
}
gVBoxAPI.UISession.Close(data->vboxSession);
- vboxIIDUnalloc(&mchiid);
-
- ret = virGetDomain(conn, def->name, def->uuid, -1);
- VBOX_RELEASE(machine);
- virDomainDefFree(def);
+ if (machineReady) {
+ ret = virGetDomain(conn, def->name, def->uuid, -1);
+ } else {
+ /* Unregister incompletely configured VM to not leave garbage behind */
+ rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
- return ret;
+ if (NS_SUCCEEDED(rc))
+ gVBoxAPI.deleteConfig(machine);
+ else
+ VIR_WARN("Could not cleanup partially created VM after failure, "
+ "rc=%08x", rc);
+ }
- cleanup:
+ vboxIIDUnalloc(&mchiid);
VBOX_RELEASE(machine);
virDomainDefFree(def);
- return NULL;
+
+ return ret;
}
static virDomainPtr
--
2.14.2
11:50 a.m.
New subject: [libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure
On Tue, 2017-10-24 at 15:35 -0400, Dawid Zamirski wrote:
Since the VBOX API requires to register an initial VM before
proceeding
to attach any remaining devices to it, any failure to attach such
devices should result in automatic cleanup of the initially
registered
VM so that the state of VBOX registry remains clean without any
leftover
"aborted" VMs in it. Failure to cleanup of such partial VMs results
in a
warning log so that actual define error stays on the top of the error
stack.
---
src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++-------------
-
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee37164..812c940e6 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1853,6 +1853,8 @@ 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;
+ bool machineReady = false;
+
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
const char *xml, unsigned int flags
if (!data->vboxObj)
return ret;
- VBOX_IID_INITIALIZE(&mchiid);
if (!(def = virDomainDefParseString(xml, data->caps, data-
>xmlopt,
NULL, parse_flags))) {
- goto cleanup;
+ return ret;
}
+ VBOX_IID_INITIALIZE(&mchiid);
virUUIDFormat(def->uuid, uuidstr);
rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine,
uuidstr);
@@ -1959,30 +1961,41 @@ 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.
+ machineReady = true;
+
+ cleanup:
+ /* Save the machine settings made till now, even when jumped
here on error,
+ * as otherwise unregister won't cleanup properly. For example,
it won't
+ * close media that were partially attached. The VBOX SDK docs
say that
+ * unregister implicitly calls saveSettings but evidently it's
not so...
*/
rc = gVBoxAPI.UIMachine.SaveSettings(machine);
There's one more code path in this function that causes a segfault here
due to machine == NULL (e.g. when CreateMachine fails). I already have
patched this but I'll hold off with sending V3 until this series is
reviewed and feedback is available.
8:41 a.m.
New subject: [libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Since the VBOX API requires to register an initial VM before
proceeding
to attach any remaining devices to it, any failure to attach such
devices should result in automatic cleanup of the initially registered
VM so that the state of VBOX registry remains clean without any leftover
"aborted" VMs in it. Failure to cleanup of such partial VMs results in a
warning log so that actual define error stays on the top of the error
stack.
---
src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee37164..812c940e6 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1853,6 +1853,8 @@ 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;
+ bool machineReady = false;
+
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
if (!data->vboxObj)
return ret;
- VBOX_IID_INITIALIZE(&mchiid);
if (!(def = virDomainDefParseString(xml, data->caps, data->xmlopt,
NULL, parse_flags))) {
- goto cleanup;
+ return ret;
}
Existing, but the { } here aren't necessary since there's only one
statement after the if. It escapes the syntax-check check because
there's two lines in the function call...
+ VBOX_IID_INITIALIZE(&mchiid);
virUUIDFormat(def->uuid, uuidstr);
rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr);
And as I assume you know - having this go to cleanup is going to cause a
problem since it's designed to clean up on failure assuming that this
succeeded, right?
I would have been OK with a posted diff to squash in...
In any case, at this point only @def and @mchiid would need to be
cleaned up. If you move the mchiid generation to after this line, then
the failure scenario here could be virDomainDefFree(def) and return ret;
(or NULL).
Whichever way works - the rest of the code looks OK, but I can wait for
the next version.
Since Patches 1 & 2 are OK and for the most part Patches 4-9 are fine
and would seem to be "movable" to just before current patch 10. That
patch also adjusts vboxDomainDefineXMLFlags, so for me it became a logic
point to at least be able to push some of the patches before you post a
a v3. For the nits found in patch 4-9 - I can make the simple
adjustments noted before pushing. That way the next series is smaller.
John
@@ -1959,30 +1961,41 @@ 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.
+ machineReady = true;
+
+ cleanup:
+ /* Save the machine settings made till now, even when jumped here on error,
+ * as otherwise unregister won't cleanup properly. For example, it won't
+ * close media that were partially attached. The VBOX SDK docs say that
+ * unregister implicitly calls saveSettings but evidently it's not so...
*/
rc = gVBoxAPI.UIMachine.SaveSettings(machine);
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"), rc);
+ machineReady = false;
}
gVBoxAPI.UISession.Close(data->vboxSession);
- vboxIIDUnalloc(&mchiid);
-
- ret = virGetDomain(conn, def->name, def->uuid, -1);
- VBOX_RELEASE(machine);
- virDomainDefFree(def);
+ if (machineReady) {
+ ret = virGetDomain(conn, def->name, def->uuid, -1);
+ } else {
+ /* Unregister incompletely configured VM to not leave garbage behind */
+ rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
- return ret;
+ if (NS_SUCCEEDED(rc))
+ gVBoxAPI.deleteConfig(machine);
+ else
+ VIR_WARN("Could not cleanup partially created VM after failure, "
+ "rc=%08x", rc);
+ }
- cleanup:
+ vboxIIDUnalloc(&mchiid);
VBOX_RELEASE(machine);
virDomainDefFree(def);
- return NULL;
+
+ return ret;
}
static virDomainPtr
2:35 p.m.
New subject: [libvirt] [PATCH v2 04/15] vbox: vboxAttachDrives now relies on address info
Previously, the driver was computing VBOX's devicePort/deviceSlot values
based on device name and max port/slot values. While this worked, it
completely ignored <address> values. Additionally, libvirt's built-in
virDomainDiskDefAssignAddress already does a good job setting default
values on virDomainDeviceDriveAddress struct which we can use to set
devicePort and deviceSlot and accomplish the same result while allowing
the cusomizing those via XML. Also, this allows to remove some code
which will make further patches smaller.
---
src/vbox/vbox_common.c | 104 ++++---------------------------------------------
1 file changed, 7 insertions(+), 97 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 812c940e6..fa8471e68 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -346,68 +346,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
@@ -1022,14 +960,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
{
size_t i;
nsresult rc = 0;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
PRUnichar *storageCtlName = NULL;
- bool error = false;
-
- /* 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
@@ -1071,7 +1002,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
VBOX_RELEASE(storageCtl);
}
- for (i = 0; i < def->ndisks && !error; i++) {
+ for (i = 0; i < def->ndisks; i++) {
const char *src = virDomainDiskGetSource(def->disks[i]);
int type = virDomainDiskGetType(def->disks[i]);
int format = virDomainDiskGetFormat(def->disks[i]);
@@ -1095,12 +1026,10 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
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;
+ PRInt32 devicePort = def->disks[i]->info.addr.drive.unit;
+ PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus;
VBOX_IID_INITIALIZE(&mediumUUID);
VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
@@ -1166,35 +1095,16 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
- storageBus = StorageBus_IDE;
+ devicePort = def->disks[i]->info.addr.drive.bus;
+ deviceSlot = def->disks[i]->info.addr.drive.unit;
} 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;
- }
-
- /* 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;
+ devicePort = 0;
+ deviceSlot = def->disks[i]->info.addr.drive.unit;
}
/* attach the harddisk/dvd/Floppy to the storage controller */
--
2.14.2
8:42 a.m.
New subject: [libvirt] [PATCH v2 04/15] vbox: vboxAttachDrives now relies on address info
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Previously, the driver was computing VBOX's devicePort/deviceSlot
values
based on device name and max port/slot values. While this worked, it
completely ignored <address> values. Additionally, libvirt's built-in
virDomainDiskDefAssignAddress already does a good job setting default
values on virDomainDeviceDriveAddress struct which we can use to set
devicePort and deviceSlot and accomplish the same result while allowing
the cusomizing those via XML. Also, this allows to remove some code
customizing
which will make further patches smaller.
---
src/vbox/vbox_common.c | 104 ++++---------------------------------------------
1 file changed, 7 insertions(+), 97 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:35 p.m.
New subject: [libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation
This commit primes vboxAttachDrives for further changes so when they
are made, the diff is less noisy:
* move variable declarations to the top of the function
* add disk variable to replace all the def->disks[i] instances
* add cleanup at the end of the loop body, so it's all in one place
rather than scattered through the loop body. It's purposefully
called 'cleanup' rather than 'skip' or 'continue' because
future
commit will treat errors as hard-failures.
---
src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++--------------------------
1 file changed, 46 insertions(+), 49 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index fa8471e68..b949c4db7 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -959,8 +959,17 @@ static void
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
+ int type, format;
+ const char *src = NULL;
nsresult rc = 0;
+ virDomainDiskDefPtr disk = NULL;
PRUnichar *storageCtlName = NULL;
+ IMedium *medium = NULL;
+ PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
+ PRUint32 devicePort, deviceSlot, deviceType, accessMode;
+ vboxIID mediumUUID;
+
+ VBOX_IID_INITIALIZE(&mediumUUID);
/* add a storage controller for the mediums to be attached */
/* this needs to change when multiple controller are supported for
@@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
}
for (i = 0; i < def->ndisks; i++) {
- const char *src = virDomainDiskGetSource(def->disks[i]);
- int type = virDomainDiskGetType(def->disks[i]);
- int format = virDomainDiskGetFormat(def->disks[i]);
+ disk = def->disks[i];
+ src = virDomainDiskGetSource(disk);
+ type = virDomainDiskGetType(disk);
+ format = virDomainDiskGetFormat(disk);
+ deviceType = DeviceType_Null;
+ accessMode = AccessMode_ReadOnly;
+ devicePort = disk->info.addr.drive.unit;
+ deviceSlot = disk->info.addr.drive.bus;
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) device: %d", i, disk->device);
+ VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus);
VIR_DEBUG("disk(%zu) src: %s", i, src);
- VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst);
+ VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst);
VIR_DEBUG("disk(%zu) driverName: %s", i,
- virDomainDiskGetDriver(def->disks[i]));
+ virDomainDiskGetDriver(disk));
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
+ VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode);
+ VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src->readonly
? "True" : "False"));
- VIR_DEBUG("disk(%zu) shared: %s", i,
(def->disks[i]->src->shared
+ VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src->shared
? "True" : "False"));
if (type == VIR_STORAGE_TYPE_FILE && src) {
- IMedium *medium = NULL;
- vboxIID mediumUUID;
- PRUnichar *mediumFileUtf16 = NULL;
- PRUint32 deviceType = DeviceType_Null;
- PRUint32 accessMode = AccessMode_ReadOnly;
- PRInt32 devicePort = def->disks[i]->info.addr.drive.unit;
- PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus;
-
- VBOX_IID_INITIALIZE(&mediumUUID);
VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
deviceType = DeviceType_HardDisk;
accessMode = AccessMode_ReadWrite;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
deviceType = DeviceType_DVD;
accessMode = AccessMode_ReadOnly;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
deviceType = DeviceType_Floppy;
accessMode = AccessMode_ReadWrite;
} else {
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ goto cleanup;
}
gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
deviceType, accessMode, &medium);
if (!medium) {
- PRUnichar *mediumEmpty = NULL;
-
VBOX_UTF8_TO_UTF16("", &mediumEmpty);
rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
@@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (!medium) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to attach the following disk/dvd/floppy
"
- "to the machine: %s, rc=%08x"),
- src, (unsigned)rc);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ "to the machine: %s, rc=%08x"), src, rc);
+ 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;
+ src, rc);
+ goto cleanup;
}
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (def->disks[i]->src->readonly) {
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (disk->src->readonly) {
gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
- VIR_DEBUG("setting harddisk to immutable");
- } else if (!def->disks[i]->src->readonly) {
+ VIR_DEBUG("Setting harddisk to immutable");
+ } else if (!disk->src->readonly) {
gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
- VIR_DEBUG("setting harddisk type to normal");
+ VIR_DEBUG("Setting harddisk type to normal");
}
}
- if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) {
VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
devicePort = def->disks[i]->info.addr.drive.bus;
deviceSlot = def->disks[i]->info.addr.drive.unit;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) {
+ } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) {
VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) {
+ } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
devicePort = 0;
- deviceSlot = def->disks[i]->info.addr.drive.unit;
+ deviceSlot = disk->info.addr.drive.unit;
}
/* attach the harddisk/dvd/Floppy to the storage controller */
@@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not attach the file as "
- "harddisk/dvd/floppy: %s, rc=%08x"),
- src, (unsigned)rc);
+ _("Could not attach the file as "
+ "harddisk/dvd/floppy: %s, rc=%08x"), src,
rc);
} else {
DEBUGIID("Attached HDD/DVD/Floppy with UUID",
&mediumUUID);
}
-
+ cleanup:
VBOX_MEDIUM_RELEASE(medium);
vboxIIDUnalloc(&mediumUUID);
VBOX_UTF16_FREE(mediumFileUtf16);
--
2.14.2
8:43 a.m.
New subject: [libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This commit primes vboxAttachDrives for further changes so when they
are made, the diff is less noisy:
* move variable declarations to the top of the function
* add disk variable to replace all the def->disks[i] instances
* add cleanup at the end of the loop body, so it's all in one place
rather than scattered through the loop body. It's purposefully
called 'cleanup' rather than 'skip' or 'continue' because
future
commit will treat errors as hard-failures.
---
src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++--------------------------
1 file changed, 46 insertions(+), 49 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index fa8471e68..b949c4db7 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -959,8 +959,17 @@ static void
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
+ int type, format;
+ const char *src = NULL;
nsresult rc = 0;
+ virDomainDiskDefPtr disk = NULL;
PRUnichar *storageCtlName = NULL;
+ IMedium *medium = NULL;
+ PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
+ PRUint32 devicePort, deviceSlot, deviceType, accessMode;
+ vboxIID mediumUUID;
+
+ VBOX_IID_INITIALIZE(&mediumUUID);
The cleanup: label would clean this up on the first pass through the
loop, so it needs to move outside the last }.
I can do this for you; however, I have a question - something I noticed
while reviewing patch 7. There's a call:
rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
for which it wasn't clear whether the mediumUUID is overwritten. If it
is, then although existing - it probably needs some cleanup and of
course would change the flow a bit to doing that initializer each time
through the loop.
Moving it does have a ripple effect on subsequent patches, but it's
manageable.
If moving it to the end is acceptable, then
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
otherwise let me know and I'll let you repost as part of a v3
John
/* add a storage controller for the mediums to be attached */
/* this needs to change when multiple controller are supported for
@@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data,
IMachine *machine)
}
for (i = 0; i < def->ndisks; i++) {
- const char *src = virDomainDiskGetSource(def->disks[i]);
- int type = virDomainDiskGetType(def->disks[i]);
- int format = virDomainDiskGetFormat(def->disks[i]);
+ disk = def->disks[i];
+ src = virDomainDiskGetSource(disk);
+ type = virDomainDiskGetType(disk);
+ format = virDomainDiskGetFormat(disk);
+ deviceType = DeviceType_Null;
+ accessMode = AccessMode_ReadOnly;
+ devicePort = disk->info.addr.drive.unit;
+ deviceSlot = disk->info.addr.drive.bus;
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) device: %d", i, disk->device);
+ VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus);
VIR_DEBUG("disk(%zu) src: %s", i, src);
- VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst);
+ VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst);
VIR_DEBUG("disk(%zu) driverName: %s", i,
- virDomainDiskGetDriver(def->disks[i]));
+ virDomainDiskGetDriver(disk));
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
+ VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode);
+ VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src->readonly
? "True" : "False"));
- VIR_DEBUG("disk(%zu) shared: %s", i,
(def->disks[i]->src->shared
+ VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src->shared
? "True" : "False"));
if (type == VIR_STORAGE_TYPE_FILE && src) {
- IMedium *medium = NULL;
- vboxIID mediumUUID;
- PRUnichar *mediumFileUtf16 = NULL;
- PRUint32 deviceType = DeviceType_Null;
- PRUint32 accessMode = AccessMode_ReadOnly;
- PRInt32 devicePort = def->disks[i]->info.addr.drive.unit;
- PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus;
-
- VBOX_IID_INITIALIZE(&mediumUUID);
VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
deviceType = DeviceType_HardDisk;
accessMode = AccessMode_ReadWrite;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
deviceType = DeviceType_DVD;
accessMode = AccessMode_ReadOnly;
- } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
deviceType = DeviceType_Floppy;
accessMode = AccessMode_ReadWrite;
} else {
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ goto cleanup;
}
gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
deviceType, accessMode, &medium);
if (!medium) {
- PRUnichar *mediumEmpty = NULL;
-
VBOX_UTF8_TO_UTF16("", &mediumEmpty);
rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
@@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data,
IMachine *machine)
if (!medium) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to attach the following disk/dvd/floppy
"
- "to the machine: %s, rc=%08x"),
- src, (unsigned)rc);
- VBOX_UTF16_FREE(mediumFileUtf16);
- continue;
+ "to the machine: %s, rc=%08x"), src, rc);
+ 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;
+ src, rc);
+ goto cleanup;
}
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (def->disks[i]->src->readonly) {
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (disk->src->readonly) {
gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
- VIR_DEBUG("setting harddisk to immutable");
- } else if (!def->disks[i]->src->readonly) {
+ VIR_DEBUG("Setting harddisk to immutable");
+ } else if (!disk->src->readonly) {
gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
- VIR_DEBUG("setting harddisk type to normal");
+ VIR_DEBUG("Setting harddisk type to normal");
}
}
- if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) {
VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
devicePort = def->disks[i]->info.addr.drive.bus;
deviceSlot = def->disks[i]->info.addr.drive.unit;
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) {
+ } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) {
VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
- } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) {
+ } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
devicePort = 0;
- deviceSlot = def->disks[i]->info.addr.drive.unit;
+ deviceSlot = disk->info.addr.drive.unit;
}
/* attach the harddisk/dvd/Floppy to the storage controller */
@@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data,
IMachine *machine)
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not attach the file as "
- "harddisk/dvd/floppy: %s, rc=%08x"),
- src, (unsigned)rc);
+ _("Could not attach the file as "
+ "harddisk/dvd/floppy: %s, rc=%08x"), src,
rc);
} else {
DEBUGIID("Attached HDD/DVD/Floppy with UUID",
&mediumUUID);
}
-
+ cleanup:
VBOX_MEDIUM_RELEASE(medium);
vboxIIDUnalloc(&mediumUUID);
VBOX_UTF16_FREE(mediumFileUtf16);
11:19 a.m.
New subject: [libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation
On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> This commit primes vboxAttachDrives for further changes so when
> they
> are made, the diff is less noisy:
>
> * move variable declarations to the top of the function
> * add disk variable to replace all the def->disks[i] instances
> * add cleanup at the end of the loop body, so it's all in one place
> rather than scattered through the loop body. It's purposefully
> called 'cleanup' rather than 'skip' or 'continue' because
future
> commit will treat errors as hard-failures.
> ---
> src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++------------
> --------------
> 1 file changed, 46 insertions(+), 49 deletions(-)
>
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index fa8471e68..b949c4db7 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -959,8 +959,17 @@ static void
> vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> *machine)
> {
> size_t i;
> + int type, format;
> + const char *src = NULL;
> nsresult rc = 0;
> + virDomainDiskDefPtr disk = NULL;
> PRUnichar *storageCtlName = NULL;
> + IMedium *medium = NULL;
> + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
> + PRUint32 devicePort, deviceSlot, deviceType, accessMode;
> + vboxIID mediumUUID;
> +
> + VBOX_IID_INITIALIZE(&mediumUUID);
The cleanup: label would clean this up on the first pass through the
loop, so it needs to move outside the last }.
The VBOX_IID_INITIALIZE macro is a simple struct initializer that is
equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact,
src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly
that, but that macro is not available to vbox_common.c This is likely
another remanent of VBOX <= 3 support that needs to be cleaned up - I
will do so in a separate patch as it will touch to many parts of the
driver and when you take a closer look we don't really need vboxIID
struct at all and can simply use PRUnichar directly for VBOX UUIDs.
I can do this for you; however, I have a question - something I
noticed
while reviewing patch 7. There's a call:
rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
for which it wasn't clear whether the mediumUUID is overwritten. If
it
is, then although existing - it probably needs some cleanup and of
course would change the flow a bit to doing that initializer each
time
through the loop.
Yes, it is overwritten but since the cleanup: label is at the end of
the loop body it will set mediumUIID to {NULL, true} which is the same
as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in
src/vbox/vbox_tmpl.c)
Moving it does have a ripple effect on subsequent patches, but it's
manageable.
If moving it to the end is acceptable, then
As per above, I'd rather leave this patch "as is" as the (future)
driver cleanup patches for vboxIID will make this clearer.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
otherwise let me know and I'll let you repost as part of a v3
John
>
> /* add a storage controller for the mediums to be attached */
> /* this needs to change when multiple controller are supported
> for
> @@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> }
>
> for (i = 0; i < def->ndisks; i++) {
> - const char *src = virDomainDiskGetSource(def->disks[i]);
> - int type = virDomainDiskGetType(def->disks[i]);
> - int format = virDomainDiskGetFormat(def->disks[i]);
> + disk = def->disks[i];
> + src = virDomainDiskGetSource(disk);
> + type = virDomainDiskGetType(disk);
> + format = virDomainDiskGetFormat(disk);
> + deviceType = DeviceType_Null;
> + accessMode = AccessMode_ReadOnly;
> + devicePort = disk->info.addr.drive.unit;
> + deviceSlot = disk->info.addr.drive.bus;
>
> 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) device: %d", i, disk->device);
> + VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus);
> VIR_DEBUG("disk(%zu) src: %s", i, src);
> - VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]-
> >dst);once vboxIID will (probably) be removed in separate patches
> at some point in the future, I'd rather leave this patch "as is"
> for now.
> + VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst);
> VIR_DEBUG("disk(%zu) driverName: %s", i,
> - virDomainDiskGetDriver(def->disks[i]));
> + virDomainDiskGetDriver(disk));
> 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
> + VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode);
> + VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src-
> >readonly
> ? "True" :
"False"));
> - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]-
> >src->shared
> + VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src-
> >shared
> ? "True" :
"False"));
>
> if (type == VIR_STORAGE_TYPE_FILE && src) {
> - IMedium *medium = NULL;
> - vboxIID mediumUUID;
> - PRUnichar *mediumFileUtf16 = NULL;
> - PRUint32 deviceType = DeviceType_Null;
> - PRUint32 accessMode = AccessMode_ReadOnly;
> - PRInt32 devicePort = def->disks[i]-
> >info.addr.drive.unit;
> - PRInt32 deviceSlot = def->disks[i]-
> >info.addr.drive.bus;
> -
> - VBOX_IID_INITIALIZE(&mediumUUID);
> VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
>
> - if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_DISK) {
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> deviceType = DeviceType_HardDisk;
> accessMode = AccessMode_ReadWrite;
> - } else if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_CDROM) {
> + } else if (disk->device ==
> VIR_DOMAIN_DISK_DEVICE_CDROM) {
> deviceType = DeviceType_DVD;
> accessMode = AccessMode_ReadOnly;
> - } else if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> + } else if (disk->device ==
> VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> deviceType = DeviceType_Floppy;
> accessMode = AccessMode_ReadWrite;
> } else {
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - continue;
> + goto cleanup;
> }
>
> gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj,
> mediumFileUtf16,
> deviceType,
> accessMode, &medium);
>
> if (!medium) {
> - PRUnichar *mediumEmpty = NULL;
> -
> VBOX_UTF8_TO_UTF16("", &mediumEmpty);
>
> rc = gVBoxAPI.UIVirtualBox.OpenMedium(data-
> >vboxObj,
> @@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> if (!medium) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to attach the following
> disk/dvd/floppy "
> - "to the machine: %s, rc=%08x"),
> - src, (unsigned)rc);
> - VBOX_UTF16_FREE(mediumFileUtf16);
> - continue;
> + "to the machine: %s, rc=%08x"),
> src, rc);
> + 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;
> + src, rc);
> + goto cleanup;
> }
>
> - if (def->disks[i]->device ==
> VIR_DOMAIN_DISK_DEVICE_DISK) {
> - if (def->disks[i]->src->readonly) {
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> + if (disk->src->readonly) {
> gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Immutable);
> - VIR_DEBUG("setting harddisk to immutable");
> - } else if (!def->disks[i]->src->readonly) {
> + VIR_DEBUG("Setting harddisk to immutable");
> + } else if (!disk->src->readonly) {
> gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Normal);
> - VIR_DEBUG("setting harddisk type to normal");
> + VIR_DEBUG("Setting harddisk type to normal");
> }
> }
>
> - if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> VBOX_UTF8_TO_UTF16("IDE Controller",
> &storageCtlName);
> devicePort = def->disks[i]->info.addr.drive.bus;
> deviceSlot = def->disks[i]->info.addr.drive.unit;
> - } else if (def->disks[i]->bus ==
> VIR_DOMAIN_DISK_BUS_SATA) {
> + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) {
> VBOX_UTF8_TO_UTF16("SATA Controller",
> &storageCtlName);
> - } else if (def->disks[i]->bus ==
> VIR_DOMAIN_DISK_BUS_SCSI) {
> + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
> VBOX_UTF8_TO_UTF16("SCSI Controller",
> &storageCtlName);
> - } else if (def->disks[i]->bus ==
> VIR_DOMAIN_DISK_BUS_FDC) {
> + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
> VBOX_UTF8_TO_UTF16("Floppy Controller",
> &storageCtlName);
> devicePort = 0;
> - deviceSlot = def->disks[i]->info.addr.drive.unit;
> + deviceSlot = disk->info.addr.drive.unit;
> }
>
> /* attach the harddisk/dvd/Floppy to the storage
> controller */
> @@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
>
> if (NS_FAILED(rc)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("could not attach the file as "
> - "harddisk/dvd/floppy: %s,
> rc=%08x"),
> - src, (unsigned)rc);
> + _("Could not attach the file as "
> + "harddisk/dvd/floppy: %s,
> rc=%08x"), src, rc);
> } else {
> DEBUGIID("Attached HDD/DVD/Floppy with UUID",
> &mediumUUID);
> }
> -
> + cleanup:
> VBOX_MEDIUM_RELEASE(medium);
> vboxIIDUnalloc(&mediumUUID);
> VBOX_UTF16_FREE(mediumFileUtf16);
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:46 p.m.
New subject: [libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation
On 11/03/2017 12:19 PM, Dawid Zamirski wrote:
On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
>
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
>> This commit primes vboxAttachDrives for further changes so when
>> they
>> are made, the diff is less noisy:
>>
>> * move variable declarations to the top of the function
>> * add disk variable to replace all the def->disks[i] instances
>> * add cleanup at the end of the loop body, so it's all in one place
>> rather than scattered through the loop body. It's purposefully
>> called 'cleanup' rather than 'skip' or 'continue'
because future
>> commit will treat errors as hard-failures.
>> ---
>> src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++------------
>> --------------
>> 1 file changed, 46 insertions(+), 49 deletions(-)
>>
>> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
>> index fa8471e68..b949c4db7 100644
>> --- a/src/vbox/vbox_common.c
>> +++ b/src/vbox/vbox_common.c
>> @@ -959,8 +959,17 @@ static void
>> vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
>> *machine)
>> {
>> size_t i;
>> + int type, format;
>> + const char *src = NULL;
>> nsresult rc = 0;
>> + virDomainDiskDefPtr disk = NULL;
>> PRUnichar *storageCtlName = NULL;
>> + IMedium *medium = NULL;
>> + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
>> + PRUint32 devicePort, deviceSlot, deviceType, accessMode;
>> + vboxIID mediumUUID;
>> +
>> + VBOX_IID_INITIALIZE(&mediumUUID);
>
> The cleanup: label would clean this up on the first pass through the
> loop, so it needs to move outside the last }.
>
The VBOX_IID_INITIALIZE macro is a simple struct initializer that is
equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact,
src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly
that, but that macro is not available to vbox_common.c This is likely
another remanent of VBOX <= 3 support that needs to be cleaned up - I
will do so in a separate patch as it will touch to many parts of the
driver and when you take a closer look we don't really need vboxIID
struct at all and can simply use PRUnichar directly for VBOX UUIDs.
OK it really wasn't overly intuitively obvious what VBOX_IID_INITIALIZE
a/k/a gVBoxAPI.UIID.vboxIIDInitialize really did, your explanation makes
sense.
> I can do this for you; however, I have a question - something I
> noticed
> while reviewing patch 7. There's a call:
>
> rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
>
> for which it wasn't clear whether the mediumUUID is overwritten. If
> it
> is, then although existing - it probably needs some cleanup and of
> course would change the flow a bit to doing that initializer each
> time
> through the loop.
Yes, it is overwritten but since the cleanup: label is at the end of
the loop body it will set mediumUIID to {NULL, true} which is the same
as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in
src/vbox/vbox_tmpl.c)
>
> Moving it does have a ripple effect on subsequent patches, but it's
> manageable.
>
> If moving it to the end is acceptable, then
As per above, I'd rather leave this patch "as is" as the (future)
driver cleanup patches for vboxIID will make this clearer.
OK I'll leave it as is. Patches 1-2 and 4-9 are thus all R-B and will
be pushed in a little while.
John
>
> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
>
> otherwise let me know and I'll let you repost as part of a v3
>
> John
>
[...]
2:35 p.m.
New subject: [libvirt] [PATCH v2 06/15] vbox: Errors in vboxAttachDrives are now critical
Previously, if one tried to define a VBOX VM and the API failed to
perform the requested actions for some reason, it would just log the
error and move on to process remaining disk definitions. This is not
desired as it could result in incorrectly defined VM without the caller
even knowing about it. So now all the code paths that call
virReportError are now treated as hard failures as they should have
been.
---
src/vbox/vbox_common.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index b949c4db7..9f4bf18a3 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -955,17 +955,17 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data,
}
}
-static void
+static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
- int type, format;
+ int type, format, ret = 0;
const char *src = NULL;
nsresult rc = 0;
virDomainDiskDefPtr disk = NULL;
PRUnichar *storageCtlName = NULL;
IMedium *medium = NULL;
- PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
+ PRUnichar *mediumFileUtf16 = NULL;
PRUint32 devicePort, deviceSlot, deviceType, accessMode;
vboxIID mediumUUID;
@@ -1049,6 +1049,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
deviceType = DeviceType_Floppy;
accessMode = AccessMode_ReadWrite;
} else {
+ ret = -1;
goto cleanup;
}
@@ -1056,19 +1057,17 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
deviceType, accessMode, &medium);
if (!medium) {
- VBOX_UTF8_TO_UTF16("", &mediumEmpty);
-
rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
mediumFileUtf16,
deviceType, accessMode,
&medium);
- VBOX_UTF16_FREE(mediumEmpty);
}
if (!medium) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to attach the following disk/dvd/floppy
"
"to the machine: %s, rc=%08x"), src, rc);
+ ret = -1;
goto cleanup;
}
@@ -1078,6 +1077,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
_("Can't get the UUID of the file to be attached
"
"as harddisk/dvd/floppy: %s, rc=%08x"),
src, rc);
+ ret = -1;
goto cleanup;
}
@@ -1117,6 +1117,8 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not attach the file as "
"harddisk/dvd/floppy: %s, rc=%08x"), src,
rc);
+ ret = -1;
+ goto cleanup;
} else {
DEBUGIID("Attached HDD/DVD/Floppy with UUID",
&mediumUUID);
}
@@ -1125,8 +1127,13 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
vboxIIDUnalloc(&mediumUUID);
VBOX_UTF16_FREE(mediumFileUtf16);
VBOX_UTF16_FREE(storageCtlName);
+
+ if (ret < 0)
+ break;
}
}
+
+ return ret;
}
static void
@@ -1857,7 +1864,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
vboxSetBootDeviceOrder(def, data, machine);
- vboxAttachDrives(def, data, machine);
+ if (vboxAttachDrives(def, data, machine) < 0)
+ goto cleanup;
vboxAttachSound(def, machine);
if (vboxAttachNetwork(def, data, machine) < 0)
goto cleanup;
--
2.14.2
8:44 a.m.
New subject: [libvirt] [PATCH v2 06/15] vbox: Errors in vboxAttachDrives are now critical
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Previously, if one tried to define a VBOX VM and the API failed to
perform the requested actions for some reason, it would just log the
error and move on to process remaining disk definitions. This is not
desired as it could result in incorrectly defined VM without the caller
even knowing about it. So now all the code paths that call
virReportError are now treated as hard failures as they should have
been.
---
src/vbox/vbox_common.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index b949c4db7..9f4bf18a3 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -955,17 +955,17 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data,
}
}
-static void
+static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
- int type, format;
+ int type, format, ret = 0;
const char *src = NULL;
nsresult rc = 0;
virDomainDiskDefPtr disk = NULL;
PRUnichar *storageCtlName = NULL;
IMedium *medium = NULL;
- PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
+ PRUnichar *mediumFileUtf16 = NULL;
PRUint32 devicePort, deviceSlot, deviceType, accessMode;
vboxIID mediumUUID;
@@ -1049,6 +1049,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
deviceType = DeviceType_Floppy;
accessMode = AccessMode_ReadWrite;
} else {
+ ret = -1;
goto cleanup;
}
@@ -1056,19 +1057,17 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data,
IMachine *machine)
deviceType, accessMode, &medium);
if (!medium) {
- VBOX_UTF8_TO_UTF16("", &mediumEmpty);
-
rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
mediumFileUtf16,
deviceType, accessMode,
&medium);
- VBOX_UTF16_FREE(mediumEmpty);
}
The mediumEmpty changes could have gone into the previous patch or in a
separate patch. It's unused as of commit 34364df3 which should have
never copied it in from the "old" code which ended up getting removed as
part of commit c7c286c6.
I can extract it into a separate patch before pushing...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]
2:35 p.m.
New subject: [libvirt] [PATCH v2 07/15] vbox: Support empty removable drives.
Original code was checking for non empty disk source before proceeding
to actually attach disk device to VM. This prevented from creating
empty removable devices like DVD or floppy. Therefore, this patch
re-organizes the loop work-flow to allow such configurations as well as
makes the code follow better libvirt practices. Additionally, adjusted
debug logs to be more helpful - removed old ones and added new which
give more valuable info for troubleshooting.
---
src/vbox/vbox_common.c | 206 +++++++++++++++++++++++++++++++------------------
1 file changed, 130 insertions(+), 76 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 9f4bf18a3..2bd891efb 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -959,11 +959,12 @@ static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
- int type, format, ret = 0;
+ int type, ret = 0;
const char *src = NULL;
nsresult rc = 0;
virDomainDiskDefPtr disk = NULL;
PRUnichar *storageCtlName = NULL;
+ char *controllerName = NULL;
IMedium *medium = NULL;
PRUnichar *mediumFileUtf16 = NULL;
PRUint32 devicePort, deviceSlot, deviceType, accessMode;
@@ -1015,47 +1016,104 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data,
IMachine *machine)
disk = def->disks[i];
src = virDomainDiskGetSource(disk);
type = virDomainDiskGetType(disk);
- format = virDomainDiskGetFormat(disk);
deviceType = DeviceType_Null;
accessMode = AccessMode_ReadOnly;
devicePort = disk->info.addr.drive.unit;
deviceSlot = disk->info.addr.drive.bus;
- VIR_DEBUG("disk(%zu) type: %d", i, type);
- VIR_DEBUG("disk(%zu) device: %d", i, disk->device);
- VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus);
- VIR_DEBUG("disk(%zu) src: %s", i, src);
- VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst);
- VIR_DEBUG("disk(%zu) driverName: %s", i,
- virDomainDiskGetDriver(disk));
- VIR_DEBUG("disk(%zu) driverType: %s", i,
- virStorageFileFormatTypeToString(format));
- VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode);
- VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src->readonly
- ? "True" : "False"));
- VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src->shared
- ? "True" : "False"));
-
- if (type == VIR_STORAGE_TYPE_FILE && src) {
- 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));
+ ret = -1;
+ goto cleanup;
+ }
- 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 {
+ switch ((virDomainDiskDevice) disk->device) {
+ case VIR_DOMAIN_DISK_DEVICE_DISK:
+ if (!src) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Missing disk source file path"));
ret = -1;
goto cleanup;
}
- gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
- deviceType, accessMode, &medium);
+ deviceType = DeviceType_HardDisk;
+ accessMode = AccessMode_ReadWrite;
+
+ break;
+
+ case VIR_DOMAIN_DISK_DEVICE_CDROM:
+ deviceType = DeviceType_DVD;
+ accessMode = AccessMode_ReadOnly;
+
+ break;
+ case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
+ deviceType = DeviceType_Floppy;
+ accessMode = AccessMode_ReadWrite;
+
+ break;
+ case VIR_DOMAIN_DISK_DEVICE_LUN:
+ case VIR_DOMAIN_DISK_DEVICE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s disk
device"),
+ virDomainDiskDeviceTypeToString(disk->device));
+ ret = -1;
+ goto cleanup;
+ }
+
+ switch ((virDomainDiskBus) disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
+ devicePort = def->disks[i]->info.addr.drive.bus;
+ deviceSlot = def->disks[i]->info.addr.drive.unit;
+
+ break;
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
+
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
+
+ break;
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
+ devicePort = 0;
+ deviceSlot = disk->info.addr.drive.unit;
+
+ break;
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ case VIR_DOMAIN_DISK_BUS_XEN:
+ case VIR_DOMAIN_DISK_BUS_USB:
+ case VIR_DOMAIN_DISK_BUS_UML:
+ case VIR_DOMAIN_DISK_BUS_SD:
+ case VIR_DOMAIN_DISK_BUS_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s bus type"),
+ virDomainDiskBusTypeToString(disk->bus));
+ ret = -1;
+ goto cleanup;
+ }
+ /* If disk source is specified, lookup IMedium - removable drives don't
+ * have either.
+ */
+ if (src) {
+ VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
+ VIR_DEBUG("Looking up medium %s, type: %d, mode: %d", src,
+ deviceType, accessMode);
+
+ rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
+ deviceType, accessMode,
&medium);
+
+ /* The following is not needed for vbox 4.2+ but older versions have
+ * distinct find and open operations where the former looks in vbox
+ * media registry while the 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,
@@ -1065,7 +1123,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (!medium) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to attach the following disk/dvd/floppy
"
+ _("Failed to open the following disk/dvd/floppy
"
"to the machine: %s, rc=%08x"), src, rc);
ret = -1;
goto cleanup;
@@ -1080,57 +1138,53 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
ret = -1;
goto cleanup;
}
+ }
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (disk->src->readonly) {
- gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
- VIR_DEBUG("Setting harddisk to immutable");
- } else if (!disk->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);
+ VIR_DEBUG("Setting hard disk to immutable");
+ } else if (!disk->src->readonly) {
+ gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
+ VIR_DEBUG("Setting hard disk type to normal");
}
- if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) {
- VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
- devicePort = def->disks[i]->info.addr.drive.bus;
- deviceSlot = def->disks[i]->info.addr.drive.unit;
- } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) {
- VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
- } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
- VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
- } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
- VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
- devicePort = 0;
- deviceSlot = disk->info.addr.drive.unit;
- }
+ }
- /* attach the harddisk/dvd/Floppy to the storage controller */
- rc = gVBoxAPI.UIMachine.AttachDevice(machine,
- storageCtlName,
- devicePort,
- deviceSlot,
- deviceType,
- medium);
+ VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
+ VIR_DEBUG("Attaching disk(%zu), controller: %s, port: %d, slot: %d, "
+ "type: %d, medium: %s", i, controllerName, devicePort,
+ deviceSlot, deviceType, medium == NULL ? "empty" : src);
+ VBOX_UTF8_FREE(controllerName);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not attach the file as "
- "harddisk/dvd/floppy: %s, rc=%08x"), src,
rc);
- ret = -1;
- goto cleanup;
- } else {
- DEBUGIID("Attached HDD/DVD/Floppy with UUID",
&mediumUUID);
- }
- cleanup:
- VBOX_MEDIUM_RELEASE(medium);
- vboxIIDUnalloc(&mediumUUID);
- VBOX_UTF16_FREE(mediumFileUtf16);
- VBOX_UTF16_FREE(storageCtlName);
+ /* Attach the harddisk/dvd/Floppy to the storage controller,
+ * medium == NULL is ok here
+ */
+ rc = gVBoxAPI.UIMachine.AttachDevice(machine,
+ storageCtlName,
+ devicePort,
+ deviceSlot,
+ deviceType,
+ medium);
- if (ret < 0)
- break;
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not attach the file as "
+ "harddisk/dvd/floppy: %s, rc=%08x"), src, rc);
+ ret = -1;
+ goto cleanup;
+ } else {
+ DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID);
}
+
+ cleanup:
+ VBOX_MEDIUM_RELEASE(medium);
+ vboxIIDUnalloc(&mediumUUID);
+ VBOX_UTF16_FREE(mediumFileUtf16);
+ VBOX_UTF16_FREE(storageCtlName);
+
+ if (ret < 0)
+ break;
}
return ret;
--
2.14.2
8:46 a.m.
New subject: [libvirt] [PATCH v2 07/15] vbox: Support empty removable drives.
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Original code was checking for non empty disk source before
proceeding
to actually attach disk device to VM. This prevented from creating
empty removable devices like DVD or floppy. Therefore, this patch
re-organizes the loop work-flow to allow such configurations as well as
makes the code follow better libvirt practices. Additionally, adjusted
debug logs to be more helpful - removed old ones and added new which
give more valuable info for troubleshooting.
---
src/vbox/vbox_common.c | 206 +++++++++++++++++++++++++++++++------------------
1 file changed, 130 insertions(+), 76 deletions(-)
There's a lot going on here, but I'm not sure how much is separable
other than perhaps the debug stuff, so even though there's a lot going
on ...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:35 p.m.
New subject: [libvirt] [PATCH v2 08/15] vbox: Add more IStorageController API mappings
This patch exposes additional methods of the native VBOX API to the
libvirt 'unified' vbox API to deal with IStorageController. The exposed
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 2679b60f7..a25f14c09 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;
@@ -1916,6 +1921,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)
{
@@ -2265,6 +2282,7 @@ static vboxUniformedArray _UArray = {
.handleGetMachines = _handleGetMachines,
.handleGetHardDisks = _handleGetHardDisks,
.handleUSBGetDeviceFilters = _handleUSBGetDeviceFilters,
+ .handleMachineGetStorageControllers = _handleMachineGetStorageControllers,
.handleMachineGetMediumAttachments = _handleMachineGetMediumAttachments,
.handleMachineGetSharedFolders = _handleMachineGetSharedFolders,
.handleSnapshotGetChildren = _handleSnapshotGetChildren,
@@ -2496,6 +2514,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
8:47 a.m.
New subject: [libvirt] [PATCH v2 08/15] vbox: Add more IStorageController API mappings
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This patch exposes additional methods of the native VBOX API to the
libvirt 'unified' vbox API to deal with IStorageController. The exposed
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(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:35 p.m.
New subject: [libvirt] [PATCH v2 09/15] domain: Allow 'model' attribute for ide controller
The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
needed to allow setting IDE controller model in VirtualBox driver.
---
docs/formatdomain.html.in | 4 ++++
docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
src/conf/domain_conf.c | 9 +++++++++
src/conf/domain_conf.h | 9 +++++++++
src/libvirt_private.syms | 2 ++
5 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4609e2ec2..8dff685ad 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3648,6 +3648,10 @@
<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><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>
</dl>
<p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 710b3af7f..9cec1a063 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1975,12 +1975,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>
@@ -2041,6 +2040,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 77c20c697..57f291624 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -377,6 +377,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",
@@ -9785,6 +9790,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;
}
@@ -9800,6 +9807,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 38de70b15..0def905b2 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 || \
@@ -3219,6 +3227,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 448d962b2..2e67366b7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -234,6 +234,8 @@ virDomainControllerFindUnusedIndex;
virDomainControllerInsert;
virDomainControllerInsertPreAlloced;
virDomainControllerIsPSeriesPHB;
+virDomainControllerModelIDETypeFromString;
+virDomainControllerModelIDETypeToString;
virDomainControllerModelPCITypeToString;
virDomainControllerModelSCSITypeFromString;
virDomainControllerModelSCSITypeToString;
--
2.14.2
8:48 a.m.
New subject: [libvirt] [PATCH v2 09/15] domain: Allow 'model' attribute for ide controller
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
The optional values are 'piix3', 'piix4' or
'ich6'. Those will be
needed to allow setting IDE controller model in VirtualBox driver.
---
docs/formatdomain.html.in | 4 ++++
docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
src/conf/domain_conf.c | 9 +++++++++
src/conf/domain_conf.h | 9 +++++++++
src/libvirt_private.syms | 2 ++
5 files changed, 40 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
NB: Since 3.9 is already generated, I can modify formatdomain.html.in to
list 3.10
2:35 p.m.
New subject: [libvirt] [PATCH v2 10/15] vbox: Process <controller> element in domain XML
This patch enables the VBOX driver to process the <controller> element
in domain XML through which one can now customize the controller model.
Since VirtualBox has two distinct SAS and SCSI they do not "map"
directly to libvirt XML, he VBOX driver uses <contoller type="scsi"
model="lsisas1068" /> to create SAS controller in VBOX VM. Additionally
once can set model on the IDE controller to be one of "piix3",
"piix4"
or "ich6".
---
src/vbox/vbox_common.c | 214 ++++++++++++++++++++++++++++++++++++++-----------
src/vbox/vbox_common.h | 8 ++
2 files changed, 176 insertions(+), 46 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 2bd891efb..9d45e4a76 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -406,6 +406,160 @@ 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;
+ char *debugName = NULL;
+ int ret = -1;
+
+ /* libvirt controller type => vbox bus type */
+ switch ((virDomainControllerType) 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;
+ case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
+ case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+ case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+ case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+ case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s controller
type"),
+ virDomainControllerTypeToString(controller->type));
+ return -1;
+ }
+
+ /* libvirt scsi model => vbox scsi model */
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ switch ((virDomainControllerModelSCSI) controller->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
+ 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;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s SCSI "
+ "controller model"),
+
virDomainControllerModelSCSITypeToString(controller->model));
+ goto cleanup;
+ }
+ /* libvirt ide model => vbox ide model */
+ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
+ switch ((virDomainControllerModelIDE) 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;
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s IDE "
+ "controller model"),
+
virDomainControllerModelIDETypeToString(controller->model));
+ goto cleanup;
+ }
+ }
+
+ VBOX_UTF16_TO_UTF8(controllerName, &debugName);
+ VIR_DEBUG("Adding VBOX storage controller (name: %s, busType: %d)",
+ debugName, vboxBusType);
+
+ rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName,
+ vboxBusType, &vboxController);
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to add storage controller "
+ "(name: %s, busType: %d), rc=%08x"),
+ debugName, vboxBusType, rc);
+ goto cleanup;
+ }
+
+ /* only IDE or SCSI controller have model choices */
+ 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"), rc);
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+
+ cleanup:
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_FREE(debugName);
+ 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,
@@ -959,7 +1113,7 @@ static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
- int type, ret = 0;
+ int type, ret = 0, model = -1;
const char *src = NULL;
nsresult rc = 0;
virDomainDiskDefPtr disk = NULL;
@@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
VBOX_IID_INITIALIZE(&mediumUUID);
- /* 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; i++) {
disk = def->disks[i];
src = virDomainDiskGetSource(disk);
@@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
switch ((virDomainDiskBus) disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
- VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &storageCtlName);
devicePort = def->disks[i]->info.addr.drive.bus;
deviceSlot = def->disks[i]->info.addr.drive.unit;
break;
case VIR_DOMAIN_DISK_BUS_SATA:
- VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &storageCtlName);
break;
case VIR_DOMAIN_DISK_BUS_SCSI:
- VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName);
+
+ model = virDomainDeviceFindControllerModel(def, &disk->info,
+ VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+ if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ VBOX_UTF16_FREE(storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName);
+ }
break;
case VIR_DOMAIN_DISK_BUS_FDC:
- VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName);
devicePort = 0;
deviceSlot = disk->info.addr.drive.unit;
@@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
VIR_DEBUG("Setting hard disk type to normal");
}
-
}
VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
@@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
vboxSetBootDeviceOrder(def, data, machine);
+ if (vboxAttachStorageControllers(def, data, machine) < 0)
+ goto cleanup;
if (vboxAttachDrives(def, data, machine) < 0)
goto cleanup;
vboxAttachSound(def, machine);
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index b08ad1e3e..3340374c1 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;
--
2.14.2
8:51 a.m.
New subject: [libvirt] [PATCH v2 10/15] vbox: Process <controller> element in domain XML
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This patch enables the VBOX driver to process the <controller>
element
in domain XML through which one can now customize the controller model.
Since VirtualBox has two distinct SAS and SCSI they do not "map"
directly to libvirt XML, he VBOX driver uses <contoller type="scsi"
model="lsisas1068" /> to create SAS controller in VBOX VM. Additionally
once can set model on the IDE controller to be one of "piix3",
"piix4"
or "ich6".
---
src/vbox/vbox_common.c | 214 ++++++++++++++++++++++++++++++++++++++-----------
src/vbox/vbox_common.h | 8 ++
2 files changed, 176 insertions(+), 46 deletions(-)
So beyond patch 3 which I know you have to repost anyway - starting with
this patch and going forward, I figure you have to repost anyway as long
as I get the question in patch 5 answered of course...
This patch left me with a concern. Up to this point vboxAttachDrives
would add at least one controller for each type supported regardless of
whether the VM used it.
After this patch only those controllers defined in the VM XML will be
added. Which would seem to be fine, except for the case of hotplug which
I wasn't clear whether vbox support or not.
Let's say the VM is defined with and IDE controller, if someone tried to
add a SCSI disk after startup, then there'd be no SCSI controller
already present.
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 2bd891efb..9d45e4a76 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -406,6 +406,160 @@ 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;
+ char *debugName = NULL;
+ int ret = -1;
+
+ /* libvirt controller type => vbox bus type */
+ switch ((virDomainControllerType) 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;
I think prior to this patch - there should be a patch that "manages" all
those range check differences, storageBus comparison checks, etc. for
VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches.
John
+
+ break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
+ case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+ case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+ case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+ case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s controller
type"),
+ virDomainControllerTypeToString(controller->type));
+ return -1;
+ }
+
+ /* libvirt scsi model => vbox scsi model */
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ switch ((virDomainControllerModelSCSI) controller->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
+ 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;
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s SCSI "
+ "controller model"),
+
virDomainControllerModelSCSITypeToString(controller->model));
+ goto cleanup;
+ }
+ /* libvirt ide model => vbox ide model */
+ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
+ switch ((virDomainControllerModelIDE) 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;
+ case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The vbox driver does not support %s IDE "
+ "controller model"),
+
virDomainControllerModelIDETypeToString(controller->model));
+ goto cleanup;
+ }
+ }
+
+ VBOX_UTF16_TO_UTF8(controllerName, &debugName);
+ VIR_DEBUG("Adding VBOX storage controller (name: %s, busType: %d)",
+ debugName, vboxBusType);
+
+ rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName,
+ vboxBusType, &vboxController);
+
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to add storage controller "
+ "(name: %s, busType: %d), rc=%08x"),
+ debugName, vboxBusType, rc);
+ goto cleanup;
+ }
+
+ /* only IDE or SCSI controller have model choices */
+ 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"), rc);
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+
+ cleanup:
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_FREE(debugName);
+ 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,
@@ -959,7 +1113,7 @@ static int
vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
size_t i;
- int type, ret = 0;
+ int type, ret = 0, model = -1;
const char *src = NULL;
nsresult rc = 0;
virDomainDiskDefPtr disk = NULL;
@@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
VBOX_IID_INITIALIZE(&mediumUUID);
- /* 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; i++) {
disk = def->disks[i];
src = virDomainDiskGetSource(disk);
@@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data,
IMachine *machine)
switch ((virDomainDiskBus) disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
- VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &storageCtlName);
devicePort = def->disks[i]->info.addr.drive.bus;
deviceSlot = def->disks[i]->info.addr.drive.unit;
break;
case VIR_DOMAIN_DISK_BUS_SATA:
- VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &storageCtlName);
break;
case VIR_DOMAIN_DISK_BUS_SCSI:
- VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName);
+
+ model = virDomainDeviceFindControllerModel(def, &disk->info,
+
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+ if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ VBOX_UTF16_FREE(storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName);
+ }
break;
case VIR_DOMAIN_DISK_BUS_FDC:
- VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
+ VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName);
devicePort = 0;
deviceSlot = disk->info.addr.drive.unit;
@@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
VIR_DEBUG("Setting hard disk type to normal");
}
-
}
VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
@@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags
gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
vboxSetBootDeviceOrder(def, data, machine);
+ if (vboxAttachStorageControllers(def, data, machine) < 0)
+ goto cleanup;
if (vboxAttachDrives(def, data, machine) < 0)
goto cleanup;
vboxAttachSound(def, machine);
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index b08ad1e3e..3340374c1 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;
10:28 a.m.
New subject: [libvirt] [PATCH v2 10/15] vbox: Process <controller> element in domain XML
On Fri, 2017-11-03 at 09:51 -0400, John Ferlan wrote:
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> This patch enables the VBOX driver to process the <controller>
> element
> in domain XML through which one can now customize the controller
> model.
>
> Since VirtualBox has two distinct SAS and SCSI they do not "map"
> directly to libvirt XML, he VBOX driver uses <contoller type="scsi"
> model="lsisas1068" /> to create SAS controller in VBOX VM.
> Additionally
> once can set model on the IDE controller to be one of "piix3",
> "piix4"
> or "ich6".
> ---
> src/vbox/vbox_common.c | 214
> ++++++++++++++++++++++++++++++++++++++-----------
> src/vbox/vbox_common.h | 8 ++
> 2 files changed, 176 insertions(+), 46 deletions(-)
>
So beyond patch 3 which I know you have to repost anyway - starting
with
this patch and going forward, I figure you have to repost anyway as
long
as I get the question in patch 5 answered of course...
This patch left me with a concern. Up to this point vboxAttachDrives
would add at least one controller for each type supported regardless
of
whether the VM used it.
After this patch only those controllers defined in the VM XML will be
added. Which would seem to be fine, except for the case of hotplug
which
I wasn't clear whether vbox support or not.
Let's say the VM is defined with and IDE controller, if someone tried
to
add a SCSI disk after startup, then there'd be no SCSI controller
already present.
I've just checked this with VirutalBox GUI and the only thing that can
be changed on live VM is removable media (CD/DVD). All options to add
disk devices to controller are disabled when VM is running. Neither
quick web search returned anything useful regarding hot-add disk
support on VBOX.
Having said that, the original behavior was, IIRC, the reason why I
actually started working on this series. According to our tech support
they have ran into a couple of cases where the OS would BSOD (some
legacy Windows OS e.g. 2000 or XP) when there was a controller (without
a disk) attached to VM that the OS did not handle well. It sounded very
strange to me but I've been told it was not a one-off case though I
personally haven't reproduced it. Unfortunately, I won't be able to
provide more details as we no longer use VBox internally (migrated to
KVM) so those potential test cases are no longer around.
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 2bd891efb..9d45e4a76 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -406,6 +406,160 @@ 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;
> + char *debugName = NULL;
> + int ret = -1;
> +
> + /* libvirt controller type => vbox bus type */
> + switch ((virDomainControllerType) 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;
I think prior to this patch - there should be a patch that "manages"
all
those range check differences, storageBus comparison checks, etc. for
VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches.
John
> +
> + break;
> + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> + case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> + case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> + case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The vbox driver does not support %s
> controller type"),
> + virDomainControllerTypeToString(controller-
> >type));
> + return -1;
> + }
> +
> + /* libvirt scsi model => vbox scsi model */
> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> + switch ((virDomainControllerModelSCSI) controller->model)
> {
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
> + 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;
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The vbox driver does not support %s
> SCSI "
> + "controller model"),
> + virDomainControllerModelSCSITypeToStrin
> g(controller->model));
> + goto cleanup;
> + }
> + /* libvirt ide model => vbox ide model */
> + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> {
> + switch ((virDomainControllerModelIDE) 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;
> + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The vbox driver does not support %s
> IDE "
> + "controller model"),
> + virDomainControllerModelIDETypeToStri
> ng(controller->model));
> + goto cleanup;
> + }
> + }
> +
> + VBOX_UTF16_TO_UTF8(controllerName, &debugName);
> + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType:
> %d)",
> + debugName, vboxBusType);
> +
> + rc = gVBoxAPI.UIMachine.AddStorageController(machine,
> controllerName,
> + vboxBusType,
> &vboxController);
> +
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to add storage controller "
> + "(name: %s, busType: %d), rc=%08x"),
> + debugName, vboxBusType, rc);
> + goto cleanup;
> + }
> +
> + /* only IDE or SCSI controller have model choices */
> + if (vboxModel != StorageControllerType_Null) {
> + rc =
> gVBoxAPI.UIStorageController.SetControllerType(vboxController,
> + vboxMo
> del);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to change storage controller
> model, "
> + "rc=%08x"), rc);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VBOX_UTF16_FREE(controllerName);
> + VBOX_UTF8_FREE(debugName);
> + 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,
> @@ -959,7 +1113,7 @@ static int
> vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> *machine)
> {
> size_t i;
> - int type, ret = 0;
> + int type, ret = 0, model = -1;
> const char *src = NULL;
> nsresult rc = 0;
> virDomainDiskDefPtr disk = NULL;
> @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
>
> VBOX_IID_INITIALIZE(&mediumUUID);
>
> - /* 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; i++) {
> disk = def->disks[i];
> src = virDomainDiskGetSource(disk);
> @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
>
> switch ((virDomainDiskBus) disk->bus) {
> case VIR_DOMAIN_DISK_BUS_IDE:
> - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> &storageCtlName);
> devicePort = def->disks[i]->info.addr.drive.bus;
> deviceSlot = def->disks[i]->info.addr.drive.unit;
>
> break;
> case VIR_DOMAIN_DISK_BUS_SATA:
> - VBOX_UTF8_TO_UTF16("SATA Controller",
> &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> &storageCtlName);
>
> break;
> case VIR_DOMAIN_DISK_BUS_SCSI:
> - VBOX_UTF8_TO_UTF16("SCSI Controller",
> &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> &storageCtlName);
> +
> + model = virDomainDeviceFindControllerModel(def, &disk-
> >info,
> + VIR_DOMAIN_
> CONTROLLER_TYPE_SCSI);
> + if (model ==
> VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> + VBOX_UTF16_FREE(storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> &storageCtlName);
> + }
>
> break;
> case VIR_DOMAIN_DISK_BUS_FDC:
> - VBOX_UTF8_TO_UTF16("Floppy Controller",
> &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> &storageCtlName);
> devicePort = 0;
> deviceSlot = disk->info.addr.drive.unit;
>
> @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Normal);
> VIR_DEBUG("Setting hard disk type to normal");
> }
> -
> }
>
> VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
> @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
> gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
>
> vboxSetBootDeviceOrder(def, data, machine);
> + if (vboxAttachStorageControllers(def, data, machine) < 0)
> + goto cleanup;
> if (vboxAttachDrives(def, data, machine) < 0)
> goto cleanup;
> vboxAttachSound(def, machine);
> diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> index b08ad1e3e..3340374c1 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;
>
12:25 p.m.
New subject: [libvirt] [PATCH v2 10/15] vbox: Process <controller> element in domain XML
On Fri, 2017-11-03 at 09:51 -0400, John Ferlan wrote:
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
>
> + 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;
I think prior to this patch - there should be a patch that "manages"
all
those range check differences, storageBus comparison checks, etc. for
VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches.
John
Up until this patch, the series modifies the definexml functionality in
which case input is libvirt XML that is translated to vbox values. The
supported libvirt types are 'validated' by the typed switch...case
statement so it's not possible to run into a vbox value that the driver
does not recognize. The patches that follow are for dumpxml and those
do get updated to handle StorageBus_SAS (the new one introduced in this
series) in patch #12 (see vboxGenerateMediumName change) and in patch
#15 where vboxDumpDisks is changed to use typed switch statement on
storageBus.
Dawid
> +
> + break;
> + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> + case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> + case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> + case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The vbox driver does not support %s
> controller type"),
> + virDomainControllerTypeToString(controller-
> >type));
> + return -1;
> + }
> +
> + /* libvirt scsi model => vbox scsi model */
> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> + switch ((virDomainControllerModelSCSI) controller->model)
> {
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
> + 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;
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The vbox driver does not support %s
> SCSI "
> + "controller model"),
> + virDomainControllerModelSCSITypeToStrin
> g(controller->model));
> + goto cleanup;
> + }
> + /* libvirt ide model => vbox ide model */
> + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> {
> + switch ((virDomainControllerModelIDE) 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;
> + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The vbox driver does not support %s
> IDE "
> + "controller model"),
> + virDomainControllerModelIDETypeToStri
> ng(controller->model));
> + goto cleanup;
> + }
> + }
> +
> + VBOX_UTF16_TO_UTF8(controllerName, &debugName);
> + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType:
> %d)",
> + debugName, vboxBusType);
> +
> + rc = gVBoxAPI.UIMachine.AddStorageController(machine,
> controllerName,
> + vboxBusType,
> &vboxController);
> +
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to add storage controller "
> + "(name: %s, busType: %d), rc=%08x"),
> + debugName, vboxBusType, rc);
> + goto cleanup;
> + }
> +
> + /* only IDE or SCSI controller have model choices */
> + if (vboxModel != StorageControllerType_Null) {
> + rc =
> gVBoxAPI.UIStorageController.SetControllerType(vboxController,
> + vboxMo
> del);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to change storage controller
> model, "
> + "rc=%08x"), rc);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VBOX_UTF16_FREE(controllerName);
> + VBOX_UTF8_FREE(debugName);
> + 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,
> @@ -959,7 +1113,7 @@ static int
> vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> *machine)
> {
> size_t i;
> - int type, ret = 0;
> + int type, ret = 0, model = -1;
> const char *src = NULL;
> nsresult rc = 0;
> virDomainDiskDefPtr disk = NULL;
> @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
>
> VBOX_IID_INITIALIZE(&mediumUUID);
>
> - /* 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; i++) {
> disk = def->disks[i];
> src = virDomainDiskGetSource(disk);
> @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
>
> switch ((virDomainDiskBus) disk->bus) {
> case VIR_DOMAIN_DISK_BUS_IDE:
> - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> &storageCtlName);
> devicePort = def->disks[i]->info.addr.drive.bus;
> deviceSlot = def->disks[i]->info.addr.drive.unit;
>
> break;
> case VIR_DOMAIN_DISK_BUS_SATA:
> - VBOX_UTF8_TO_UTF16("SATA Controller",
> &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> &storageCtlName);
>
> break;
> case VIR_DOMAIN_DISK_BUS_SCSI:
> - VBOX_UTF8_TO_UTF16("SCSI Controller",
> &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> &storageCtlName);
> +
> + model = virDomainDeviceFindControllerModel(def, &disk-
> >info,
> + VIR_DOMAIN_
> CONTROLLER_TYPE_SCSI);
> + if (model ==
> VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> + VBOX_UTF16_FREE(storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> &storageCtlName);
> + }
>
> break;
> case VIR_DOMAIN_DISK_BUS_FDC:
> - VBOX_UTF8_TO_UTF16("Floppy Controller",
> &storageCtlName);
> + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> &storageCtlName);
> devicePort = 0;
> deviceSlot = disk->info.addr.drive.unit;
>
> @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> gVBoxAPI.UIMedium.SetType(medium,
> MediumType_Normal);
> VIR_DEBUG("Setting hard disk type to normal");
> }
> -
> }
>
> VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
> @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
> gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
>
> vboxSetBootDeviceOrder(def, data, machine);
> + if (vboxAttachStorageControllers(def, data, machine) < 0)
> + goto cleanup;
> if (vboxAttachDrives(def, data, machine) < 0)
> goto cleanup;
> vboxAttachSound(def, machine);
> diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> index b08ad1e3e..3340374c1 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;
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:35 p.m.
New subject: [libvirt] [PATCH v2 11/15] vbox: Add vboxDumpStorageControllers
---
src/vbox/vbox_common.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 9d45e4a76..715eb670e 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3153,6 +3153,123 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr def,
IMachine *mach
goto release_filters;
}
+
+static int
+vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine)
+{
+ vboxArray storageControllers = VBOX_ARRAY_INITIALIZER;
+ IStorageController *controller = NULL;
+ PRUint32 storageBus = StorageBus_Null;
+ PRUint32 controllerType = StorageControllerType_Null;
+ virDomainControllerDefPtr cont = NULL;
+ size_t i = 0;
+ int model = -1, ret = -1;
+ virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+
+ gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine,
+ gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));
+
+ for (i = 0; i < storageControllers.count; i++) {
+ controller = storageControllers.items[i];
+ storageBus = StorageBus_Null;
+ controllerType = StorageControllerType_Null;
+ type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+ model = -1;
+
+ if (!controller)
+ continue;
+
+ gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+ gVBoxAPI.UIStorageController.GetControllerType(controller,
+ &controllerType);
+
+ /* vbox controller model => libvirt controller model */
+ switch ((enum StorageControllerType) 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;
+ case StorageControllerType_BusLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC;
+
+ break;
+ case StorageControllerType_LsiLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+
+ break;
+ case StorageControllerType_LsiLogicSas:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068;
+
+ break;
+ case StorageControllerType_IntelAhci:
+ case StorageControllerType_I82078:
+ case StorageControllerType_Null:
+ model = -1;
+
+ break;
+ }
+
+ /* vbox controller bus => libvirt controller type */
+ switch ((enum StorageBus) storageBus) {
+ case StorageBus_IDE:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_IDE;
+
+ break;
+ case StorageBus_SCSI:
+ case StorageBus_SAS:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
+
+ break;
+ case StorageBus_SATA:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_SATA;
+
+ break;
+ case StorageBus_Floppy:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_FDC;
+
+ break;
+ case StorageBus_Null:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unsupported null storage bus"));
+
+ goto cleanup;
+ }
+
+ if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) {
+ cont = virDomainDefAddController(def, type, -1, model);
+ if (!cont) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to add %s controller type
definition"),
+ virDomainControllerTypeToString(type));
+ goto cleanup;
+ }
+ }
+ }
+
+ ret = 0;
+
+ cleanup:
+ gVBoxAPI.UArray.vboxArrayRelease(&storageControllers);
+
+ if (ret < 0) {
+ for (i = 0; i < def->ncontrollers; i++)
+ virDomainControllerDefFree(def->controllers[i]);
+ VIR_FREE(def->controllers);
+ def->ncontrollers = 0;
+ }
+
+ return ret;
+}
+
+
static void
vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
@@ -4000,6 +4117,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
goto cleanup;
if (vboxDumpDisplay(def, data, machine) < 0)
goto cleanup;
+ if (vboxDumpStorageControllers(def, machine) < 0)
+ goto cleanup;
vboxDumpIDEHDDs(def, data, machine);
--
2.14.2
8:52 a.m.
New subject: [libvirt] [PATCH v2 11/15] vbox: Add vboxDumpStorageControllers
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
---
src/vbox/vbox_common.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 9d45e4a76..715eb670e 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3153,6 +3153,123 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr def,
IMachine *mach
goto release_filters;
}
+
+static int
+vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine)
+{
+ vboxArray storageControllers = VBOX_ARRAY_INITIALIZER;
+ IStorageController *controller = NULL;
+ PRUint32 storageBus = StorageBus_Null;
+ PRUint32 controllerType = StorageControllerType_Null;
+ virDomainControllerDefPtr cont = NULL;
+ size_t i = 0;
+ int model = -1, ret = -1;
+ virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+
+ gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine,
+ gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));
+
+ for (i = 0; i < storageControllers.count; i++) {
+ controller = storageControllers.items[i];
+ storageBus = StorageBus_Null;
+ controllerType = StorageControllerType_Null;
+ type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+ model = -1;
+
+ if (!controller)
+ continue;
+
+ gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+ gVBoxAPI.UIStorageController.GetControllerType(controller,
+ &controllerType);
+
+ /* vbox controller model => libvirt controller model */
+ switch ((enum StorageControllerType) 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;
+ case StorageControllerType_BusLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC;
+
+ break;
+ case StorageControllerType_LsiLogic:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+
+ break;
+ case StorageControllerType_LsiLogicSas:
+ model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068;
+
+ break;
+ case StorageControllerType_IntelAhci:
+ case StorageControllerType_I82078:
+ case StorageControllerType_Null:
+ model = -1;
+
+ break;
+ }
+
+ /* vbox controller bus => libvirt controller type */
+ switch ((enum StorageBus) storageBus) {
+ case StorageBus_IDE:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_IDE;
+
+ break;
+ case StorageBus_SCSI:
+ case StorageBus_SAS:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
+
+ break;
+ case StorageBus_SATA:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_SATA;
+
+ break;
+ case StorageBus_Floppy:
+ type = VIR_DOMAIN_CONTROLLER_TYPE_FDC;
+
+ break;
+ case StorageBus_Null:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unsupported null storage bus"));
+
+ goto cleanup;
+ }
+
+ if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) {
+ cont = virDomainDefAddController(def, type, -1, model);
+ if (!cont) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to add %s controller type
definition"),
+ virDomainControllerTypeToString(type));
+ goto cleanup;
+ }
+ }
+ }
+
+ ret = 0;
+
+ cleanup:
+ gVBoxAPI.UArray.vboxArrayRelease(&storageControllers);
+
+ if (ret < 0) {
+ for (i = 0; i < def->ncontrollers; i++)
+ virDomainControllerDefFree(def->controllers[i]);
+ VIR_FREE(def->controllers);
+ def->ncontrollers = 0;
Although not necessarily a problem, since we're erroring out anyway,
eventually when virDomainDefFree is called, this will be cleaned out
anyway, so it can be removed...
Remember that vboxDomainGetXMLDesc won't call virDomainDefFormat and
it'll return NULL
With that adjustment,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ }
+
+ return ret;
+}
+
+
static void
vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
@@ -4000,6 +4117,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
goto cleanup;
if (vboxDumpDisplay(def, data, machine) < 0)
goto cleanup;
+ if (vboxDumpStorageControllers(def, machine) < 0)
+ goto cleanup;
vboxDumpIDEHDDs(def, data, machine);
2:35 p.m.
New subject: [libvirt] [PATCH v2 12/15] vbox: Correctly generate drive name in dumpxml
If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML generated
by dumpxml used to produce "sda" for both of those disks. This is an
invalid domain XML as libvirt does not allow duplicate device names. To
address this, keep the running total of disks that will use "sd" prefix
for device name and pass it to the vboxGenerateMediumName which no
longer tries to "compute" the value based only on current and max
port and slot values. After this the vboxGetMaxPortSlotValues is not
needed and was deleted.
---
src/vbox/vbox_common.c | 414 +++++++++++++++++++++----------------------------
1 file changed, 177 insertions(+), 237 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 715eb670e..9dc36a1b2 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -290,61 +290,6 @@ 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_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_Floppy,
-
&maxSlotPerPort[StorageBus_Floppy]);
-
- VBOX_RELEASE(sysProps);
-
- return true;
-}
/**
* function to generate the name for medium,
@@ -352,57 +297,40 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
*
* @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_Floppy))
+ (storageBus > StorageBus_SAS))
return NULL;
- maxPortPerInst = aMaxPortPerInst[storageBus];
- maxSlotPerPort = aMaxSlotPerPort[storageBus];
- total = (deviceInst * maxPortPerInst * maxSlotPerPort)
- + (devicePort * maxSlotPerPort)
- + deviceSlot;
-
if (storageBus == StorageBus_IDE) {
prefix = "hd";
- } else if ((storageBus == StorageBus_SATA) ||
- (storageBus == StorageBus_SCSI)) {
+ 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;
}
@@ -3270,20 +3198,17 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine
*machine)
}
-static void
-vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
+static int
+vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
- /* dump IDE hdds if present */
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
- bool error = false;
+ size_t sdCount = 0, i;
int diskCount = 0;
- size_t i;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
+ int ret = -1;
def->ndisks = 0;
gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
-
gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+ gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
/* get the number of attachments */
for (i = 0; i < mediumAttachments.count; i++) {
@@ -3300,24 +3225,19 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
}
/* 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;
- }
- def->disks[i] = disk;
- }
- } else {
- error = true;
- }
+ if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
+ goto cleanup;
- if (!error)
- error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
maxSlotPerPort);
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+ if (!disk)
+ goto cleanup;
+
+ def->disks[i] = disk;
+ }
/* get the attachment details here */
- for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks
&& !error; i++) {
+ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
IMediumAttachment *imediumattach = mediumAttachments.items[i];
IStorageController *storageController = NULL;
PRUnichar *storageControllerName = NULL;
@@ -3327,7 +3247,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
IMedium *medium = NULL;
PRUnichar *mediumLocUtf16 = NULL;
char *mediumLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
@@ -3363,16 +3282,36 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (!virDomainDiskGetSource(def->disks[diskCount])) {
VBOX_RELEASE(medium);
VBOX_RELEASE(storageController);
- error = true;
- break;
+
+ goto cleanup;
}
gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+ gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+ gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+
+ def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
+ devicePort,
+ deviceSlot,
+ sdCount);
+ if (!def->disks[diskCount]->dst) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not generate medium name for the disk "
+ "at: port:%d, slot:%d"), devicePort, deviceSlot);
+ VBOX_RELEASE(medium);
+ VBOX_RELEASE(storageController);
+
+ goto cleanup;
+ }
+
if (storageBus == StorageBus_IDE) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
+ sdCount++;
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
+ sdCount++;
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
@@ -3386,24 +3325,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
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;
- }
gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
if (readOnly == PR_TRUE)
@@ -3417,15 +3338,20 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
diskCount++;
}
+ ret = 0;
+
+ cleanup:
gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
/* cleanup on error */
- if (error) {
+ if (ret < 0) {
for (i = 0; i < def->ndisks; i++)
VIR_FREE(def->disks[i]);
VIR_FREE(def->disks);
def->ndisks = 0;
}
+
+ return ret;
}
static int
@@ -4120,7 +4046,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
if (vboxDumpStorageControllers(def, machine) < 0)
goto cleanup;
- vboxDumpIDEHDDs(def, data, machine);
+ if (vboxDumpDisks(def, data, machine) < 0)
+ goto cleanup;
vboxDumpSharedFolders(def, data, machine);
vboxDumpNetwork(def, data, machine, networkAdapterCount);
@@ -5676,8 +5603,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;
@@ -5686,9 +5614,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;
@@ -5757,9 +5683,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;
@@ -5769,7 +5692,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;
@@ -5778,26 +5700,72 @@ 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 storage controller name"));
goto cleanup;
}
- if (!disk)
- continue;
- rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
+
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ storageControllerName,
+ &storageController);
+ VBOX_UTF16_FREE(storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get controller"));
+ _("Cannot get storage controller by name"));
goto cleanup;
}
- if (!storageControllerName) {
- VBOX_RELEASE(disk);
+
+ rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot get storage controller bus"));
+ VBOX_RELEASE(storageController);
+ goto cleanup;
+ }
+
+ 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 port"));
+ 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 slot"));
+ 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 empty removable disk */
+ if (!disk) {
+ VBOX_RELEASE(storageController);
continue;
}
+
handle = gVBoxAPI.UArray.handleMediumGetChildren(disk);
rc = gVBoxAPI.UArray.vboxArrayGet(&children, disk, handle);
if (NS_FAILED(rc)) {
@@ -5820,60 +5788,30 @@ 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(storageController);
+ VBOX_RELEASE(disk);
+ VBOX_RELEASE(child);
goto cleanup;
}
VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8);
VBOX_UTF16_FREE(childLocUtf16);
if (VIR_STRDUP(def->disks[diskCount].src->path, childLocUtf8)
< 0) {
- VBOX_RELEASE(child);
VBOX_RELEASE(storageController);
+ VBOX_RELEASE(disk);
+ VBOX_RELEASE(child);
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);
}
@@ -5881,10 +5819,16 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
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++)
@@ -5896,9 +5840,9 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr
def,
return ret;
}
-static
-int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
- virDomainSnapshotDefPtr def)
+static int
+vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
+ virDomainSnapshotDefPtr def)
{
virDomainPtr dom = snapshot->domain;
vboxDriverPtr data = dom->conn->privateData;
@@ -5910,10 +5854,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
IMedium *disk = NULL;
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 i = 0, diskCount = 0, sdCount = 0;
int ret = -1;
if (!data->vboxObj)
@@ -5976,9 +5917,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;
@@ -5987,7 +5925,6 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
PRBool readOnly = PR_FALSE;
PRUnichar *mediumLocUtf16 = NULL;
char *mediumLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
IMediumAttachment *imediumattach = mediumAttachments.items[i];
@@ -5996,7 +5933,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get medium"));
+ _("Cannot get medium"));
goto cleanup;
}
if (!disk)
@@ -6004,7 +5941,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller name"));
+ _("Cannot get storage controller name"));
goto cleanup;
}
if (!storageControllerName)
@@ -6012,18 +5949,18 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
storageControllerName,
&storageController);
+ VBOX_UTF16_FREE(storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller"));
+ _("Cannot get storage controller"));
goto cleanup;
}
- VBOX_UTF16_FREE(storageControllerName);
if (!storageController)
continue;
rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get disk location"));
+ _("Cannot get disk location"));
goto cleanup;
}
VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
@@ -6036,14 +5973,48 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot get storage controller bus"));
+ _("Cannot get storage controller bus"));
+ goto cleanup;
+ }
+ 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 slot"));
+ 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;
+ }
+
+ def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
+ devicePort,
+ deviceSlot,
+ sdCount);
+ if (!def->dom->disks[diskCount]->dst) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not generate medium name for the disk "
+ "at: port:%d, slot:%d"), devicePort, deviceSlot);
+ ret = -1;
goto cleanup;
}
+
if (storageBus == StorageBus_IDE) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
+ sdCount++;
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
+ sdCount++;
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
@@ -6062,46 +6033,15 @@ 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 "
- "at: controller instance:%u, port:%d, slot:%d"),
- deviceInst, devicePort, deviceSlot);
- ret = -1;
- goto cleanup;
- }
- diskCount ++;
+
+ diskCount++;
}
- /* cleanup on error */
ret = 0;
+
cleanup:
if (ret < 0) {
for (i = 0; i < def->dom->ndisks; i++)
--
2.14.2
8:52 a.m.
New subject: [libvirt] [PATCH v2 12/15] vbox: Correctly generate drive name in dumpxml
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML
generated
by dumpxml used to produce "sda" for both of those disks. This is an
invalid domain XML as libvirt does not allow duplicate device names. To
address this, keep the running total of disks that will use "sd" prefix
for device name and pass it to the vboxGenerateMediumName which no
longer tries to "compute" the value based only on current and max
port and slot values. After this the vboxGetMaxPortSlotValues is not
needed and was deleted.
---
src/vbox/vbox_common.c | 414 +++++++++++++++++++++----------------------------
1 file changed, 177 insertions(+), 237 deletions(-)
I think this needs a bit more splitting.
1. As noted in patch 10, managing StorageBus_SAS should already be
separated so that it doesn't show as a new range/comparison check here
2. The rename of vboxDumpIDEHDDs to vboxDumpDisks and change from void
to int should be its own patch. While it seems superfluous, it makes
review that much easier. NB: I have a couple more thoughts within the
function about
3. I think the changes to vboxSnapshotGetReadWriteDisks and
vboxSnapshotGetReadOnlyDisks could be in a separate patch and is where
the vboxGetMaxPortSlotValues would get deleted. Although I'm not 100%
sure w/r/t the relationship. Perhaps once vboxDumpDisks is cleaned up a
bit before making the change described in the commit message it'd be
clearer. Just a lot going on.
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 715eb670e..9dc36a1b2 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -290,61 +290,6 @@ 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_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_Floppy,
-
&maxSlotPerPort[StorageBus_Floppy]);
-
- VBOX_RELEASE(sysProps);
-
- return true;
-}
/**
* function to generate the name for medium,
@@ -352,57 +297,40 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
*
* @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_Floppy))
+ (storageBus > StorageBus_SAS))
return NULL;
- maxPortPerInst = aMaxPortPerInst[storageBus];
- maxSlotPerPort = aMaxSlotPerPort[storageBus];
- total = (deviceInst * maxPortPerInst * maxSlotPerPort)
- + (devicePort * maxSlotPerPort)
- + deviceSlot;
-
if (storageBus == StorageBus_IDE) {
prefix = "hd";
- } else if ((storageBus == StorageBus_SATA) ||
- (storageBus == StorageBus_SCSI)) {
+ 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;
}
@@ -3270,20 +3198,17 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine
*machine)
}
-static void
-vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
+static int
+vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
- /* dump IDE hdds if present */
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
- bool error = false;
+ size_t sdCount = 0, i;
int diskCount = 0;
- size_t i;
- PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
- PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
+ int ret = -1;
def->ndisks = 0;
gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
-
gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+ gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
/* get the number of attachments */
for (i = 0; i < mediumAttachments.count; i++) {
@@ -3300,24 +3225,19 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
}
From here....
/* 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;
- }
- def->disks[i] = disk;
- }
- } else {
- error = true;
- }
+ if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
+ goto cleanup;
- if (!error)
- error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
maxSlotPerPort);
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+ if (!disk)
+ goto cleanup;
+
+ def->disks[i] = disk;
+ }>
/* get the attachment details here */
- for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks
&& !error; i++) {
+ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
... to here should be a separate patch after the rename function and
change to int patch. It's just cleaning up the logic a bit, but now
that you have a cleanup label @error is unnecessary.
Of course it'd need to keep the vboxGetMaxPortSlotValues which would
then be removed on the subsequent patch...
IMediumAttachment *imediumattach =
mediumAttachments.items[i];
IStorageController *storageController = NULL;
PRUnichar *storageControllerName = NULL;
@@ -3327,7 +3247,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
IMedium *medium = NULL;
PRUnichar *mediumLocUtf16 = NULL;
char *mediumLocUtf8 = NULL;
- PRUint32 deviceInst = 0;
PRInt32 devicePort = 0;
PRInt32 deviceSlot = 0;
@@ -3363,16 +3282,36 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (!virDomainDiskGetSource(def->disks[diskCount])) {
VBOX_RELEASE(medium);
VBOX_RELEASE(storageController);
- error = true;
- break;
+
+ goto cleanup;
}
gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
+ gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+ gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+
+ def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
+ devicePort,
+ deviceSlot,
+ sdCount);
+ if (!def->disks[diskCount]->dst) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not generate medium name for the disk "
+ "at: port:%d, slot:%d"), devicePort,
deviceSlot);
+ VBOX_RELEASE(medium);
+ VBOX_RELEASE(storageController);
+
+ goto cleanup;
+ }
+
if (storageBus == StorageBus_IDE) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
+ sdCount++;
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI) {
+ } else if (storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS) {
+ sdCount++;
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
@@ -3386,24 +3325,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
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;
- }
gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
if (readOnly == PR_TRUE)
@@ -3417,15 +3338,20 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
diskCount++;
}
+ ret = 0;
+
+ cleanup:
gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
/* cleanup on error */
- if (error) {
+ if (ret < 0) {
for (i = 0; i < def->ndisks; i++)
VIR_FREE(def->disks[i]);
VIR_FREE(def->disks);
def->ndisks = 0;
Similar to my note about controllers in patch 11, this is now
unnecessary... With the void function, I can see why, but it'll be
cleaned up properly now anyway, so it's unnecessary.
}
+
+ return ret;
}
static int
@@ -4120,7 +4046,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
if (vboxDumpStorageControllers(def, machine) < 0)
goto cleanup;
- vboxDumpIDEHDDs(def, data, machine);
+ if (vboxDumpDisks(def, data, machine) < 0)
+ goto cleanup;
vboxDumpSharedFolders(def, data, machine);
vboxDumpNetwork(def, data, machine, networkAdapterCount);
@@ -5676,8 +5603,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data,
return snapshot;
}
Other than considering whether the split out or not, these looked OK to
me - a lot going on, but it's the same stuff and looks reasonable.
John
[...]
12:55 p.m.
New subject: [libvirt] [PATCH v2 12/15] vbox: Correctly generate drive name in dumpxml
On Fri, 2017-11-03 at 09:52 -0400, John Ferlan wrote:
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML
> generated
> by dumpxml used to produce "sda" for both of those disks. This is
> an
> invalid domain XML as libvirt does not allow duplicate device
> names. To
> address this, keep the running total of disks that will use "sd"
> prefix
> for device name and pass it to the vboxGenerateMediumName which no
> longer tries to "compute" the value based only on current and max
> port and slot values. After this the vboxGetMaxPortSlotValues is
> not
> needed and was deleted.
> ---
> src/vbox/vbox_common.c | 414 +++++++++++++++++++++--------------
> --------------
> 1 file changed, 177 insertions(+), 237 deletions(-)
>
I think this needs a bit more splitting.
1. As noted in patch 10, managing StorageBus_SAS should already be
separated so that it doesn't show as a new range/comparison check
here
2. The rename of vboxDumpIDEHDDs to vboxDumpDisks and change from
void
to int should be its own patch. While it seems superfluous, it makes
review that much easier. NB: I have a couple more thoughts within
the
function about
3. I think the changes to vboxSnapshotGetReadWriteDisks and
vboxSnapshotGetReadOnlyDisks could be in a separate patch and is
where
the vboxGetMaxPortSlotValues would get deleted. Although I'm not 100%
sure w/r/t the relationship. Perhaps once vboxDumpDisks is cleaned up
a
bit before making the change described in the commit message it'd be
clearer. Just a lot going on.
Hi John,
I've just send V3 [1] when I separated most of the changes into
individual patches as you requested. However, I could not separate
vboxSnapshotGetReadWrite and vboxSnapshotGetReadOnly disks completely
as the changes made to vboxGenerateMedium name must be combined with
the removal of vboxGetMaxPortSlotValues so instead, I've isolated the
code movement changes where I moved the code to read VBOX's storage
controller, slot and port to the beginning of the loop body - either
way, I think it's much cleaner and easier to follow the changes now.
[1] https://www.redhat.com/archives/libvir-list/2017-November/msg00252.
html
Dawid
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 715eb670e..9dc36a1b2 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -290,61 +290,6 @@ 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(sysPr
> ops,
> - Stora
> geBus_IDE,
> - &maxP
> ortPerInst[StorageBus_IDE]);
> - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> ops,
> - Stora
> geBus_SATA,
> - &maxP
> ortPerInst[StorageBus_SATA]);
> - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> ops,
> - Stora
> geBus_SCSI,
> - &maxP
> ortPerInst[StorageBus_SCSI]);
> - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> ops,
> - Stora
> geBus_Floppy,
> - &maxP
> ortPerInst[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_Floppy,
> -
> &maxSlotPerPort[StorageBus_Floppy]);
> -
> - VBOX_RELEASE(sysProps);
> -
> - return true;
> -}
>
> /**
> * function to generate the name for medium,
> @@ -352,57 +297,40 @@ static bool
> vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> *
> * @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_Floppy))
> + (storageBus > StorageBus_SAS))
> return NULL;
>
> - maxPortPerInst = aMaxPortPerInst[storageBus];
> - maxSlotPerPort = aMaxSlotPerPort[storageBus];
> - total = (deviceInst * maxPortPerInst * maxSlotPerPort)
> - + (devicePort * maxSlotPerPort)
> - + deviceSlot;
> -
> if (storageBus == StorageBus_IDE) {
> prefix = "hd";
> - } else if ((storageBus == StorageBus_SATA) ||
> - (storageBus == StorageBus_SCSI)) {
> + 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;
> }
>
> @@ -3270,20 +3198,17 @@ vboxDumpStorageControllers(virDomainDefPtr
> def, IMachine *machine)
> }
>
>
> -static void
> -vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
> *machine)
> +static int
> +vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
> *machine)
> {
> - /* dump IDE hdds if present */
> vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
> - bool error = false;
> + size_t sdCount = 0, i;
> int diskCount = 0;
> - size_t i;
> - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> + int ret = -1;
>
> def->ndisks = 0;
> gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
> - gVBoxAPI.UArray.handleMachineGetM
> ediumAttachments(machine));
> + gVBoxAPI.UArray.handleMachineGetMediumAttachments
> (machine));
>
> /* get the number of attachments */
> for (i = 0; i < mediumAttachments.count; i++) {
> @@ -3300,24 +3225,19 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> }
>
From here....
> /* 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;
> - }
> - def->disks[i] = disk;
> - }
> - } else {
> - error = true;
> - }
> + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
> + goto cleanup;
>
> - if (!error)
> - error = !vboxGetMaxPortSlotValues(data->vboxObj,
> maxPortPerInst, maxSlotPerPort);
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
> + if (!disk)
> + goto cleanup;
> +
> + def->disks[i] = disk;
> + }>
> /* get the attachment details here */
> - for (i = 0; i < mediumAttachments.count && diskCount < def-
> >ndisks && !error; i++) {
> + for (i = 0; i < mediumAttachments.count && diskCount < def-
> >ndisks; i++) {
... to here should be a separate patch after the rename function and
change to int patch. It's just cleaning up the logic a bit, but now
that you have a cleanup label @error is unnecessary.
Of course it'd need to keep the vboxGetMaxPortSlotValues which would
then be removed on the subsequent patch...
> IMediumAttachment *imediumattach =
> mediumAttachments.items[i];
> IStorageController *storageController = NULL;
> PRUnichar *storageControllerName = NULL;
> @@ -3327,7 +3247,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> IMedium *medium = NULL;
> PRUnichar *mediumLocUtf16 = NULL;
> char *mediumLocUtf8 = NULL;
> - PRUint32 deviceInst = 0;
> PRInt32 devicePort = 0;
> PRInt32 deviceSlot = 0;
>
> @@ -3363,16 +3282,36 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> if (!virDomainDiskGetSource(def->disks[diskCount])) {
> VBOX_RELEASE(medium);
> VBOX_RELEASE(storageController);
> - error = true;
> - break;
> +
> + goto cleanup;
> }
>
> gVBoxAPI.UIStorageController.GetBus(storageController,
> &storageBus);
> + gVBoxAPI.UIMediumAttachment.GetPort(imediumattach,
> &devicePort);
> + gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach,
> &deviceSlot);
> +
> + def->disks[diskCount]->dst =
> vboxGenerateMediumName(storageBus,
> + device
> Port,
> + device
> Slot,
> + sdCoun
> t);
> + if (!def->disks[diskCount]->dst) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not generate medium name for
> the disk "
> + "at: port:%d, slot:%d"), devicePort,
> deviceSlot);
> + VBOX_RELEASE(medium);
> + VBOX_RELEASE(storageController);
> +
> + goto cleanup;
> + }
> +
> if (storageBus == StorageBus_IDE) {
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
> } else if (storageBus == StorageBus_SATA) {
> + sdCount++;
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
> - } else if (storageBus == StorageBus_SCSI) {
> + } else if (storageBus == StorageBus_SCSI ||
> + storageBus == StorageBus_SAS) {
> + sdCount++;
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> } else if (storageBus == StorageBus_Floppy) {
> def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
> @@ -3386,24 +3325,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> 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,
> - device
> Inst,
> - device
> Port,
> - device
> Slot,
> - maxPor
> tPerInst,
> - maxSlo
> tPerPort);
> - 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;
> - }
>
> gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
> if (readOnly == PR_TRUE)
> @@ -3417,15 +3338,20 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> vboxDriverPtr data, IMachine *machine)
> diskCount++;
> }
>
> + ret = 0;
> +
> + cleanup:
> gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
>
> /* cleanup on error */
> - if (error) {
> + if (ret < 0) {
> for (i = 0; i < def->ndisks; i++)
> VIR_FREE(def->disks[i]);
> VIR_FREE(def->disks);
> def->ndisks = 0;
Similar to my note about controllers in patch 11, this is now
unnecessary... With the void function, I can see why, but it'll be
cleaned up properly now anyway, so it's unnecessary.
> }
> +
> + return ret;
> }
>
> static int
> @@ -4120,7 +4046,8 @@ static char
> *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
> if (vboxDumpStorageControllers(def, machine) < 0)
> goto cleanup;
>
> - vboxDumpIDEHDDs(def, data, machine);
> + if (vboxDumpDisks(def, data, machine) < 0)
> + goto cleanup;
>
> vboxDumpSharedFolders(def, data, machine);
> vboxDumpNetwork(def, data, machine, networkAdapterCount);
> @@ -5676,8 +5603,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data,
> return snapshot;
> }
>
Other than considering whether the split out or not, these looked OK
to
me - a lot going on, but it's the same stuff and looks reasonable.
John
[...]
2:35 p.m.
New subject: [libvirt] [PATCH v2 13/15] vbox: Cleanup vboxDumpDisks implementation
Primer the code for further changes:
* move variable declarations to the top of the function
* group together free/release statements
* error check and report VBOX API calls used
---
src/vbox/vbox_common.c | 188 +++++++++++++++++++++++++++++++------------------
1 file changed, 120 insertions(+), 68 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 9dc36a1b2..ee6421aae 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3202,6 +3202,16 @@ static int
vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
+ IMediumAttachment *mediumAttachment = NULL;
+ IMedium *medium = NULL;
+ IStorageController *controller = NULL;
+ PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL;
+ PRUint32 deviceType, storageBus;
+ PRInt32 devicePort, deviceSlot;
+ PRBool readOnly;
+ nsresult rc;
+ virDomainDiskDefPtr disk = NULL;
+ char *mediumLocUtf8 = NULL;
size_t sdCount = 0, i;
int diskCount = 0;
int ret = -1;
@@ -3212,15 +3222,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
/* get the number of attachments */
for (i = 0; i < mediumAttachments.count; i++) {
- IMediumAttachment *imediumattach = mediumAttachments.items[i];
- if (imediumattach) {
- IMedium *medium = NULL;
+ mediumAttachment = mediumAttachments.items[i];
+ if (!mediumAttachment)
+ continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
- if (medium) {
- def->ndisks++;
- VBOX_RELEASE(medium);
- }
+ gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+ if (medium) {
+ def->ndisks++;
+ VBOX_RELEASE(medium);
}
}
@@ -3229,7 +3238,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
for (i = 0; i < def->ndisks; i++) {
- virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+ disk = virDomainDiskDefNew(NULL);
if (!disk)
goto cleanup;
@@ -3238,104 +3247,141 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
/* get the attachment details here */
for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
- IMediumAttachment *imediumattach = 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;
- PRInt32 devicePort = 0;
- PRInt32 deviceSlot = 0;
-
- if (!imediumattach)
+ mediumAttachment = mediumAttachments.items[i];
+ controller = NULL;
+ controllerName = NULL;
+ deviceType = DeviceType_Null;
+ storageBus = StorageBus_Null;
+ readOnly = PR_FALSE;
+ medium = NULL;
+ mediumLocUtf16 = NULL;
+ mediumLocUtf8 = NULL;
+ devicePort = 0;
+ deviceSlot = 0;
+ disk = def->disks[diskCount];
+
+ if (!mediumAttachment)
continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get IMedium, rc=%08x"), rc);
+ goto cleanup;
+ }
+
if (!medium)
continue;
- gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
- if (!storageControllerName) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
+ &controllerName);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to get storage controller name,
rc=%08x"),
+ rc);
+ goto cleanup;
}
- gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
- storageControllerName,
- &storageController);
- VBOX_UTF16_FREE(storageControllerName);
- if (!storageController) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ controllerName,
+ &controller);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get storage controller by name,
rc=%08x"),
+ rc);
+ goto cleanup;
+ }
+
+ rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get medium storage location,
rc=%08x"),
+ rc);
+ goto cleanup;
}
- 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);
- if (!virDomainDiskGetSource(def->disks[diskCount])) {
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
+ if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not set disk source"));
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get device type, rc=%08x"), rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumAttachment, &devicePort);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get device port, rc=%08x"), rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumAttachment, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get device slot, rc=%08x"), rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get storage controller bus, rc=%08x"),
+ rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get read only state, rc=%08x"), rc);
goto cleanup;
}
- gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
- gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
- gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+ disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
+ sdCount);
- def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
- devicePort,
- deviceSlot,
- sdCount);
- if (!def->disks[diskCount]->dst) {
+ if (!disk->dst) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not generate medium name for the disk "
"at: port:%d, slot:%d"), devicePort, deviceSlot);
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
-
goto cleanup;
}
if (storageBus == StorageBus_IDE) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
+ disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
sdCount++;
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
} else if (storageBus == StorageBus_SCSI ||
storageBus == StorageBus_SAS) {
sdCount++;
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
+ disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
}
- 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;
+ 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);
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
diskCount++;
+
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_FREE(mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
+ VBOX_RELEASE(medium);
+ VBOX_RELEASE(controller);
}
ret = 0;
@@ -3345,6 +3391,12 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
/* cleanup on error */
if (ret < 0) {
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_FREE(mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
+ VBOX_RELEASE(medium);
+ VBOX_RELEASE(controller);
+
for (i = 0; i < def->ndisks; i++)
VIR_FREE(def->disks[i]);
VIR_FREE(def->disks);
--
2.14.2
8:54 a.m.
New subject: [libvirt] [PATCH v2 13/15] vbox: Cleanup vboxDumpDisks implementation
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Primer the code for further changes:
* move variable declarations to the top of the function
* group together free/release statements
* error check and report VBOX API calls used
---
src/vbox/vbox_common.c | 188 +++++++++++++++++++++++++++++++------------------
1 file changed, 120 insertions(+), 68 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 9dc36a1b2..ee6421aae 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3202,6 +3202,16 @@ static int
vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
{
vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
+ IMediumAttachment *mediumAttachment = NULL;
+ IMedium *medium = NULL;
+ IStorageController *controller = NULL;
+ PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL;
+ PRUint32 deviceType, storageBus;
+ PRInt32 devicePort, deviceSlot;
+ PRBool readOnly;
+ nsresult rc;
+ virDomainDiskDefPtr disk = NULL;
+ char *mediumLocUtf8 = NULL;
size_t sdCount = 0, i;
int diskCount = 0;
int ret = -1;
@@ -3212,15 +3222,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
/* get the number of attachments */
for (i = 0; i < mediumAttachments.count; i++) {
- IMediumAttachment *imediumattach = mediumAttachments.items[i];
- if (imediumattach) {
- IMedium *medium = NULL;
+ mediumAttachment = mediumAttachments.items[i];
+ if (!mediumAttachment)
+ continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
- if (medium) {
- def->ndisks++;
- VBOX_RELEASE(medium);
- }
+ gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+ if (medium) {
+ def->ndisks++;
+ VBOX_RELEASE(medium);
}
}
@@ -3229,7 +3238,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
for (i = 0; i < def->ndisks; i++) {
- virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+ disk = virDomainDiskDefNew(NULL);
if (!disk)
goto cleanup;
@@ -3238,104 +3247,141 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
/* get the attachment details here */
for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
- IMediumAttachment *imediumattach = 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;
- PRInt32 devicePort = 0;
- PRInt32 deviceSlot = 0;
-
- if (!imediumattach)
+ mediumAttachment = mediumAttachments.items[i];
+ controller = NULL;
+ controllerName = NULL;
+ deviceType = DeviceType_Null;
+ storageBus = StorageBus_Null;
+ readOnly = PR_FALSE;
+ medium = NULL;
+ mediumLocUtf16 = NULL;
+ mediumLocUtf8 = NULL;
+ devicePort = 0;
+ deviceSlot = 0;
+ disk = def->disks[diskCount];
+
+ if (!mediumAttachment)
continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get IMedium, rc=%08x"), rc);
+ goto cleanup;
+ }
+
if (!medium)
continue;
- gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
&storageControllerName);
- if (!storageControllerName) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
+ &controllerName);
^^^^^
Alignment for second line... Needs 5 more spaces right...
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to get storage controller name,
rc=%08x"),
+ rc);
+ goto cleanup;
}
- gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
- storageControllerName,
- &storageController);
- VBOX_UTF16_FREE(storageControllerName);
- if (!storageController) {
- VBOX_RELEASE(medium);
- continue;
+ rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+ controllerName,
+ &controller);
One more space each for arg2 and arg3
The rest looks OK... with the above adjustments...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get storage controller by name,
rc=%08x"),
+ rc);
+ goto cleanup;
+ }
+
+ rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get medium storage location,
rc=%08x"),
+ rc);
+ goto cleanup;
}
- 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);
- if (!virDomainDiskGetSource(def->disks[diskCount])) {
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
+ if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not set disk source"));
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get device type, rc=%08x"), rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumAttachment, &devicePort);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get device port, rc=%08x"), rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumAttachment, &deviceSlot);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get device slot, rc=%08x"), rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get storage controller bus,
rc=%08x"),
+ rc);
+ goto cleanup;
+ }
+ rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get read only state, rc=%08x"), rc);
goto cleanup;
}
- gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
- gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
- gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+ disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
+ sdCount);
- def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
- devicePort,
- deviceSlot,
- sdCount);
- if (!def->disks[diskCount]->dst) {
+ if (!disk->dst) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not generate medium name for the disk "
"at: port:%d, slot:%d"), devicePort,
deviceSlot);
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
-
goto cleanup;
}
if (storageBus == StorageBus_IDE) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
+ disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
} else if (storageBus == StorageBus_SATA) {
sdCount++;
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
} else if (storageBus == StorageBus_SCSI ||
storageBus == StorageBus_SAS) {
sdCount++;
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
} else if (storageBus == StorageBus_Floppy) {
- def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
+ disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
}
- 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;
+ 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);
- VBOX_RELEASE(medium);
- VBOX_RELEASE(storageController);
diskCount++;
+
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_FREE(mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
+ VBOX_RELEASE(medium);
+ VBOX_RELEASE(controller);
}
ret = 0;
@@ -3345,6 +3391,12 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
/* cleanup on error */
if (ret < 0) {
+ VBOX_UTF16_FREE(controllerName);
+ VBOX_UTF8_FREE(mediumLocUtf8);
+ VBOX_UTF16_FREE(mediumLocUtf16);
+ VBOX_RELEASE(medium);
+ VBOX_RELEASE(controller);
+
for (i = 0; i < def->ndisks; i++)
VIR_FREE(def->disks[i]);
VIR_FREE(def->disks);
2:35 p.m.
New subject: [libvirt] [PATCH v2 14/15] vbox: Process empty removable disks in dumpxml
Previously any removable storage device without media attached was
omitted from domain XML dump. They're still (rightfully) omitted in
snapshot XMl dump but need to be accounted properly to for the device
names to stay in 'sync' between domain and snapshot XML dumps.
---
src/vbox/vbox_common.c | 128 ++++++++++++++++++++++++++++---------------------
1 file changed, 74 insertions(+), 54 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index ee6421aae..d1d8804c7 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3213,7 +3213,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
virDomainDiskDefPtr disk = NULL;
char *mediumLocUtf8 = NULL;
size_t sdCount = 0, i;
- int diskCount = 0;
int ret = -1;
def->ndisks = 0;
@@ -3226,11 +3225,15 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (!mediumAttachment)
continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
- if (medium) {
- def->ndisks++;
- VBOX_RELEASE(medium);
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get IMedium, rc=%08x"), rc);
+ goto cleanup;
}
+
+ def->ndisks++;
+ VBOX_RELEASE(medium);
}
/* Allocate mem, if fails return error */
@@ -3246,7 +3249,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
}
/* get the attachment details here */
- for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
+ for (i = 0; i < mediumAttachments.count; i++) {
mediumAttachment = mediumAttachments.items[i];
controller = NULL;
controllerName = NULL;
@@ -3258,7 +3261,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
mediumLocUtf8 = NULL;
devicePort = 0;
deviceSlot = 0;
- disk = def->disks[diskCount];
+ disk = def->disks[i];
if (!mediumAttachment)
continue;
@@ -3270,9 +3273,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
}
- if (!medium)
- continue;
-
rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
&controllerName);
if (NS_FAILED(rc)) {
@@ -3292,22 +3292,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
}
- rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not get medium storage location,
rc=%08x"),
- rc);
- goto cleanup;
- }
-
- VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
-
- if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Could not set disk source"));
- goto cleanup;
- }
-
rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3333,11 +3317,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
rc);
goto cleanup;
}
- rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not get read only state, rc=%08x"), rc);
- goto cleanup;
+
+ if (medium) {
+ rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get medium storage location,
rc=%08x"),
+ rc);
+ goto cleanup;
+ }
+
+ VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+
+ if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not set disk source"));
+ goto cleanup;
+ }
+
+ rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get read only state, rc=%08x"),
rc);
+ goto cleanup;
+ }
}
disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
@@ -3375,8 +3378,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
- diskCount++;
-
VBOX_UTF16_FREE(controllerName);
VBOX_UTF8_FREE(mediumLocUtf8);
VBOX_UTF16_FREE(mediumLocUtf16);
@@ -5814,6 +5815,14 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
/* skip empty removable disk */
if (!disk) {
+ /* removable disks with empty (ejected) media won't be displayed
+ * in XML, but we need to update "sdCount" so that device names
match
+ * in domain dumpxml and snapshot dumpxml
+ */
+ if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+
VBOX_RELEASE(storageController);
continue;
}
@@ -5982,14 +5991,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
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",
@@ -6009,19 +6010,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
}
if (!storageController)
continue;
- 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.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -6040,6 +6028,38 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
_("Cannot get device slot"));
goto cleanup;
}
+
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot get medium"));
+ goto cleanup;
+ }
+
+ /* skip empty removable disk */
+ if (!disk) {
+ /* removable disks with empty (ejected) media won't be displayed
+ * in XML, but we need to update "sdCount" so that device names
match
+ * in domain dumpxml and snapshot dumpxml
+ */
+ if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+ continue;
+ }
+
+ 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",
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH v2 14/15] vbox: Process empty removable disks in dumpxml
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Previously any removable storage device without media attached was
omitted from domain XML dump. They're still (rightfully) omitted in
snapshot XMl dump but need to be accounted properly to for the device
XML
names to stay in 'sync' between domain and snapshot XML
dumps.
---
src/vbox/vbox_common.c | 128 ++++++++++++++++++++++++++++---------------------
1 file changed, 74 insertions(+), 54 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index ee6421aae..d1d8804c7 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3213,7 +3213,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
virDomainDiskDefPtr disk = NULL;
char *mediumLocUtf8 = NULL;
size_t sdCount = 0, i;
- int diskCount = 0;
int ret = -1;
def->ndisks = 0;
@@ -3226,11 +3225,15 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
if (!mediumAttachment)
continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
- if (medium) {
- def->ndisks++;
- VBOX_RELEASE(medium);
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get IMedium, rc=%08x"), rc);
+ goto cleanup;
}
+
+ def->ndisks++;
+ VBOX_RELEASE(medium);
}
/* Allocate mem, if fails return error */
@@ -3246,7 +3249,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
}
/* get the attachment details here */
- for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks;
i++) {
+ for (i = 0; i < mediumAttachments.count; i++) {
mediumAttachment = mediumAttachments.items[i];
controller = NULL;
controllerName = NULL;
@@ -3258,7 +3261,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
mediumLocUtf8 = NULL;
devicePort = 0;
deviceSlot = 0;
- disk = def->disks[diskCount];
+ disk = def->disks[i];
if (!mediumAttachment)
continue;
@@ -3270,9 +3273,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
}
- if (!medium)
- continue;
-
rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
&controllerName);
if (NS_FAILED(rc)) {
@@ -3292,22 +3292,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
}
- rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not get medium storage location,
rc=%08x"),
- rc);
- goto cleanup;
- }
-
- VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
-
- if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Could not set disk source"));
- goto cleanup;
- }
-
rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3333,11 +3317,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
rc);
goto cleanup;
}
- rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
- if (NS_FAILED(rc)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not get read only state, rc=%08x"), rc);
- goto cleanup;
+
+ if (medium) {
+ rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get medium storage location,
rc=%08x"),
+ rc);
+ goto cleanup;
+ }
+
+ VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+
+ if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not set disk source"));
+ goto cleanup;
+ }
+
+ rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get read only state, rc=%08x"),
rc);
+ goto cleanup;
+ }
}
disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
@@ -3375,8 +3378,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
- diskCount++;
-
VBOX_UTF16_FREE(controllerName);
VBOX_UTF8_FREE(mediumLocUtf8);
VBOX_UTF16_FREE(mediumLocUtf16);
@@ -5814,6 +5815,14 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
/* skip empty removable disk */
if (!disk) {
+ /* removable disks with empty (ejected) media won't be displayed
+ * in XML, but we need to update "sdCount" so that device names
match
+ * in domain dumpxml and snapshot dumpxml
+ */
+ if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+
VBOX_RELEASE(storageController);
continue;
}
@@ -5982,14 +5991,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
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;
Can the changes that are moving the medium code to be all together go
into a patch just before this one? Then the hunk here just becomes
adding the sdCount++ adjustment when !disk - just like all that changed
for vboxSnapshotGetReadWriteDisks.
Tks -
John
rc =
gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -6009,19 +6010,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
}
if (!storageController)
continue;
- 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.UIStorageController.GetBus(storageController, &storageBus);
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -6040,6 +6028,38 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
_("Cannot get device slot"));
goto cleanup;
}
+
+ rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+ if (NS_FAILED(rc)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot get medium"));
+ goto cleanup;
+ }
+
+ /* skip empty removable disk */
+ if (!disk) {
+ /* removable disks with empty (ejected) media won't be displayed
+ * in XML, but we need to update "sdCount" so that device names
match
+ * in domain dumpxml and snapshot dumpxml
+ */
+ if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI ||
+ storageBus == StorageBus_SAS)
+ sdCount++;
+ continue;
+ }
+
+ 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",
2:35 p.m.
New subject: [libvirt] [PATCH v2 15/15] vbox: Generate disk address element in dumpxml
This patch adds <address> element to each <disk> device since device
names alone won't adequately reflect the storage device layout in the
VM. With this patch, the ouput produced by dumpxml will faithfully
reproduce the storage layout of the VM if used with define.
---
src/vbox/vbox_common.c | 79 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 68 insertions(+), 11 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index d1d8804c7..95d35631f 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3211,8 +3211,9 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
PRBool readOnly;
nsresult rc;
virDomainDiskDefPtr disk = NULL;
+ virDomainControllerDefPtr ctrl = NULL;
char *mediumLocUtf8 = NULL;
- size_t sdCount = 0, i;
+ size_t sdCount = 0, i, j;
int ret = -1;
def->ndisks = 0;
@@ -3353,26 +3354,83 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
}
- if (storageBus == StorageBus_IDE) {
+ disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ disk->info.addr.drive.bus = 0;
+ disk->info.addr.drive.unit = devicePort;
+
+ switch ((enum StorageBus) storageBus) {
+ case StorageBus_IDE:
disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
- } else if (storageBus == StorageBus_SATA) {
- sdCount++;
+ disk->info.addr.drive.bus = devicePort; /* primary, secondary */
+ disk->info.addr.drive.unit = deviceSlot; /* master, slave */
+
+ break;
+ case StorageBus_SATA:
disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI ||
- storageBus == StorageBus_SAS) {
sdCount++;
+
+ break;
+ case StorageBus_SCSI:
+ case StorageBus_SAS:
disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
- } else if (storageBus == StorageBus_Floppy) {
+ sdCount++;
+
+ /* In vbox, if there's a disk attached to SAS controller, there will
+ * be libvirt SCSI controller present with model "lsi1068", and we
+ * need to find its index
+ */
+ for (j = 0; j < def->ncontrollers; j++) {
+ 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;
+ break;
+ }
+
+ if (storageBus == StorageBus_SCSI &&
+ ctrl->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ disk->info.addr.drive.controller = ctrl->idx;
+ break;
+ }
+ }
+
+ break;
+ case StorageBus_Floppy:
disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+
+ break;
+ case StorageBus_Null:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unsupported null storage bus"));
+ goto cleanup;
}
- if (deviceType == DeviceType_HardDisk)
+ switch ((enum DeviceType) deviceType) {
+ case DeviceType_HardDisk:
disk->device = VIR_DOMAIN_DISK_DEVICE_DISK;
- else if (deviceType == DeviceType_Floppy)
+
+ break;
+ case DeviceType_Floppy:
disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
- else if (deviceType == DeviceType_DVD)
+
+ break;
+ case DeviceType_DVD:
disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
+ break;
+ case DeviceType_Network:
+ case DeviceType_USB:
+ case DeviceType_SharedFolder:
+ case DeviceType_Null:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unsupported vbox device type: %d"), deviceType);
+ goto cleanup;
+ }
+
if (readOnly == PR_TRUE)
disk->src->readonly = true;
@@ -4098,7 +4156,6 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
goto cleanup;
if (vboxDumpStorageControllers(def, machine) < 0)
goto cleanup;
-
if (vboxDumpDisks(def, data, machine) < 0)
goto cleanup;
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH v2 15/15] vbox: Generate disk address element in dumpxml
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This patch adds <address> element to each <disk> device
since device
names alone won't adequately reflect the storage device layout in the
VM. With this patch, the ouput produced by dumpxml will faithfully
reproduce the storage layout of the VM if used with define.
---
src/vbox/vbox_common.c | 79 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 68 insertions(+), 11 deletions(-)
This one seems OK, but I didn't think long about it. I'll look again
when the v3 is posted.
What I'll also ask is that when v3 hits, please be sure to create a
docs/news.xml article that briefly describes some of the fixes you've
made in this series that would go into the next release.
Tks -
John
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index d1d8804c7..95d35631f 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3211,8 +3211,9 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
PRBool readOnly;
nsresult rc;
virDomainDiskDefPtr disk = NULL;
+ virDomainControllerDefPtr ctrl = NULL;
char *mediumLocUtf8 = NULL;
- size_t sdCount = 0, i;
+ size_t sdCount = 0, i, j;
int ret = -1;
def->ndisks = 0;
@@ -3353,26 +3354,83 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
*machine)
goto cleanup;
}
- if (storageBus == StorageBus_IDE) {
+ disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ disk->info.addr.drive.bus = 0;
+ disk->info.addr.drive.unit = devicePort;
+
+ switch ((enum StorageBus) storageBus) {
+ case StorageBus_IDE:
disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
- } else if (storageBus == StorageBus_SATA) {
- sdCount++;
+ disk->info.addr.drive.bus = devicePort; /* primary, secondary */
+ disk->info.addr.drive.unit = deviceSlot; /* master, slave */
+
+ break;
+ case StorageBus_SATA:
disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
- } else if (storageBus == StorageBus_SCSI ||
- storageBus == StorageBus_SAS) {
sdCount++;
+
+ break;
+ case StorageBus_SCSI:
+ case StorageBus_SAS:
disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
- } else if (storageBus == StorageBus_Floppy) {
+ sdCount++;
+
+ /* In vbox, if there's a disk attached to SAS controller, there will
+ * be libvirt SCSI controller present with model "lsi1068", and
we
+ * need to find its index
+ */
+ for (j = 0; j < def->ncontrollers; j++) {
+ 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;
+ break;
+ }
+
+ if (storageBus == StorageBus_SCSI &&
+ ctrl->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+ disk->info.addr.drive.controller = ctrl->idx;
+ break;
+ }
+ }
+
+ break;
+ case StorageBus_Floppy:
disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+
+ break;
+ case StorageBus_Null:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unsupported null storage bus"));
+ goto cleanup;
}
- if (deviceType == DeviceType_HardDisk)
+ switch ((enum DeviceType) deviceType) {
+ case DeviceType_HardDisk:
disk->device = VIR_DOMAIN_DISK_DEVICE_DISK;
- else if (deviceType == DeviceType_Floppy)
+
+ break;
+ case DeviceType_Floppy:
disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
- else if (deviceType == DeviceType_DVD)
+
+ break;
+ case DeviceType_DVD:
disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
+ break;
+ case DeviceType_Network:
+ case DeviceType_USB:
+ case DeviceType_SharedFolder:
+ case DeviceType_Null:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unsupported vbox device type: %d"),
deviceType);
+ goto cleanup;
+ }
+
if (readOnly == PR_TRUE)
disk->src->readonly = true;
@@ -4098,7 +4156,6 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int
flags)
goto cleanup;
if (vboxDumpStorageControllers(def, machine) < 0)
goto cleanup;
-
if (vboxDumpDisks(def, data, machine) < 0)
goto cleanup;
2571
days inactive
2585
days old
36 comments
2 participants
participants (2)
-
Dawid Zamirski -
John Ferlan