Re: [libvirt] [PATCH v3 REBASE 0/2] qemu: report block job errors from qemu to the user

[Nikolay Shirokovskiy]

ping

On 08.09.2017 10:59, Nikolay Shirokovskiy wrote:
> So that you can see nice report on migration:
> 
> "error: operation failed: migration of disk sda failed: No space left on 
> device"
> 
> diff from v2:
> 
> 1. split into 2 patches
> 2. change formal documentation where it is present accordingly
> 3. add variable initialization for safety
> 
> Nikolay Shirokovskiy (2):
>   qemu: prepare blockjob complete event error usage
>   qemu: report drive mirror errors on migration
> 
>  src/qemu/qemu_blockjob.c | 14 +--
>  src/qemu/qemu_blockjob.h |  3 ++-
>  src/qemu/qemu_domain.c   |  1 +
>  src/qemu/qemu_domain.h   |  1 +
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/qemu/qemu_migration.c| 55 
> +++-
>  src/qemu/qemu_monitor.c  |  5 ++--
>  src/qemu/qemu_monitor.h  |  4 +++-
>  src/qemu/qemu_monitor_json.c |  4 +++-
>  src/qemu/qemu_process.c  |  4 
>  10 files changed, 70 insertions(+), 25 deletions(-)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/3] Update VRDE server port handling.

[Dawid Zamirski]

v1: https://www.redhat.com/archives/libvir-list/2017-October/msg00421.html

Changes since v1:
 * separate out the patch that drops VBOX_SESSION_OPEN/CLOSE macros.
 * reverse logic to check for rdp.autoport instead of rdp.port to avoid
   handling port = -1
 * do not set rdp.port = -1 if vboxGetActiveRdpPort returns -1 and add
   more verbose code comments to clarify the logic 

Dawid Zamirski (3):
  vbox: Remove old unflexible macros
  vbox: Make autoport set RDP port range.
  vbox: Read runtime RDP port and handle autoport

 src/vbox/vbox_common.c|   3 +-
 src/vbox/vbox_tmpl.c  | 159 ++
 src/vbox/vbox_uniformed_api.h |   2 +-
 3 files changed, 118 insertions(+), 46 deletions(-)

-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/3] vbox: Remove old unflexible macros

[Dawid Zamirski]

The VBOX_SESSION_OPEN/CLOSE macros are only called in
_vboxDomainSnapshotRestore and they are unflexible because:

* assume the caller will have variable named "data"
* can only create Write lock type

As per above, it's not that hard to simply use the VBOX API directly.
---
 src/vbox/vbox_tmpl.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index dffeabde0..ce2ee9037 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -144,12 +144,6 @@ if (strUtf16) {\
   (unsigned)(iid)->m3[7]);\
 }\
 
-#define VBOX_SESSION_OPEN(/* unused */ iid_value, /* in */ machine) \
-machine->vtbl->LockMachine(machine, data->vboxSession, LockType_Write)
-
-#define VBOX_SESSION_CLOSE() \
-data->vboxSession->vtbl->UnlockMachine(data->vboxSession)
-
 #define VBOX_IID_INITIALIZER { NULL, true }
 
 static void
@@ -323,7 +317,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom,
 goto cleanup;
 }
 
-rc = VBOX_SESSION_OPEN(domiid.value, machine);
+rc = machine->vtbl->LockMachine(machine, data->vboxSession, 
LockType_Write);
 #if VBOX_API_VERSION < 500
 if (NS_SUCCEEDED(rc))
 rc = data->vboxSession->vtbl->GetConsole(data->vboxSession, &console);
@@ -368,7 +362,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom,
 #if VBOX_API_VERSION < 500
 VBOX_RELEASE(console);
 #endif /*VBOX_API_VERSION < 500*/
-VBOX_SESSION_CLOSE();
+data->vboxSession->vtbl->UnlockMachine(data->vboxSession);
 vboxIIDUnalloc(&domiid);
 return ret;
 }
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/3] vbox: Make autoport set RDP port range.

[Dawid Zamirski]

From: Dawid Zamirski 

Originally autoport in vbox driver was setting the port to default value
(3389) which caused multiple VM instances use the same port. Since
libvirt XML does not allow to set port ranges, this patch changes the
"autoport" behavior to set VBox's "TCP/Ports" property to an arbitrary
port range (3389-3689) to avoid that issue.
---
 src/vbox/vbox_tmpl.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index ce2ee9037..2b3f2e3eb 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -146,6 +146,9 @@ if (strUtf16) {\
 
 #define VBOX_IID_INITIALIZER { NULL, true }
 
+/* default RDP port range to use for auto-port setting */
+#define VBOX_RDP_AUTOPORT_RANGE "3389-3689"
+
 static void
 _vboxIIDUnalloc(vboxDriverPtr data, vboxIID *iid)
 {
@@ -1595,15 +1598,21 @@ _vrdeServerGetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
 }
 
 static nsresult
-_vrdeServerSetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
-IVRDEServer *VRDEServer, virDomainGraphicsDefPtr graphics)
+_vrdeServerSetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer,
+virDomainGraphicsDefPtr graphics)
 {
 nsresult rc = 0;
 PRUnichar *VRDEPortsKey = NULL;
 PRUnichar *VRDEPortsValue = NULL;
 
 VBOX_UTF8_TO_UTF16("TCP/Ports", &VRDEPortsKey);
-VRDEPortsValue = PRUnicharFromInt(data->pFuncs, graphics->data.rdp.port);
+
+if (graphics->data.rdp.autoport)
+VBOX_UTF8_TO_UTF16(VBOX_RDP_AUTOPORT_RANGE, &VRDEPortsValue);
+else
+VRDEPortsValue = PRUnicharFromInt(data->pFuncs,
+  graphics->data.rdp.port);
+
 rc = VRDEServer->vtbl->SetVRDEProperty(VRDEServer, VRDEPortsKey,
VRDEPortsValue);
 VBOX_UTF16_FREE(VRDEPortsKey);
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[Dawid Zamirski]

VirutalBox has a IVRDEServerInfo structure available that
gives the effective runtime port that the VM is using when it's
running. This is useful when the "TCP/Ports" VBox property was set to
port range (e.g. via autoport = "yes" or via VBoxManage) in which
case it would be impossible to get the "active" port otherwise.
---
 src/vbox/vbox_common.c|   3 +-
 src/vbox/vbox_tmpl.c  | 134 +++---
 src/vbox/vbox_uniformed_api.h |   2 +-
 3 files changed, 104 insertions(+), 35 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 92ee37164..d542f2b76 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3326,7 +3326,8 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 if (VIR_ALLOC(graphics) < 0)
 goto cleanup;
 
-gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, graphics);
+gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, machine, graphics);
+gVBoxAPI.UISession.Close(data->vboxSession);
 
 graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP;
 
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 2b3f2e3eb..c7682f13c 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -221,29 +221,6 @@ _vboxIIDFromArrayItem(vboxDriverPtr data, vboxIID *iid,
 _vboxIIDFromArrayItem(data, iid, array, idx)
 #define DEBUGIID(msg, strUtf16) DEBUGPRUnichar(msg, strUtf16)
 
-/**
- * Converts Utf-16 string to int
- */
-static int PRUnicharToInt(PCVBOXXPCOM pFuncs, PRUnichar *strUtf16)
-{
-char *strUtf8 = NULL;
-int ret = 0;
-
-if (!strUtf16)
-return -1;
-
-pFuncs->pfnUtf16ToUtf8(strUtf16, &strUtf8);
-if (!strUtf8)
-return -1;
-
-if (virStrToLong_i(strUtf8, NULL, 10, &ret) < 0)
-ret = -1;
-
-pFuncs->pfnUtf8Free(strUtf8);
-
-return ret;
-}
-
 /**
  * Converts int to Utf-16 string
  */
@@ -280,6 +257,54 @@ static virDomainState _vboxConvertState(PRUint32 state)
 }
 }
 
+
+static int
+vboxGetActiveVRDEServerPort(ISession *session, IMachine *machine)
+{
+nsresult rc;
+PRInt32 port = -1;
+IVRDEServerInfo *vrdeInfo = NULL;
+IConsole *console = NULL;
+
+rc = machine->vtbl->LockMachine(machine, session, LockType_Shared);
+if (NS_FAILED(rc)) {
+VIR_WARN("Could not obtain shared lock on VBox VM, rc=%08x", rc);
+return -1;
+}
+
+rc = session->vtbl->GetConsole(session, &console);
+if (NS_FAILED(rc)) {
+VIR_WARN("Could not get VBox session console, rc=%08x", rc);
+goto cleanup;
+}
+
+/* it may be null if VM is not running */
+if (!console)
+goto cleanup;
+
+rc = console->vtbl->GetVRDEServerInfo(console, &vrdeInfo);
+
+if (NS_FAILED(rc) || !vrdeInfo) {
+VIR_WARN("Could not get VBox VM VRDEServerInfo, rc=%08x", rc);
+goto cleanup;
+}
+
+rc = vrdeInfo->vtbl->GetPort(vrdeInfo, &port);
+
+if (NS_FAILED(rc)) {
+VIR_WARN("Could not read port from VRDEServerInfo, rc=%08x", rc);
+goto cleanup;
+}
+
+ cleanup:
+VBOX_RELEASE(console);
+VBOX_RELEASE(vrdeInfo);
+session->vtbl->UnlockMachine(session);
+
+return port;
+}
+
+
 static int
 _vboxDomainSnapshotRestore(virDomainPtr dom,
   IMachine *machine,
@@ -1576,24 +1601,67 @@ _vrdeServerSetEnabled(IVRDEServer *VRDEServer, PRBool 
enabled)
 }
 
 static nsresult
-_vrdeServerGetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
-IVRDEServer *VRDEServer, virDomainGraphicsDefPtr graphics)
+_vrdeServerGetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer,
+IMachine *machine, virDomainGraphicsDefPtr graphics)
 {
 nsresult rc;
 PRUnichar *VRDEPortsKey = NULL;
 PRUnichar *VRDEPortsValue = NULL;
+PRInt32 port = -1;
+ssize_t nmatches = 0;
+char **matches = NULL;
+char *portUtf8 = NULL;
+
+/* get active (effective) port - available only when VM is running and has
+ * the VBOX extensions installed (without extenstions RDP server
+ * functionality is disabled)
+ */
+port = vboxGetActiveVRDEServerPort(data->vboxSession, machine);
 
+if (port > 0)
+graphics->data.rdp.port = port;
+
+/* get the port (or port range) set in VM properties, this info will
+ * be used to determine whether to set autoport flag
+ */
 VBOX_UTF8_TO_UTF16("TCP/Ports", &VRDEPortsKey);
-rc = VRDEServer->vtbl->GetVRDEProperty(VRDEServer, VRDEPortsKey, 
&VRDEPortsValue);
-VBOX_UTF16_FREE(VRDEPortsKey);
-if (VRDEPortsValue) {
-/* even if vbox supports mutilpe ports, single port for now here */
-graphics->data.rdp.port = PRUnicharToInt(data->pFuncs, VRDEPortsValue);
-VBOX_UTF16_FREE(VRDEPortsValue);
-} else {
-graphics->data.rdp.autoport = true;
+rc = VRDEServer->vtbl->GetVRDEProperty(VRDEServer, VRDEPortsKey,
+   &VRDEPortsValue);
+
+i

[libvirt] [PATCH v2 13/15] vbox: Cleanup vboxDumpDisks implementation

[Dawid Zamirski]

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)) {
+ 

[libvirt] [PATCH v2 11/15] vbox: Add vboxDumpStorageControllers

[Dawid Zamirski]

---
 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 14/15] vbox: Process empty removable disks in dumpxml

[Dawid Zamirski]

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,
@@ -,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 d

[libvirt] [PATCH v2 12/15] vbox: Correctly generate drive name in dumpxml

[Dawid Zamirski]

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   vboxInput 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   connInput 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

[libvirt] [PATCH v2 06/15] vbox: Errors in vboxAttachDrives are now critical

[Dawid Zamirski]

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 00/15] vbox: Improve handling of storage devices

[Dawid Zamirski]

From: Dawid Zamirski 

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  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 10/15] vbox: Process element in domain XML

[Dawid Zamirski]

This patch enables the VBOX driver to process the  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  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)",
+  

[libvirt] [PATCH v2 01/15] vbox: Update ATTRIBUTE_UNUSED usage

[Dawid Zamirski]

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 < 500
 return vboxObj->vtbl->CreateHardDisk(vboxObj, format, location, medium);
 #elif VBOX_API_VERSION >= 500 /*VBOX_API_VERSION >= 500*/
-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 >= 500*/
 }
 
@@ -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 == 400
-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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 15/15] vbox: Generate disk address element in dumpxml

[Dawid Zamirski]

This patch adds  element to each  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 09/15] domain: Allow 'model' attribute for ide controller

[Dawid Zamirski]

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 @@
  Since 1.3.5, USB controllers accept a
  ports attribute to configure how many devices can be
  connected to the controller.
+ide
+Since 3.9.0 for the vbox driver, the
+ide controller has an optional attribute
+model, which is one of "piix3", "piix4" or "ich6".
   
 
 
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 @@
   
 
 
-  
+  
   
 
   
 fdc
-ide
 sata
 ccid
   
@@ -2041,6 +2040,21 @@
   
 
   
+  
+  
+
+  ide
+
+
+  
+
+  piix3
+  piix4
+  ich6
+
+  
+
+  
   
   
 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 07/15] vbox: Support empty removable drives.

[Dawid Zamirski]

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:
+

[libvirt] [PATCH v2 08/15] vbox: Add more IStorageController API mappings

[Dawid Zamirski]

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation

[Dawid Zamirski]

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,

[libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure

[Dawid Zamirski]

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 02/15] vbox: Close media when undefining domains

[Dawid Zamirski]

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 04/15] vbox: vboxAttachDrives now relies on address info

[Dawid Zamirski]

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  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, &mediumF

[libvirt] [[RFC] 2/8] QEMU Event handling: Introduce async event helpers in qemu_event.[ch]

[Prerna Saxena]

These contain basic functions for setting up event lists (global
as well as per-VM). Also include methods for enqueuing and
dequeuing events.
Per-event metadata is also encoded herewith.

Signed-off-by: Prerna Saxena 
---
 src/Makefile.am   |   1 +
 src/qemu/qemu_event.c |  75 +
 src/qemu/qemu_event.h | 224 ++
 3 files changed, 300 insertions(+)
 create mode 100644 src/qemu/qemu_event.c
 create mode 100644 src/qemu/qemu_event.h

diff --git a/src/Makefile.am b/src/Makefile.am
index b74856b..73a98ca 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -903,6 +903,7 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_domain.c qemu/qemu_domain.h   \
qemu/qemu_domain_address.c qemu/qemu_domain_address.h   \
qemu/qemu_cgroup.c qemu/qemu_cgroup.h   \
+   qemu/qemu_event.c qemu/qemu_event.h \
qemu/qemu_hostdev.c qemu/qemu_hostdev.h \
qemu/qemu_hotplug.c qemu/qemu_hotplug.h \
qemu/qemu_hotplugpriv.h \
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
new file mode 100644
index 000..e27ea0d
--- /dev/null
+++ b/src/qemu/qemu_event.c
@@ -0,0 +1,75 @@
+/*
+ * qemu_event.c:
+ *optimize qemu async event handling.
+ *
+ * Copyright (C) 2017-2026 Nutanix, Inc.
+ * Copyright (C) 2017 Prerna Saxena
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: Prerna Saxena 
+ */
+
+#include "config.h"
+#include "internal.h"
+# include "qemu_monitor.h"
+# include "qemu_conf.h"
+# include "qemu_event.h"
+#include "qemu_process.h"
+
+#include "virerror.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virobject.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+VIR_LOG_INIT("qemu.qemu_event");
+
+VIR_ENUM_IMPL(qemuMonitorEvent,
+  QEMU_EVENT_LAST,
+  "ACPI Event", "Balloon Change", "Block IO Error",
+  "Block Job Event",
+  "Block Write Threshold", "Device Deleted",
+  "Device Tray Moved", "Graphics", "Guest Panicked",
+  "Migration", "Migration pass",
+  "Nic RX Filter Changed", "Powerdown", "Reset", "Resume",
+  "RTC Change", "Shutdown", "Stop",
+  "Suspend", "Suspend To Disk",
+  "Virtual Serial Port Change",
+  "Wakeup", "Watchdog");
+
+virQemuEventList* virQemuEventListInit(void)
+{
+virQemuEventList *ev_list;
+if (VIR_ALLOC(ev_list) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "Unable to allocate virQemuEventList");
+return NULL;
+}
+
+if (virMutexInit(&ev_list->lock) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("cannot initialize mutex"));
+VIR_FREE(ev_list);
+return NULL;
+}
+
+ev_list->head = NULL;
+ev_list->last = NULL;
+
+return ev_list;
+}
diff --git a/src/qemu/qemu_event.h b/src/qemu/qemu_event.h
new file mode 100644
index 000..9781795
--- /dev/null
+++ b/src/qemu/qemu_event.h
@@ -0,0 +1,224 @@
+/*
+ * qemu_event.h: interaction with QEMU JSON monitor event layer
+ * Carve out improved interactions with qemu.
+ *
+ * Copyright (C) 2017-2026 Nutanix, Inc.
+ * Copyright (C) 2017 Prerna Saxena
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: Prerna Saxena 
+ */
+
+
+#ifndef QEMU_EVENT_H
+# define QEMU_EVENT_H
+
+# include "internal.h"
+# include "virobject.h"
+
+typedef enum {
+QEMU_EVENT_ACPI_OST,
+QEMU_E

[libvirt] [[RFC] 4/8] Events: Allow monitor to "enqueue" events to a queue. Also introduce a framework of handlers for each event type, that can be called when the handler is running an event.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_event.c|  11 +
 src/qemu/qemu_event.h|   8 +
 src/qemu/qemu_monitor.c  | 592 +++-
 src/qemu/qemu_monitor.h  |  80 +++--
 src/qemu/qemu_monitor_json.c | 291 ++--
 src/qemu/qemu_process.c  | 789 +++
 src/qemu/qemu_process.h  |   2 +
 tests/qemumonitortestutils.c |   2 +-
 8 files changed, 1273 insertions(+), 502 deletions(-)

diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
index d52fad2..beb309f 100644
--- a/src/qemu/qemu_event.c
+++ b/src/qemu/qemu_event.c
@@ -50,6 +50,7 @@ VIR_ENUM_IMPL(qemuMonitorEvent,
   "RTC Change", "Shutdown", "Stop",
   "Suspend", "Suspend To Disk",
   "Virtual Serial Port Change",
+  "Spice migrated",
   "Wakeup", "Watchdog");
 
 virQemuEventList* virQemuEventListInit(void)
@@ -302,3 +303,13 @@ void virDomainConsumeVMEvents(virDomainObjPtr vm, void 
*opaque)
 }
 return;
 }
+
+extern void qemuProcessEmitMonitorEvent(qemuEventPtr ev, void *opaque);
+
+void virEventRunHandler(qemuEventPtr ev, void *opaque)
+{
+if (!ev)
+return;
+
+return qemuProcessEmitMonitorEvent(ev, opaque);
+}
diff --git a/src/qemu/qemu_event.h b/src/qemu/qemu_event.h
index 4173834..8552fd1 100644
--- a/src/qemu/qemu_event.h
+++ b/src/qemu/qemu_event.h
@@ -51,6 +51,7 @@ typedef enum {
 QEMU_EVENT_SUSPEND,
 QEMU_EVENT_SUSPEND_DISK,
 QEMU_EVENT_SERIAL_CHANGE,
+QEMU_EVENT_SPICE_MIGRATED,
 QEMU_EVENT_WAKEUP,
 QEMU_EVENT_WATCHDOG,
 
@@ -102,7 +103,12 @@ struct qemuEventTrayChangeData {
 int reason;
 };
 
+struct qemuEventShutdownData {
+virTristateBool guest_initiated;
+};
+
 struct qemuEventGuestPanicData {
+void *info;
 };
 
 struct qemuEventMigrationStatusData {
@@ -159,6 +165,7 @@ struct _qemuEvent {
 struct qemuEventBlockThresholdData ev_threshold;
 struct qemuEventDeviceDeletedData ev_deviceDel;
 struct qemuEventTrayChangeData ev_tray;
+struct qemuEventShutdownData ev_shutdown;
 struct qemuEventGuestPanicData ev_panic;
 struct qemuEventMigrationStatusData ev_migStatus;
 struct qemuEventMigrationPassData ev_migPass;
@@ -219,5 +226,6 @@ int virQemuVmEventListInit(virDomainObjPtr vm);
 int virEnqueueVMEvent(virQemuEventList *qlist, qemuEventPtr ev);
 qemuEventPtr virDequeueVMEvent(virQemuEventList *qlist, virDomainObjPtr vm);
 void virEventWorkerScanQueue(void *dummy, void *opaque);
+void virEventRunHandler(qemuEventPtr ev, void *opaque);
 void virDomainConsumeVMEvents(virDomainObjPtr vm, void *opaque);
 #endif
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 7a26785..4e45cf9 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -34,6 +34,7 @@
 #include "qemu_monitor_json.h"
 #include "qemu_domain.h"
 #include "qemu_process.h"
+#include "qemu_event.h"
 #include "virerror.h"
 #include "viralloc.h"
 #include "virlog.h"
@@ -1316,6 +1317,14 @@ qemuMonitorGetDiskSecret(qemuMonitorPtr mon,
 return ret;
 }
 
+static int
+qemuMonitorEnqueueEvent(qemuMonitorPtr mon, qemuEventPtr ev)
+{
+int ret = -1;
+QEMU_MONITOR_CALLBACK(mon, ret, domainEnqueueEvent, ev->vm, ev);
+
+return ret;
+}
 
 int
 qemuMonitorEmitEvent(qemuMonitorPtr mon, const char *event,
@@ -1332,90 +1341,189 @@ qemuMonitorEmitEvent(qemuMonitorPtr mon, const char 
*event,
 
 
 int
-qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest)
+qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest,
+long long seconds, unsigned int micros)
 {
 int ret = -1;
-VIR_DEBUG("mon=%p guest=%u", mon, guest);
 mon->willhangup = 1;
+qemuEventPtr ev;
+
+if (VIR_ALLOC(ev) < 0){
+return ret;
+}
+
+ev->ev_type = QEMU_EVENT_SHUTDOWN;
+ev->vm = mon->vm;
+ev->seconds = seconds;
+ev->micros = micros;
+ev->evData.ev_shutdown.guest_initiated = guest;
 
-QEMU_MONITOR_CALLBACK(mon, ret, domainShutdown, mon->vm, guest);
+VIR_DEBUG("Vm %s received shutdown event initiated by %u",
+mon->vm->def->name, guest);
+virObjectRef(ev->vm);
+ret = qemuMonitorEnqueueEvent(mon, ev);
 return ret;
 }
 
 
 int
-qemuMonitorEmitReset(qemuMonitorPtr mon)
+qemuMonitorEmitReset(qemuMonitorPtr mon, long long seconds, unsigned int 
micros)
 {
 int ret = -1;
-VIR_DEBUG("mon=%p", mon);
+qemuEventPtr ev;
+
+if (VIR_ALLOC(ev) < 0){
+return ret;
+}
+
+ev->ev_type = QEMU_EVENT_RESET;
+ev->vm = mon->vm;
+ev->seconds = seconds;
+ev->micros = micros;
 
-QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm);
+VIR_DEBUG("Vm %s received reset event", mon->vm->def->name);
+virObjectRef(ev->vm);
+ret = qemuMonitorEnqueueEvent(mon, ev);
 return ret;
 }
 
 
 int
-qemuMonitorEmitPowerdown(qemuMonitorPtr mon)
+qemuMonit

[libvirt] [[RFC] 7/8] Fold back the 2-stage event implementation for a few events : Watchdog, Monitor EOF, Serial changed, Guest panic, Nic RX filter changed .. into single level.

[Prerna Saxena]

Also, the enqueuing of a new event now triggers virEventWorkerScanQueue()

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_driver.c  |  61 ++--
 src/qemu/qemu_process.c | 121 +++-
 2 files changed, 29 insertions(+), 153 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9d495fb..881f253 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -138,8 +138,6 @@ VIR_LOG_INIT("qemu.qemu_driver");
 
 #define QEMU_NB_BANDWIDTH_PARAM 7
 
-static void qemuProcessEventHandler(void *data, void *opaque);
-
 static int qemuStateCleanup(void);
 
 static int qemuDomainObjStart(virConnectPtr conn,
@@ -936,7 +934,9 @@ qemuStateInitialize(bool privileged,
 
 qemuProcessReconnectAll(conn, qemu_driver);
 
-qemu_driver->workerPool = virThreadPoolNew(0, 1, 0, 
qemuProcessEventHandler, qemu_driver);
+qemu_driver->workerPool = virThreadPoolNew(0, 1, 0, 
virEventWorkerScanQueue,
+   
qemu_driver);
+
 if (!qemu_driver->workerPool)
 goto error;
 
@@ -3645,61 +3645,6 @@ qemuDomainScreenshot(virDomainPtr dom,
 }
 
 
-
-
-
-
-
-
-
-
-static void qemuProcessEventHandler(void *data, void *opaque)
-{
-struct qemuProcessEvent *processEvent = data;
-virDomainObjPtr vm = processEvent->vm;
-virQEMUDriverPtr driver = opaque;
-
-VIR_DEBUG("vm=%p, event=%d", vm, processEvent->eventType);
-
-virObjectLock(vm);
-
-switch (processEvent->eventType) {
-case QEMU_PROCESS_EVENT_WATCHDOG:
-processWatchdogEvent(driver, vm, processEvent->action);
-break;
-case QEMU_PROCESS_EVENT_GUESTPANIC:
-processGuestPanicEvent(driver, vm, processEvent->action,
-   processEvent->data);
-break;
-case QEMU_PROCESS_EVENT_DEVICE_DELETED:
-processDeviceDeletedEvent(driver, vm, processEvent->data);
-break;
-case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
-processNicRxFilterChangedEvent(driver, vm, processEvent->data);
-break;
-case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
-processSerialChangedEvent(driver, vm, processEvent->data,
-  processEvent->action);
-break;
-case QEMU_PROCESS_EVENT_BLOCK_JOB:
-processBlockJobEvent(driver, vm,
- processEvent->data,
- processEvent->action,
- processEvent->status);
-break;
-case QEMU_PROCESS_EVENT_MONITOR_EOF:
-processMonitorEOFEvent(driver, vm);
-break;
-case QEMU_PROCESS_EVENT_LAST:
-break;
-}
-
-virDomainConsumeVMEvents(vm, driver);
-virDomainObjEndAPI(&vm);
-VIR_FREE(processEvent);
-}
-
-
 static int
 qemuDomainSetVcpusAgent(virDomainObjPtr vm,
 unsigned int nvcpus)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d2b5fe8..f9270e0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -962,21 +962,11 @@ qemuProcessEventHandleWatchdog1(qemuEventPtr ev,
 }
 
 if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) {
-struct qemuProcessEvent *processEvent;
-if (VIR_ALLOC(processEvent) == 0) {
-processEvent->eventType = QEMU_PROCESS_EVENT_WATCHDOG;
-processEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
-processEvent->vm = vm;
-/* Hold an extra reference because we can't allow 'vm' to be
- * deleted before handling watchdog event is finished.
- */
-virObjectRef(vm);
-if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) 
{
-if (!virObjectUnref(vm))
-vm = NULL;
-VIR_FREE(processEvent);
-}
-}
+/* Hold an extra reference because we can't allow 'vm' to be
+ * deleted before handling watchdog event is finished.*/
+virObjectRef(vm);
+processWatchdogEvent(driver, vm, VIR_DOMAIN_WATCHDOG_ACTION_DUMP);
+virObjectUnref(vm);
 }
 
 qemuDomainEventQueue(driver, watchdogEvent);
@@ -1068,12 +1058,10 @@ qemuProcessEventHandleBlockJob(qemuEventPtr ev,
void *opaque)
 {
 virQEMUDriverPtr driver = opaque;
-struct qemuProcessEvent *processEvent = NULL;
 virDomainDiskDefPtr disk;
 qemuDomainDiskPrivatePtr diskPriv;
-char *data = NULL;
 virDomainObjPtr vm;
-const char *diskAlias;
+char *diskAlias = NULL;
 int type, status;
 
 if (!ev)
@@ -1082,7 +1070,7 @@ qemuProcessEventHandleBlockJob(qemuEventPtr ev,
 
 if (!ev->vm) {
 VIR_WARN("Unable to locate VM, dropping Block Job event");
-goto cleanup;
+goto error;
 }
 
 diskAlias = ev->evData.ev_blockJob.device;
@@ -1103,31 +1091,16 @@ qemuProcessEventHandleBlockJob(qemuEventPtr ev

[libvirt] [[RFC] 5/8] Events: Plumb event handling calls before a domain's APIs complete.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_driver.c | 131 +++--
 1 file changed, 128 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8a005d0..b249347 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1900,6 +1900,7 @@ static int qemuDomainSuspend(virDomainPtr dom)
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 
 qemuDomainEventQueue(driver, event);
@@ -1967,6 +1968,7 @@ static int qemuDomainResume(virDomainPtr dom)
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 qemuDomainEventQueue(driver, event);
 virObjectUnref(cfg);
@@ -2057,6 +2059,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, 
unsigned int flags)
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2159,6 +2162,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2206,6 +2210,7 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2294,6 +2299,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 qemuDomainEventQueue(driver, event);
 return ret;
@@ -2307,6 +2313,7 @@ qemuDomainDestroy(virDomainPtr dom)
 
 static char *qemuDomainGetOSType(virDomainPtr dom) {
 virDomainObjPtr vm;
+virQEMUDriverPtr driver = dom->conn->privateData;
 char *type = NULL;
 
 if (!(vm = qemuDomObjFromDomain(dom)))
@@ -2318,6 +2325,7 @@ static char *qemuDomainGetOSType(virDomainPtr dom) {
 ignore_value(VIR_STRDUP(type, virDomainOSTypeToString(vm->def->os.type)));
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return type;
 }
@@ -2328,6 +2336,7 @@ qemuDomainGetMaxMemory(virDomainPtr dom)
 {
 virDomainObjPtr vm;
 unsigned long long ret = 0;
+virQEMUDriverPtr driver = dom->conn->privateData;
 
 if (!(vm = qemuDomObjFromDomain(dom)))
 goto cleanup;
@@ -2338,6 +2347,7 @@ qemuDomainGetMaxMemory(virDomainPtr dom)
 ret = virDomainDefGetMemoryTotal(vm->def);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2455,6 +2465,7 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, 
unsigned long newmem,
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 virObjectUnref(cfg);
 return ret;
@@ -2543,6 +2554,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr 
dom, int period,
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 virObjectUnref(cfg);
 return ret;
@@ -2583,6 +2595,7 @@ static int qemuDomainInjectNMI(virDomainPtr domain, 
unsigned int flags)
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2646,6 +2659,7 @@ static int qemuDomainSendKey(virDomainPtr domain,
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2702,6 +2716,7 @@ qemuDomainGetInfo(virDomainPtr dom,
 ret = 0;
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -2739,6 +2754,7 @@ qemuDomainGetControlInfo(virDomainPtr dom,
 virDomainObjPtr vm;
 qemuDomainObjPrivatePtr priv;
 int ret = -1;
+virQEMUDriverPtr driver = dom->conn->privateData;
 
 virCheckFlags(0, -1);
 
@@ -2788,6 +2804,7 @@ qemuDomainGetControlInfo(virDomainPtr dom,
 ret = 0;
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 return ret;
 }
@@ -3577,6 +3594,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, 
const char *dxml,
  compressedpath, dxml, flags);
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 VIR_FREE(compressedpath);
 virObjectUnref(cfg);
@@ -3653,6 +3671,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int 
flags)
 vm->hasManagedSave = true;
 
  cleanup:
+virDomainConsumeVMEvents(vm, driver);
 virDomainObjEndAPI(&vm);
 VIR_FREE(name);
 VIR_FREE(compressedpath);
@@ -3689,7 +3708,7 @@ qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned 
int flags)
 {
 virDomainObjPtr vm = NULL;
 i

[libvirt] [[RFC] 8/8] Initialize the per-VM event queues in context of domain init.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_driver.c | 20 
 src/qemu/qemu_event.c  |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 881f253..e4b6d06 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1781,6 +1781,11 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
 goto cleanup;
+
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
 virObjectRef(vm);
 def = NULL;
 
@@ -5531,6 +5536,11 @@ qemuDomainRestoreFlags(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
 goto cleanup;
+
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
 virObjectRef(vm);
 def = NULL;
 
@@ -6208,6 +6218,11 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
0, &oldDef)))
 goto cleanup;
 
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
+
 virObjectRef(vm);
 def = NULL;
 if (qemuDomainHasBlockjob(vm, true)) {
@@ -15073,6 +15088,11 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr 
conn,
NULL)))
 goto cleanup;
 
+if (virQemuVmEventListInit(vm) < 0) {
+virDomainObjListRemove(driver->domains, vm);
+goto cleanup;
+}
+
 virObjectRef(vm);
 def = NULL;
 
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
index beb309f..00a06ee 100644
--- a/src/qemu/qemu_event.c
+++ b/src/qemu/qemu_event.c
@@ -145,6 +145,8 @@ int virEnqueueVMEvent(virQemuEventList *qlist, qemuEventPtr 
ev)
 
 /* Insert into per-Vm queue */
 vmq = ev->vm->vmq;
+if (!vmq)
+goto error;
 
 virMutexLock(&(vmq->lock));
 if (vmq->last) {
-- 
2.9.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [[RFC] 6/8] Code refactor: Move helper functions of doCoreDump*, syncNicRxFilter*, and qemuOpenFile* to qemu_process.[ch]

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_driver.c  | 1161 ---
 src/qemu/qemu_process.c | 1133 +
 src/qemu/qemu_process.h |   86 
 3 files changed, 1219 insertions(+), 1161 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b249347..9d495fb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -151,11 +151,6 @@ static int qemuDomainObjStart(virConnectPtr conn,
 static int qemuDomainManagedSaveLoad(virDomainObjPtr vm,
  void *opaque);
 
-static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
-  bool dynamicOwnership,
-  const char *path, int oflags,
-  bool *needUnlink, bool *bypassSecurityDriver);
-
 static int qemuGetDHCPInterfaces(virDomainPtr dom,
  virDomainObjPtr vm,
  virDomainInterfacePtr **ifaces);
@@ -2819,38 +2814,6 @@ qemuDomainGetControlInfo(virDomainPtr dom,
 
 verify(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL));
 
-typedef enum {
-QEMU_SAVE_FORMAT_RAW = 0,
-QEMU_SAVE_FORMAT_GZIP = 1,
-QEMU_SAVE_FORMAT_BZIP2 = 2,
-/*
- * Deprecated by xz and never used as part of a release
- * QEMU_SAVE_FORMAT_LZMA
- */
-QEMU_SAVE_FORMAT_XZ = 3,
-QEMU_SAVE_FORMAT_LZOP = 4,
-/* Note: add new members only at the end.
-   These values are used in the on-disk format.
-   Do not change or re-use numbers. */
-
-QEMU_SAVE_FORMAT_LAST
-} virQEMUSaveFormat;
-
-VIR_ENUM_DECL(qemuSaveCompression)
-VIR_ENUM_IMPL(qemuSaveCompression, QEMU_SAVE_FORMAT_LAST,
-  "raw",
-  "gzip",
-  "bzip2",
-  "xz",
-  "lzop")
-
-VIR_ENUM_DECL(qemuDumpFormat)
-VIR_ENUM_IMPL(qemuDumpFormat, VIR_DOMAIN_CORE_DUMP_FORMAT_LAST,
-  "elf",
-  "kdump-zlib",
-  "kdump-lzo",
-  "kdump-snappy")
-
 typedef struct _virQEMUSaveHeader virQEMUSaveHeader;
 typedef virQEMUSaveHeader *virQEMUSaveHeaderPtr;
 struct _virQEMUSaveHeader {
@@ -3062,214 +3025,6 @@ qemuCompressGetCommand(virQEMUSaveFormat compression)
 return ret;
 }
 
-/**
- * qemuOpenFile:
- * @driver: driver object
- * @vm: domain object
- * @path: path to file to open
- * @oflags: flags for opening/creation of the file
- * @needUnlink: set to true if file was created by this function
- * @bypassSecurityDriver: optional pointer to a boolean that will be set to 
true
- *if security driver operations are pointless (due to
- *NFS mount)
- *
- * Internal function to properly create or open existing files, with
- * ownership affected by qemu driver setup and domain DAC label.
- *
- * Returns the file descriptor on success and negative errno on failure.
- *
- * This function should not be used on storage sources. Use
- * qemuDomainStorageFileInit and storage driver APIs if possible.
- **/
-static int
-qemuOpenFile(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *path,
- int oflags,
- bool *needUnlink,
- bool *bypassSecurityDriver)
-{
-int ret = -1;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-uid_t user = cfg->user;
-gid_t group = cfg->group;
-bool dynamicOwnership = cfg->dynamicOwnership;
-virSecurityLabelDefPtr seclabel;
-
-virObjectUnref(cfg);
-
-/* TODO: Take imagelabel into account? */
-if (vm &&
-(seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL &&
-seclabel->label != NULL &&
-(virParseOwnershipIds(seclabel->label, &user, &group) < 0))
-goto cleanup;
-
-ret = qemuOpenFileAs(user, group, dynamicOwnership,
- path, oflags, needUnlink, bypassSecurityDriver);
-
- cleanup:
-return ret;
-}
-
-static int
-qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
-   bool dynamicOwnership,
-   const char *path, int oflags,
-   bool *needUnlink, bool *bypassSecurityDriver)
-{
-struct stat sb;
-bool is_reg = true;
-bool need_unlink = false;
-bool bypass_security = false;
-unsigned int vfoflags = 0;
-int fd = -1;
-int path_shared = virFileIsSharedFS(path);
-uid_t uid = geteuid();
-gid_t gid = getegid();
-
-/* path might be a pre-existing block dev, in which case
- * we need to skip the create step, and also avoid unlink
- * in the failure case */
-if (oflags & O_CREAT) {
-need_unlink = true;
-
-/* Don't force chown on network-shared FS
- * as it is likely to fail. */
-if (path_shared <= 0 || dynamicOwnership)
-vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
-
-if (stat(path, &sb) == 0) {
-/* It already exists, we don't want to del

[libvirt] [[RFC] 1/8] Introduce virObjectTrylock()

[Prerna Saxena]

This is a wrapper function that:
(1) Attempts to take a lock on the object.
(2) gracefully returns if the object is already locked.

Signed-off-by: Prerna Saxena 
---
 src/libvirt_private.syms |  1 +
 src/util/virobject.c | 26 ++
 src/util/virobject.h |  4 
 src/util/virthread.c |  5 +
 src/util/virthread.h |  1 +
 5 files changed, 37 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9243c55..c0ab8b5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2362,6 +2362,7 @@ virObjectRWLockableNew;
 virObjectRWLockRead;
 virObjectRWLockWrite;
 virObjectRWUnlock;
+virObjectTrylock;
 virObjectUnlock;
 virObjectUnref;
 
diff --git a/src/util/virobject.c b/src/util/virobject.c
index cfa821c..796ea06 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -495,6 +495,32 @@ virObjectRWLockWrite(void *anyobj)
 
 
 /**
+ * virObjectTrylock:
+ * @anyobj: any instance of virObjectLockable or virObjectRWLockable
+ *
+ * Attempt to acquire a lock on @anyobj. The lock must be released by
+ * virObjectUnlock.
+ * Returns:
+ *0: If the lock was successfully taken.
+ *errno : Indicates error.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ */
+int
+virObjectTrylock(void *anyobj)
+{
+virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+if (!obj)
+return -1;
+
+return virMutexTrylock(&obj->lock);
+}
+
+/**
  * virObjectUnlock:
  * @anyobj: any instance of virObjectLockable
  *
diff --git a/src/util/virobject.h b/src/util/virobject.h
index ac6cf22..402ea32 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -124,6 +124,10 @@ void
 virObjectLock(void *lockableobj)
 ATTRIBUTE_NONNULL(1);
 
+int
+virObjectTrylock(void *lockableobj)
+ATTRIBUTE_NONNULL(1);
+
 void
 virObjectRWLockRead(void *lockableobj)
 ATTRIBUTE_NONNULL(1);
diff --git a/src/util/virthread.c b/src/util/virthread.c
index 6c49515..07b7a3f 100644
--- a/src/util/virthread.c
+++ b/src/util/virthread.c
@@ -89,6 +89,11 @@ void virMutexLock(virMutexPtr m)
 pthread_mutex_lock(&m->lock);
 }
 
+int  virMutexTrylock(virMutexPtr m)
+{
+return pthread_mutex_trylock(&m->lock);
+}
+
 void virMutexUnlock(virMutexPtr m)
 {
 pthread_mutex_unlock(&m->lock);
diff --git a/src/util/virthread.h b/src/util/virthread.h
index e466d9b..8e3da2c 100644
--- a/src/util/virthread.h
+++ b/src/util/virthread.h
@@ -132,6 +132,7 @@ int virMutexInitRecursive(virMutexPtr m) 
ATTRIBUTE_RETURN_CHECK;
 void virMutexDestroy(virMutexPtr m);
 
 void virMutexLock(virMutexPtr m);
+int virMutexTrylock(virMutexPtr m);
 void virMutexUnlock(virMutexPtr m);
 
 
-- 
2.9.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [[RFC] 3/8] Setup global and per-VM event queues. Also initialize per-VM queues when libvirt reconnects to an existing VM.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/conf/domain_conf.h  |   3 +
 src/qemu/qemu_conf.h|   4 +
 src/qemu/qemu_driver.c  |   9 ++
 src/qemu/qemu_event.c   | 229 
 src/qemu/qemu_event.h   |   1 -
 src/qemu/qemu_process.c |   2 +
 6 files changed, 247 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a42efcf..7fe38e7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2496,6 +2496,9 @@ struct _virDomainObj {
 
 unsigned long long original_memlock; /* Original RLIMIT_MEMLOCK, zero if no
   * restore will be required later */
+
+/* Pointer to per-VM Event Queue */
+void *vmq;
 };
 
 typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn,
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 13b6f81..e63dc98 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -33,6 +33,7 @@
 # include "domain_conf.h"
 # include "snapshot_conf.h"
 # include "domain_event.h"
+# include "qemu_event.h"
 # include "virthread.h"
 # include "security/security_manager.h"
 # include "virpci.h"
@@ -235,6 +236,9 @@ struct _virQEMUDriver {
 /* Immutable pointer, self-locking APIs */
 virDomainObjListPtr domains;
 
+/* Immutable pointer, contains Qemu Driver Event List */
+virQemuEventList *ev_list;
+
 /* Immutable pointer */
 char *qemuImgBinary;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7c6f167..8a005d0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -52,6 +52,7 @@
 #include "qemu_command.h"
 #include "qemu_parse_command.h"
 #include "qemu_cgroup.h"
+#include "qemu_event.h"
 #include "qemu_hostdev.h"
 #include "qemu_hotplug.h"
 #include "qemu_monitor.h"
@@ -650,6 +651,14 @@ qemuStateInitialize(bool privileged,
 if (!(qemu_driver->domains = virDomainObjListNew()))
 goto error;
 
+/* Init domain Async QMP events */
+qemu_driver->ev_list = virQemuEventListInit();
+if (!qemu_driver->ev_list) {
+virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to initialize QMP event queues"));
+goto error;
+}
+
 /* Init domain events */
 qemu_driver->domainEventState = virObjectEventStateNew();
 if (!qemu_driver->domainEventState)
diff --git a/src/qemu/qemu_event.c b/src/qemu/qemu_event.c
index e27ea0d..d52fad2 100644
--- a/src/qemu/qemu_event.c
+++ b/src/qemu/qemu_event.c
@@ -73,3 +73,232 @@ virQemuEventList* virQemuEventListInit(void)
 
 return ev_list;
 }
+
+int virQemuVmEventListInit(virDomainObjPtr vm)
+{
+virQemuVmEventQueue* vmq;
+if (!vm)
+return -1;
+
+if (VIR_ALLOC(vmq) < 0)
+return -1;
+
+vmq->last = NULL;
+vmq->head = NULL;
+
+if (!virMutexInit(&vmq->lock)) {
+vm->vmq = vmq;
+return 0;
+}
+return -1;
+}
+/**
+ * virEnqueueVMEvent()
+ * Adds a new event to:
+ * - Global event queue
+ * - the event queue for this VM
+ *
+ * Return : 0 (success)
+ * -1 (failure)
+ */
+int virEnqueueVMEvent(virQemuEventList *qlist, qemuEventPtr ev)
+{
+struct _qemuGlobalEventListElement *globalEntry;
+virQemuVmEventQueue *vmq;
+struct _qemuVmEventQueueElement *vmq_entry;
+
+if (!qlist || !ev || !ev->vm || !ev->vm->vmq) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "No queue list instantiated."
+   "Dropping event %d for Vm %s",
+   ev->ev_type, ev->vm->def->name);
+goto error;
+}
+
+if (VIR_ALLOC(globalEntry) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "Allocation error."
+   "Dropping event %d for Vm %s",
+   ev->ev_type, ev->vm->def->name);
+goto error;
+}
+
+if (VIR_ALLOC(vmq_entry) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "Allocation error."
+   "Dropping event %d for Vm %s",
+   ev->ev_type, ev->vm->def->name);
+free(globalEntry);
+goto error;
+}
+
+vmq_entry->ev = ev;
+vmq_entry->next = NULL;
+
+virObjectRef(ev->vm);
+globalEntry->vm = ev->vm;
+globalEntry->next = NULL;
+globalEntry->prev = NULL;
+/* Note that this order needs to be maintained
+ * for dequeue too else ABBA deadlocks will happen */
+
+/* Insert into per-Vm queue */
+vmq = ev->vm->vmq;
+
+virMutexLock(&(vmq->lock));
+if (vmq->last) {
+vmq->last->next = vmq_entry;
+vmq_entry->ev->ev_id = vmq->last->ev->ev_id + 1;
+} else {
+vmq->head = vmq_entry;
+vmq_entry->ev->ev_id = 1;
+}
+vmq->last = vmq_entry;
+globalEntry->ev_id = vmq_entry->ev->ev_id;
+/* Insert the event into the global queue */
+virMutexLock(&(qlist->lock));
+if (qlist->last) {
+qlist->last->next = globalEntry

[libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.

[Prerna Saxena]

As noted in
https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
libvirt-QEMU driver handles all async events from the main loop.
Each event handling needs the per-VM lock to make forward progress. In
the case where an async event is received for the same VM which has an
RPC running, the main loop is held up contending for the same lock.

This impacts scalability, and should be addressed on priority.

Note that libvirt does have a 2-step deferred handling for a few event
categories, but (1) That is insufficient since blockign happens before
the handler could disambiguate which one needs to be posted to this
other queue.
(2) There needs to be homogeniety.

The current series builds a framework for recording and handling VM
events.
It initializes per-VM event queue, and a global event queue pointing to
events from all the VMs. Event handling is staggered in 2 stages:
- When an event is received, it is enqueued in the per-VM queue as well
  as the global queues.
- The global queue is built into the QEMU Driver as a threadpool
  (currently with a single thread).
- Enqueuing of a new event triggers the global event worker thread, which
  then attempts to take a lock for this event's VM.
- If the lock is available, the event worker runs the function handling
  this event type. Once done, it dequeues this event from the global
  as well as per-VM queues.
- If the lock is unavailable(ie taken by RPC thread), the event worker 
  thread leaves this as-is and picks up the next event.
- Once the RPC thread completes, it looks for events pertaining to the
  VM in the per-VM event queue. It then processes the events serially
  (holding the VM lock) until there are no more events remaining for
  this VM. At this point, the per-VM lock is relinquished.

Patch Series status:
Strictly RFC only. No compilation issues. I have not had a chance to
(stress) test it after rebase to latest master.
Note that documentation and test coverage is TBD, since a few open
points remain.

Known issues/ caveats:
- RPC handling time will become non-deterministic.
- An event will only be "notified" to a client once the RPC for same VM 
completes.
- Needs careful consideration in all cases where a QMP event is used to
  "signal" an RPC thread, else will deadlock.

Will be happy to drive more discussion in the community and completely
implement it.

Prerna Saxena (8):
  Introduce virObjectTrylock()
  QEMU Event handling: Introduce async event helpers in qemu_event.[ch]
  Setup global and per-VM event queues. Also initialize per-VM queues
when libvirt reconnects to an existing VM.
  Events: Allow monitor to "enqueue" events to a queue. Also introduce a
framework of handlers for each event type, that can be called when
the handler is running an event.
  Events: Plumb event handling calls before a domain's APIs complete.
  Code refactor: Move helper functions of doCoreDump*, syncNicRxFilter*,
and qemuOpenFile* to qemu_process.[ch]
  Fold back the 2-stage event implementation for a few events :
Watchdog, Monitor EOF, Serial changed, Guest panic, Nic RX filter
changed .. into single level.
  Initialize the per-VM event queues in context of domain init.

 src/Makefile.am  |1 +
 src/conf/domain_conf.h   |3 +
 src/libvirt_private.syms |1 +
 src/qemu/qemu_conf.h |4 +
 src/qemu/qemu_driver.c   | 1710 +++
 src/qemu/qemu_event.c|  317 +++
 src/qemu/qemu_event.h|  231 +
 src/qemu/qemu_monitor.c  |  592 ++--
 src/qemu/qemu_monitor.h  |   80 +-
 src/qemu/qemu_monitor_json.c |  291 +++---
 src/qemu/qemu_process.c  | 2031 ++
 src/qemu/qemu_process.h  |   88 ++
 src/util/virobject.c |   26 +
 src/util/virobject.h |4 +
 src/util/virthread.c |5 +
 src/util/virthread.h |1 +
 tests/qemumonitortestutils.c |2 +-
 17 files changed, 3411 insertions(+), 1976 deletions(-)
 create mode 100644 src/qemu/qemu_event.c
 create mode 100644 src/qemu/qemu_event.h

-- 
2.9.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virt-aa-helper: grant locking permission on -f

[Christian Ehrhardt]

Hot-adding disks does not parse the full XML to generate apparmor rules.
Instead it uses -f  to append a generic rule for that file path.

580cdaa7: "virt-aa-helper: locking disk files for qemu 2.10" implemented
the qemu 2.10 requirement to allow locking on disks images that are part of
the domain xml.

But on attach-device a user will still trigger an apparmor deny by going
through virt-aa-helper -f, to fix that add the lock "k" permission to the
append file case of virt-aa-helper.

Signed-off-by: Christian Ehrhardt 
---
 src/security/virt-aa-helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index ef1bf01..ee3913d 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1157,7 +1157,7 @@ get_files(vahControl * ctl)
 }
 
 if (ctl->newfile)
-if (vah_add_file(&buf, ctl->newfile, "rw") != 0)
+if (vah_add_file(&buf, ctl->newfile, "rwk") != 0)
 goto cleanup;
 
 if (virBufferError(&buf)) {
@@ -1341,7 +1341,7 @@ main(int argc, char **argv)
 vah_error(ctl, 1, _("profile exists"));
 
 if (ctl->append && ctl->newfile) {
-if (vah_add_file(&buf, ctl->newfile, "rw") != 0)
+if (vah_add_file(&buf, ctl->newfile, "rwk") != 0)
 goto cleanup;
 } else {
 if (ctl->def->virtType == VIR_DOMAIN_VIRT_QEMU ||
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/4] qemu: Use predictable file names for memory-backend-file

[Michal Privoznik]

In some cases management application needs to allocate memory for
qemu upfront and then just let qemu use that. Since we don't want
to expose path for memory-backend-file anywhere in the domain
XML, we can generate predictable paths. In this case:

  $memoryBackingDir/libvirt/qemu/$shortName/$alias

where $shortName is result of virDomainObjGetShortName().

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c|   9 +-
 src/qemu/qemu_conf.c   |  55 -
 src/qemu/qemu_conf.h   |   9 +-
 src/qemu/qemu_driver.c |  17 +++
 src/qemu/qemu_hotplug.c|   2 +-
 src/qemu/qemu_process.c| 137 +++--
 src/qemu/qemu_process.h|   8 +-
 .../qemuxml2argv-cpu-numa-memshared.args   |   6 +-
 .../qemuxml2argv-fd-memory-numa-topology.args  |   3 +-
 .../qemuxml2argv-fd-memory-numa-topology2.args |   6 +-
 .../qemuxml2argv-fd-memory-numa-topology3.args |   9 +-
 .../qemuxml2argv-hugepages-memaccess2.args |   9 +-
 12 files changed, 207 insertions(+), 63 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82d2fb2a3..d6d2654cd 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
 } else {
 /* We can have both pagesize and mem source. If that's the case,
  * prefer hugepages as those are more specific. */
-if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
+if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, &memPath) 
< 0)
 goto cleanup;
 }
 
@@ -3542,12 +3542,13 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
 unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
 cell);
 
+if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
+goto cleanup;
+
 *backendStr = NULL;
 mem.size = memsize;
 mem.targetNode = cell;
-
-if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
-goto cleanup;
+mem.info.alias = alias;
 
 if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, 
priv->qemuCaps,
 def, &mem, priv->autoNodeset, false)) 
< 0)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 23dcc25e1..22c023255 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
 }
 
 
+int
+qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir);
+}
+
+
+int
+qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+   virQEMUDriverConfigPtr cfg,
+   char **path)
+{
+char *shortName = NULL;
+char *base = NULL;
+int ret = -1;
+
+if (!(shortName = virDomainDefGetShortName(def)) ||
+qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
+virAsprintf(path, "%s/%s", base, shortName) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(base);
+VIR_FREE(shortName);
+return ret;
+}
+
+
 /**
  * qemuGetMemoryBackingPath:
+ * @def: domain definition
  * @cfg: the driver config
+ * @alias: memory object alias
  * @memPath: constructed path
  *
  * Constructs path to memory backing dir and stores it at @memPath.
@@ -1761,8 +1793,27 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
  *  -1 otherwise (with error reported).
  */
 int
-qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
  char **memPath)
 {
-return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+char *domainPath = NULL;
+int ret = -1;
+
+if (!alias) {
+/* This should never happen (TM) */
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("memory device alias is not assigned"));
+goto cleanup;
+}
+
+if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
+virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(domainPath);
+return ret;
 }
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 9d6866816..a553e30e2 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -364,6 +364,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
unsigned long long pagesize,
char **memPath);
 
-int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+int qemuGetMemoryBackingBa

[libvirt] [PATCH v2 2/4] qemu: Move memPath generation from memoryBackingDir to a separate function

[Michal Privoznik]

In near future we will need more than just a plain VIR_STRDUP().
Better implement that in a separate function and in
qemuBuildMemoryBackendStr() which is complicated enough already.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c |  2 +-
 src/qemu/qemu_conf.c| 18 ++
 src/qemu/qemu_conf.h|  3 +++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b1cfafa79..82d2fb2a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3439,7 +3439,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
 } else {
 /* We can have both pagesize and mem source. If that's the case,
  * prefer hugepages as those are more specific. */
-if (VIR_STRDUP(memPath, cfg->memoryBackingDir) < 0)
+if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
 goto cleanup;
 }
 
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index bf6b334f4..23dcc25e1 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1748,3 +1748,21 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
 
 return 0;
 }
+
+
+/**
+ * qemuGetMemoryBackingPath:
+ * @cfg: the driver config
+ * @memPath: constructed path
+ *
+ * Constructs path to memory backing dir and stores it at @memPath.
+ *
+ * Returns: 0 on success,
+ *  -1 otherwise (with error reported).
+ */
+int
+qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+ char **memPath)
+{
+return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 13b6f818a..9d6866816 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -363,4 +363,7 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
virQEMUDriverConfigPtr cfg,
unsigned long long pagesize,
char **memPath);
+
+int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+ char **memPath);
 #endif /* __QEMUD_CONF_H */
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/4] conf: s/virDomainObjGetShortName/virDomainDefGetShortName/

[Michal Privoznik]

This function works over domain definition and not domain object.
Its name is thus misleading.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 4 ++--
 src/conf/domain_conf.h   | 2 +-
 src/libvirt_private.syms | 2 +-
 src/qemu/qemu_conf.c | 2 +-
 src/qemu/qemu_domain.c   | 4 ++--
 src/qemu/qemu_driver.c   | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 77c20c697..58b75c672 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27871,14 +27871,14 @@ virDomainDefHasMemballoon(const virDomainDef *def)
 #define VIR_DOMAIN_SHORT_NAME_MAX 20
 
 /**
- * virDomainObjGetShortName:
+ * virDomainDefGetShortName:
  * @vm: Machine for which to get a name
  * @unique: Make sure the name is unique (use id as well)
  *
  * Shorten domain name to avoid possible path length limitations.
  */
 char *
-virDomainObjGetShortName(const virDomainDef *def)
+virDomainDefGetShortName(const virDomainDef *def)
 {
 wchar_t wshortname[VIR_DOMAIN_SHORT_NAME_MAX + 1] = {0};
 size_t len = 0;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 38de70b15..bd541a9c5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3366,7 +3366,7 @@ int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
 
 bool virDomainDefHasMemballoon(const virDomainDef *def) ATTRIBUTE_NONNULL(1);
 
-char *virDomainObjGetShortName(const virDomainDef *def) ATTRIBUTE_NONNULL(1);
+char *virDomainDefGetShortName(const virDomainDef *def) ATTRIBUTE_NONNULL(1);
 
 int
 virDomainGetBlkioParametersAssignFromDef(virDomainDefPtr def,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 448d962b2..3ebff18aa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -262,6 +262,7 @@ virDomainDefGetMemoryInitial;
 virDomainDefGetMemoryTotal;
 virDomainDefGetOnlineVcpumap;
 virDomainDefGetSecurityLabelDef;
+virDomainDefGetShortName;
 virDomainDefGetVcpu;
 virDomainDefGetVcpuPinInfoHelper;
 virDomainDefGetVcpus;
@@ -457,7 +458,6 @@ virDomainObjGetMetadata;
 virDomainObjGetOneDef;
 virDomainObjGetOneDefState;
 virDomainObjGetPersistentDef;
-virDomainObjGetShortName;
 virDomainObjGetState;
 virDomainObjNew;
 virDomainObjParseNode;
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index ec61c9c52..bf6b334f4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1659,7 +1659,7 @@ qemuGetDomainHugepagePath(const virDomainDef *def,
   virHugeTLBFSPtr hugepage)
 {
 char *base = qemuGetBaseHugepagePath(hugepage);
-char *domPath = virDomainObjGetShortName(def);
+char *domPath = virDomainDefGetShortName(def);
 char *ret = NULL;
 
 if (base && domPath)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c7c9e94da..e828fa2f6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1705,7 +1705,7 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
 {
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 qemuDomainObjPrivatePtr priv = vm->privateData;
-char *domname = virDomainObjGetShortName(vm->def);
+char *domname = virDomainDefGetShortName(vm->def);
 int ret = -1;
 
 if (!domname)
@@ -8203,7 +8203,7 @@ qemuDomainGetPreservedMountPath(virQEMUDriverConfigPtr 
cfg,
 char *path = NULL;
 char *tmp;
 const char *suffix = mountpoint + strlen(DEVPREFIX);
-char *domname = virDomainObjGetShortName(vm->def);
+char *domname = virDomainDefGetShortName(vm->def);
 size_t off;
 
 if (!domname)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 74fdfdb0f..135c20749 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4079,7 +4079,7 @@ getAutoDumpPath(virQEMUDriverPtr driver,
 virDomainObjPtr vm)
 {
 char *dumpfile = NULL;
-char *domname = virDomainObjGetShortName(vm->def);
+char *domname = virDomainDefGetShortName(vm->def);
 char timestr[100];
 struct tm time_info;
 time_t curtime = time(NULL);
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 4/4] news: Document predictable file names for memory-backend-file

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 docs/news.xml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 989b82aa5..29eaacb9a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -51,6 +51,17 @@
   
 
 
+  
+
+  qemu: Generate predictable paths for qemu memory backends
+
+
+  In some cases management applications need to know
+  paths passed to memory-backend-file objects upfront.
+  Libvirt now generates predictable paths so applications
+  can prepare the files if they need to do so.
+
+  
 
 
   
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/4] Predictable file names for memory-backend-file

[Michal Privoznik]

v2 of:

https://www.redhat.com/archives/libvir-list/2017-October/msg01063.html

Patches are to be found here too:

https://github.com/zippy2/libvirt/tree/qemu_mem_path_v3

diff to v1:
-Dropped qemu.conf config knob
-s/rmdir/virFileDeleteTree/ in qemuProcessBuildDestroyMemoryPathsImpl() because
 qemu leaves files behind and thus we need to unlink them too.

Michal Privoznik (4):
  conf: s/virDomainObjGetShortName/virDomainDefGetShortName/
  qemu: Move memPath generation from memoryBackingDir to a separate
function
  qemu: Use predictable file names for memory-backend-file
  news: Document predictable file names for memory-backend-file

 docs/news.xml  |  11 ++
 src/conf/domain_conf.c |   4 +-
 src/conf/domain_conf.h |   2 +-
 src/libvirt_private.syms   |   2 +-
 src/qemu/qemu_command.c|   9 +-
 src/qemu/qemu_conf.c   |  71 ++-
 src/qemu/qemu_conf.h   |  10 ++
 src/qemu/qemu_domain.c |   4 +-
 src/qemu/qemu_driver.c |  19 ++-
 src/qemu/qemu_hotplug.c|   2 +-
 src/qemu/qemu_process.c| 137 +++--
 src/qemu/qemu_process.h|   8 +-
 .../qemuxml2argv-cpu-numa-memshared.args   |   6 +-
 .../qemuxml2argv-fd-memory-numa-topology.args  |   3 +-
 .../qemuxml2argv-fd-memory-numa-topology2.args |   6 +-
 .../qemuxml2argv-fd-memory-numa-topology3.args |   9 +-
 .../qemuxml2argv-hugepages-memaccess2.args |   9 +-
 17 files changed, 244 insertions(+), 68 deletions(-)

-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 2/2] virtlogd: add missing netserver refcount increment on reload

[Nikolay Shirokovskiy]

After virNetDaemonAddServerPostExec call in virtlogd we should have
netserver refcount set to 2. One goes to netdaemon servers hashtable
and one goes to virtlogd own reference to netserver. Let's add
missing increment in virNetDaemonAddServerPostExec itself while holding
daemon lock.

We also have to unref new extra ref after virtlockd call to 
virNetDaemonAddServerPostExec.
---
 src/locking/lock_daemon.c | 1 +
 src/rpc/virnetdaemon.c| 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index fe3eaf9..41a06b2 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, 
bool privileged)
   virLockDaemonClientFree,
   (void*)(intptr_t)(privileged ? 
0x1 : 0x0
 goto error;
+virObjectUnref(srv);
 
 return lockd;
 
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 495bc4b..f3e3ffe 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -312,6 +312,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
 
 if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
 goto error;
+virObjectRef(srv);
 
 virJSONValueFree(object);
 virObjectUnlock(dmn);
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination

[Nikolay Shirokovskiy]

The problem is incorrect order of qemu driver shutdown and shutdown
of netserver threads that serve client requests (thru qemu driver
particularly).

Net server threads are shutdowned upon dispose which is triggered
by last daemon object unref at the end of main function. At the same
time qemu driver is shutdowned earlier in virStateCleanup. As a result
netserver threads see invalid driver object in the middle of request
processing.

Let's move shutting down netserver threads earlier to virNetDaemonClose.

Note: order of last daemon unref and virStateCleanup
is introduced in 85c3a182 for a valid reason.
---

One can use next patch to trigger crash on termination. Call domstats function 
and then send TERM to libvirtd.

Patch to trigger crash
#   diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
#   index cf5e4ad..39a57aa 100644
#   --- a/src/qemu/qemu_driver.c
#   +++ b/src/qemu/qemu_driver.c
#   @@ -20144,6 +20144,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
#domflags = 0;
#vm = vms[i];
#
#   +sleep(5);
#   +
#virObjectLock(vm);
#
#if (HAVE_JOB(privflags) &&

 src/rpc/virnetdaemon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index e3b9390..495bc4b 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -880,6 +880,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
 virObjectLock(dmn);
 
 virHashForEach(dmn->servers, daemonServerClose, NULL);
+virHashFree(dmn->servers);
+dmn->servers = NULL;
 
 virObjectUnlock(dmn);
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 0/2] daemon: fix termination/reload issues

[Nikolay Shirokovskiy]

v1: https://www.redhat.com/archives/libvir-list/2017-September/msg01006.html

   Changes from v2:

Fix syntax check issues.

   Changes from v1:

1. Patches that fixes qemu driver termination are moved out of this series
2. New version of [1] now fixes issue in a way that suggested by reviewer
3. [2] fixes another issue that was discovered in the process of review

Nikolay Shirokovskiy (2):
  libvirtd: fix crash on termination [1]
  virtlogd: add missing netserver refcount increment on reload   [2]

 src/locking/lock_daemon.c | 1 +
 src/rpc/virnetdaemon.c| 3 +++
 2 files changed, 4 insertions(+)

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/2] daemon: fix termination/reload issues

[Nikolay Shirokovskiy]

Please, ignore. Need to resend after syntax check issues fixed.

On 24.10.2017 12:52, Nikolay Shirokovskiy wrote:
> v1: https://www.redhat.com/archives/libvir-list/2017-September/msg01006.html
> 
> Changes from v1:
> 
> 1. Patches that fixes qemu driver termination are moved out of this series
> 2. New version of [1] now fixes issue in a way that suggested by reviewer
> 3. [2] fixes another issue that was discovered in the process of review
> 
> Nikolay Shirokovskiy (2):
>   libvirtd: fix crash on termination [1]
>   virtlogd: add missing netserver refcount increment on reload   [2]
> 
>  src/locking/lock_daemon.c | 1 +
>  src/rpc/virnetdaemon.c| 6 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/2] libvirtd: fix crash on termination

[Nikolay Shirokovskiy]

The problem is incorrect order of qemu driver shutdown and shutdown
of netserver threads that serve client requests (thru qemu driver
particularly).

Net server threads are shutdowned upon dispose which is triggered
by last daemon object unref at the end of main function. At the same
time qemu driver is shutdowned earlier in virStateCleanup. As a result
netserver threads see invalid driver object in the middle of request
processing.

Let's move shutting down netserver threads earlier to virNetDaemonClose.

Note: order of last daemon unref and virStateCleanup
is introduced in 85c3a182 for a valid reason.
---

One can use next patch to trigger crash on termination. Call domstats function 
and then send TERM to libvirtd.

[2] patch to trigger crash
#   diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
#   index cf5e4ad..39a57aa 100644
#   --- a/src/qemu/qemu_driver.c
#   +++ b/src/qemu/qemu_driver.c
#   @@ -20144,6 +20144,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
#domflags = 0;
#vm = vms[i];
#
#   +sleep(5);
#   +
#virObjectLock(vm);
#
#if (HAVE_JOB(privflags) &&

 src/rpc/virnetdaemon.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index e3b9390..c05df68 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -102,7 +102,8 @@ virNetDaemonDispose(void *obj)
 if (dmn->sigwatch > 0)
 virEventRemoveHandle(dmn->sigwatch);
 
-virHashFree(dmn->servers);
+if (dmn->servers)
+virHashFree(dmn->servers);
 
 virJSONValueFree(dmn->srvObject);
 }
@@ -880,6 +881,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
 virObjectLock(dmn);
 
 virHashForEach(dmn->servers, daemonServerClose, NULL);
+virHashFree(dmn->servers);
+dmn->servers = NULL;
 
 virObjectUnlock(dmn);
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/2] daemon: fix termination/reload issues

[Nikolay Shirokovskiy]

v1: https://www.redhat.com/archives/libvir-list/2017-September/msg01006.html

Changes from v1:

1. Patches that fixes qemu driver termination are moved out of this series
2. New version of [1] now fixes issue in a way that suggested by reviewer
3. [2] fixes another issue that was discovered in the process of review

Nikolay Shirokovskiy (2):
  libvirtd: fix crash on termination [1]
  virtlogd: add missing netserver refcount increment on reload   [2]

 src/locking/lock_daemon.c | 1 +
 src/rpc/virnetdaemon.c| 6 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/2] virtlogd: add missing netserver refcount increment on reload

[Nikolay Shirokovskiy]

After virNetDaemonAddServerPostExec call in virtlogd we should have
netserver refcount set to 2. One goes to netdaemon servers hashtable
and one goes to virtlogd own reference to netserver. Let's add
missing increment in virNetDaemonAddServerPostExec itself while holding
daemon lock.

We also have to unref new extra ref after virtlockd call to 
virNetDaemonAddServerPostExec.
---
 src/locking/lock_daemon.c | 1 +
 src/rpc/virnetdaemon.c| 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index fe3eaf9..41a06b2 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, 
bool privileged)
   virLockDaemonClientFree,
   (void*)(intptr_t)(privileged ? 
0x1 : 0x0
 goto error;
+virObjectUnref(srv);
 
 return lockd;
 
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index c05df68..8dc042b 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -313,6 +313,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
 
 if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
 goto error;
+virObjectRef(srv);
 
 virJSONValueFree(object);
 virObjectUnlock(dmn);
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] spec: Restart libvirtd in posttrans

[Pavel Hrdina]

On Mon, Oct 23, 2017 at 06:07:49PM +0200, Pavel Hrdina wrote:
> On Mon, Oct 23, 2017 at 04:00:57PM +0200, Jiri Denemark wrote:
> > When upgrading libvirt packages, there's no strict ordering for the
> > installation or removal of the individual libvirt sub packages. Thus
> > libvirt-daemon may be upgraded (and its %postun scriptlet) started
> > before all sub packages with driver libraries are upgraded. When
> > libvirt-daemon's %postun scriptlet restarts the daemon old drivers may
> > still be laying around and the daemon may crash when it tries to use
> > them.
> > 
> > Let's restart the daemon in %posttrans to make sure libvirtd is
> > restarted only after all sub packages are at the same version.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1464300
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  libvirt.spec.in | 16 +++-
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Pavel Hrdina 

After some discussion in person we might want to improve the spec file
to restart libvirtd only once.  In order to do that you can follow this
guide [1].  The idea is to create temporary file in %post which will
indicate that we should restart libvirtd and in %posttrans we will
restart libvirtd only if the file exists and remove it.  This will
ensure that only one %posttrans will actually restart libvirtd.

Pavel

[1] 


> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virsh: domain: Fix option handling in domxml-to-native

[Pavel Hrdina]

On Tue, Oct 24, 2017 at 09:51:31AM +0200, Peter Krempa wrote:
> Commit fdeac7a05fdf85458d72e89efcfa0f444525aaad tried to fix the output
> of 'virsh --help domxml-to-native' by switching types around. One of the
> changes broke the option parser. VSH_OT_ARGV should be used only for
> variable argument count, not to make the help generator look pretty.
> 
> The correct option type in this case is VSH_OT_STRING as it's not
> mandatory now since it can be substituted by using --domain.
> 
> This makes --help for this command look incorrect, but the parser works
> as it should.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1494400
> ---
>  tools/virsh-domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Reset hasManagedSave after removing a corrupted image

[Peter Krempa]

On Tue, Oct 24, 2017 at 10:39:53 +0200, Jiri Denemark wrote:
> When starting a domain with managed save image, we try to restore it
> first. If the image is corrupted, we silently unlink it and just
> normally start the domain. At this point the domain has no managed save
> image, yet we did not reset the hasManagedSave flag.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1460962
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_driver.c | 1 +
>  1 file changed, 1 insertion(+)

ACK


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] iohelper: use saferead if later write with O_DIRECT

[Jiri Denemark]

On Thu, Sep 28, 2017 at 10:06:47 +0300, Nikolay Shirokovskiy wrote:
> One of the usecases of iohelper is to read from pipe and write
> to file with O_DIRECT. As we read from pipe we can have partial
> read and then we fail to write this data because output file
> is open with O_DIRECT and buffer size is not aligned.
> ---
>  src/util/iohelper.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

ACK and pushed, thanks.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu: Reset hasManagedSave after removing a corrupted image

[Jiri Denemark]

When starting a domain with managed save image, we try to restore it
first. If the image is corrupted, we silently unlink it and just
normally start the domain. At this point the domain has no managed save
image, yet we did not reset the hasManagedSave flag.

https://bugzilla.redhat.com/show_bug.cgi?id=1460962

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 32a416f9e..74fdfdb0f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7256,6 +7256,7 @@ qemuDomainObjStart(virConnectPtr conn,
 } else {
 VIR_WARN("Ignoring incomplete managed state %s", managed_save);
 priv->job.current->operation = op;
+vm->hasManagedSave = false;
 }
 }
 }
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 3/5] qemu.conf: Introduce memory_predictable_file_names

[Daniel P. Berrange]

On Mon, Oct 23, 2017 at 07:10:04PM +0200, Michal Privoznik wrote:
> On 10/23/2017 06:47 PM, Daniel P. Berrange wrote:
> > On Mon, Oct 23, 2017 at 05:43:16PM +0200, Michal Privoznik wrote:
> >> In some cases management application needs to allocate memory for
> >> qemu upfront and then just let qemu use that. Since we don't want
> >> to expose path for memory-backend-file anywhere in the domain
> >> XML, we can have a configuration knob that when enabled generated
> >> predictable paths. In this case:
> >>
> >>   $memoryBackingDir/libvirt/qemu/$shortName/$alias
> >>
> >> where $shortName is result of virDomainObjGetShortName().
> > 
> > Looking at the current test cases, it seems that for huge pages
> > we currently create a directory
> > 
> >   $hugepagesMountPoint/libvirt/qemu/$shortName
> > 
> > and for non-hugepages we just use
> > 
> >$memoryBackingDir  (== /var/lib/libvirt/ram)
> > 
> > In both cases, we then just let QEMU create a random tempfile
> > name within these directories.
> > 
> 
> Correct.
> 
> > So, IIUC, your proposal here is just making the non-hugepages
> > case work the same way the hugepages case works, except that
> > it is adding an extra level of $alias in the directory path.
> 
> Almost. $alias is actual file, not dir.

Ah, ok, I couldn't see that from the diff :-)

> > I don't think this extra level of nesting is really desirable,
> > or needed. We should just give QEMU a filename, so that it
> > doesn't create a random tempfile. This is achieved by simply
> > doing a
> > 
> >mkdir($memoryBackingDir/libvirt/qemu/$shortName)
> > 
> > but then giving a
> > 
> >   mem-path=$memoryBackingDir/libvirt/qemu/$shortName/$alias
> 
> Yes. That's exactly what I'm doing. A snippet from next patch where we
> can see this in action:
> 
> 
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> index 5700c3413..352819429 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> @@ -13 +13,1 @@ QEMU_AUDIO_DRV=none \
> --object
> memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
> +-object
> memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
> 
> 
> And even though you're not disputing '/libvirt/qemu/' part in the path,
> I'll just point out for future reference that memoryBackingDir can be
> just any path in the system. It's just that the default is
> /var/lib/libvirt/qemu which makes the generated path ugly (and indeed
> renders the part redundant).

Yeah, that makes sense, even if it feels wierd. Better to be consistent
than to try to special case this. That said, it would be reasonable to
consider   /var/lib/libvirt/ram  as default mem path, given we include
the driver name.

> > QEMU will try to open that, and will get ENOENT, at which
> > point it to O_CREAT that file with the exact name we've
> > given, instead of using a tempfile name.
> > 
> > 
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>  src/qemu/libvirtd_qemu.aug |   1 +
> >>  src/qemu/qemu.conf |   5 ++
> >>  src/qemu/qemu_command.c|   9 +--
> >>  src/qemu/qemu_conf.c   |  76 +++-
> >>  src/qemu/qemu_conf.h   |  10 ++-
> >>  src/qemu/qemu_driver.c |  19 +
> >>  src/qemu/qemu_hotplug.c|   2 +-
> >>  src/qemu/qemu_process.c| 141 
> >> ++---
> >>  src/qemu/qemu_process.h|   8 +--
> >>  src/qemu/test_libvirtd_qemu.aug.in |   1 +
> >>  10 files changed, 220 insertions(+), 52 deletions(-)
> > 
> > I'd expect to see current tests updated wrt the new naming scheme
> > for paths
> 
> See next patch.

Ok, when you get rid of the config option, I guess you'll need to merge
that into this patch to avoid temporary regression in tests.


> > NB, this creates potentially many levels in the dir hiearchy
> > 
> >   $memoryBackingDir/libvirt
> >   $memoryBackingDir/libvirt/qemu/
> >   $memoryBackingDir/libvirt/qemu/$shortName
> 
> Only these ^^ are dirs.
> 
> >   $memoryBackingDir/libvirt/qemu/$shortName/$alias
> 
> This is actual file.

Ok

> >> +if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> >> +   def, path) < 0) {
> >> +virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +_("Unable to label %s"), path);
> >> +return -1;
> >> +}
> >> +} else {
> >> +if (rmdir(path) < 0 &&
> >> +errno != ENOENT)
> >> +VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
> >> + path, errno);
> > 
> > This only deletes
> > 
> >   $memoryBackingDir/libvirt/qemu/$shortName/$alias
> > 
> > so we're leaving around
> > 
> >   $memoryBackingDir/libvirt/qemu/$shortName
> 
> Is that so? I'

[libvirt] [PATCH] virsh: domain: Fix option handling in domxml-to-native

[Peter Krempa]

Commit fdeac7a05fdf85458d72e89efcfa0f444525aaad tried to fix the output
of 'virsh --help domxml-to-native' by switching types around. One of the
changes broke the option parser. VSH_OT_ARGV should be used only for
variable argument count, not to make the help generator look pretty.

The correct option type in this case is VSH_OT_STRING as it's not
mandatory now since it can be substituted by using --domain.

This makes --help for this command look incorrect, but the parser works
as it should.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1494400
---
 tools/virsh-domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index e1a312a6a..1e33e8295 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10152,7 +10152,7 @@ static const vshCmdOptDef opts_domxmltonative[] = {
  .help = N_("domain name, id or uuid")
 },
 {.name = "xml",
- .type = VSH_OT_ARGV,
+ .type = VSH_OT_STRING,
  .help = N_("xml data file to export from")
 },
 {.name = NULL}
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list