6:06 p.m.
Stumbled on this while changing qemu_conf.c for other
reasons. Moved code around here and there to avoid
repetition.
Final result is 23 less lines for the same functionality,
which is nothing to write home about but seems okay
to contribute.
Daniel Henrique Barboza (3):
qemu_conf.c: introduce qemuAddRemoveSharedHostdevInternal
qemu_conf.c: introduce qemuAddRemoveSharedDiskInternal
qemu_conf.c: introduce qemuAddRemoveSharedDeviceInternal
src/qemu/qemu_conf.c | 193 +++++++++++++++++++------------------------
1 file changed, 85 insertions(+), 108 deletions(-)
--
2.21.0
6:06 p.m.
New subject: [libvirt] [PATCH v1 1/3] qemu_conf.c: introduce qemuAddRemoveSharedHostdevInternal
qemuAddSharedHostdev() has a code similar to
qemuRemoveSharedHostdev(), with exception of one line that
defines the operation (add or remove).
This patch introduces a new function that aggregates the common
code, using a flag to switch between the operations, avoiding
code repetition.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 93 ++++++++++++++++++--------------------------
1 file changed, 37 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index d771bb6916..a583440807 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1722,9 +1722,33 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev)
static int
-qemuAddSharedHostdev(virQEMUDriverPtr driver,
- virDomainHostdevDefPtr hostdev,
- const char *name)
+qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
+ const char *key,
+ const char *name)
+{
+ qemuSharedDeviceEntryPtr entry = NULL;
+ int idx;
+
+ if (!(entry = virHashLookup(driver->sharedDevices, key)))
+ return -1;
+
+ /* Nothing to do if the shared disk is not recored in the table. */
+ if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
+ return 0;
+
+ if (entry->ref != 1)
+ VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
+ else
+ ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
+
+ return 0;
+}
+
+
+static int
+qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
+ virDomainHostdevDefPtr hostdev,
+ const char *name, bool addDevice)
{
char *dev_path = NULL;
char *key = NULL;
@@ -1740,37 +1764,19 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
goto cleanup;
qemuDriverLock(driver);
- ret = qemuSharedDeviceEntryInsert(driver, key, name);
+
+ if (addDevice)
+ ret = qemuSharedDeviceEntryInsert(driver, key, name);
+ else
+ ret = qemuSharedDeviceEntryRemove(driver, key, name);
+
qemuDriverUnlock(driver);
cleanup:
VIR_FREE(dev_path);
VIR_FREE(key);
return ret;
-}
-
-
-static int
-qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
- const char *key,
- const char *name)
-{
- qemuSharedDeviceEntryPtr entry = NULL;
- int idx;
-
- if (!(entry = virHashLookup(driver->sharedDevices, key)))
- return -1;
-
- /* Nothing to do if the shared disk is not recored in the table. */
- if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
- return 0;
- if (entry->ref != 1)
- VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
- else
- ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
-
- return 0;
}
@@ -1795,7 +1801,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
if (dev->type == VIR_DOMAIN_DEVICE_DISK)
return qemuAddSharedDisk(driver, dev->data.disk, name);
else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
- return qemuAddSharedHostdev(driver, dev->data.hostdev, name);
+ return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
+ name, true);
else
return 0;
}
@@ -1830,33 +1837,6 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
}
-static int
-qemuRemoveSharedHostdev(virQEMUDriverPtr driver,
- virDomainHostdevDefPtr hostdev,
- const char *name)
-{
- char *dev_path = NULL;
- char *key = NULL;
- int ret = -1;
-
- if (!qemuIsSharedHostdev(hostdev))
- return 0;
-
- if (!(dev_path = qemuGetHostdevPath(hostdev)))
- goto cleanup;
-
- if (!(key = qemuGetSharedDeviceKey(dev_path)))
- goto cleanup;
-
- qemuDriverLock(driver);
- ret = qemuSharedDeviceEntryRemove(driver, key, name);
- qemuDriverUnlock(driver);
-
- cleanup:
- VIR_FREE(dev_path);
- VIR_FREE(key);
- return ret;
-}
/* qemuRemoveSharedDevice:
@@ -1876,7 +1856,8 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver,
if (dev->type == VIR_DOMAIN_DEVICE_DISK)
return qemuRemoveSharedDisk(driver, dev->data.disk, name);
else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
- return qemuRemoveSharedHostdev(driver, dev->data.hostdev, name);
+ return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
+ name, false);
else
return 0;
}
--
2.21.0
6:32 a.m.
New subject: [libvirt] [PATCH v1 1/3] qemu_conf.c: introduce qemuAddRemoveSharedHostdevInternal
On Tue, Sep 03, 2019 at 08:06:05PM -0300, Daniel Henrique Barboza wrote:
qemuAddSharedHostdev() has a code similar to
qemuRemoveSharedHostdev(), with exception of one line that
defines the operation (add or remove).
This patch introduces a new function that aggregates the common
code, using a flag to switch between the operations, avoiding
code repetition.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 93 ++++++++++++++++++--------------------------
1 file changed, 37 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index d771bb6916..a583440807 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1722,9 +1722,33 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev)
static int
-qemuAddSharedHostdev(virQEMUDriverPtr driver,
- virDomainHostdevDefPtr hostdev,
- const char *name)
+qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
+ const char *key,
+ const char *name)
+{
+ qemuSharedDeviceEntryPtr entry = NULL;
+ int idx;
+
+ if (!(entry = virHashLookup(driver->sharedDevices, key)))
+ return -1;
+
+ /* Nothing to do if the shared disk is not recored in the table. */
+ if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
+ return 0;
+
+ if (entry->ref != 1)
+ VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
+ else
+ ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
+
+ return 0;
+}
+
+
+static int
+qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
+ virDomainHostdevDefPtr hostdev,
+ const char *name, bool addDevice)
We prefer to have each parameter on separate line, the only case were
having them on a single line is for files where that code style is the
main one.
There is no need to name the function as Internal, it would make sense
only in case where you would keep the original functions
qemuAddSharedHostdev and qemuSharedDeviceEntryRemove where they would
internally call the new function with proper flag set.
Pavel
{
char *dev_path = NULL;
char *key = NULL;
@@ -1740,37 +1764,19 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
goto cleanup;
qemuDriverLock(driver);
- ret = qemuSharedDeviceEntryInsert(driver, key, name);
+
+ if (addDevice)
+ ret = qemuSharedDeviceEntryInsert(driver, key, name);
+ else
+ ret = qemuSharedDeviceEntryRemove(driver, key, name);
+
qemuDriverUnlock(driver);
cleanup:
VIR_FREE(dev_path);
VIR_FREE(key);
return ret;
-}
-
-
-static int
-qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
- const char *key,
- const char *name)
-{
- qemuSharedDeviceEntryPtr entry = NULL;
- int idx;
-
- if (!(entry = virHashLookup(driver->sharedDevices, key)))
- return -1;
-
- /* Nothing to do if the shared disk is not recored in the table. */
- if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
- return 0;
- if (entry->ref != 1)
- VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
- else
- ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
-
- return 0;
}
@@ -1795,7 +1801,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
if (dev->type == VIR_DOMAIN_DEVICE_DISK)
return qemuAddSharedDisk(driver, dev->data.disk, name);
else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
- return qemuAddSharedHostdev(driver, dev->data.hostdev, name);
+ return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
+ name, true);
else
return 0;
}
@@ -1830,33 +1837,6 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
}
-static int
-qemuRemoveSharedHostdev(virQEMUDriverPtr driver,
- virDomainHostdevDefPtr hostdev,
- const char *name)
-{
- char *dev_path = NULL;
- char *key = NULL;
- int ret = -1;
-
- if (!qemuIsSharedHostdev(hostdev))
- return 0;
-
- if (!(dev_path = qemuGetHostdevPath(hostdev)))
- goto cleanup;
-
- if (!(key = qemuGetSharedDeviceKey(dev_path)))
- goto cleanup;
-
- qemuDriverLock(driver);
- ret = qemuSharedDeviceEntryRemove(driver, key, name);
- qemuDriverUnlock(driver);
-
- cleanup:
- VIR_FREE(dev_path);
- VIR_FREE(key);
- return ret;
-}
/* qemuRemoveSharedDevice:
@@ -1876,7 +1856,8 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver,
if (dev->type == VIR_DOMAIN_DEVICE_DISK)
return qemuRemoveSharedDisk(driver, dev->data.disk, name);
else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
- return qemuRemoveSharedHostdev(driver, dev->data.hostdev, name);
+ return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
+ name, false);
else
return 0;
}
--
2.21.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:55 a.m.
New subject: [libvirt] [PATCH v1 1/3] qemu_conf.c: introduce qemuAddRemoveSharedHostdevInternal
On 9/4/19 1:06 AM, Daniel Henrique Barboza wrote:
qemuAddSharedHostdev() has a code similar to
qemuRemoveSharedHostdev(), with exception of one line that
defines the operation (add or remove).
This patch introduces a new function that aggregates the common
code, using a flag to switch between the operations, avoiding
code repetition.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 93 ++++++++++++++++++--------------------------
1 file changed, 37 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index d771bb6916..a583440807 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1722,9 +1722,33 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev)
static int
-qemuAddSharedHostdev(virQEMUDriverPtr driver,
- virDomainHostdevDefPtr hostdev,
- const char *name)
+qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
+ const char *key,
+ const char *name)
+{
+ qemuSharedDeviceEntryPtr entry = NULL;
+ int idx;
+
+ if (!(entry = virHashLookup(driver->sharedDevices, key)))
+ return -1;
+
+ /* Nothing to do if the shared disk is not recored in the table. */
+ if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
+ return 0;
+
+ if (entry->ref != 1)
+ VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
+ else
+ ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
+
+ return 0;
+}
+
+
+static int
+qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
+ virDomainHostdevDefPtr hostdev,
+ const char *name, bool addDevice)
How about qemuSharedHostdevAddRemoveInternal() for a name? It follows
"object" + "operation" naming schema better.
{
char *dev_path = NULL;
char *key = NULL;
Also, this can be turned into VIR_AUTOFREE().
Michal
6:06 p.m.
New subject: [libvirt] [PATCH v1 2/3] qemu_conf.c: introduce qemuAddRemoveSharedDiskInternal
Following the same idea of avoid code repetition from the
previous patch, this commit introduces a new function that
aggregates the functions of qemuAddSharedDisk() and
qemuRemoveSharedDisk() into a single place, using a flag to
switch between add/remove operations.
Both qemuAddSharedDisk() and qemuRemoveSharedDisk() are
public, so keep them around to avoid changing other files
due to an internal qemu_conf.c refactory.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 130 +++++++++++++++++++++----------------------
1 file changed, 62 insertions(+), 68 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a583440807..422f0d743d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1646,19 +1646,35 @@ qemuSharedDeviceEntryInsert(virQEMUDriverPtr driver,
}
-/* qemuAddSharedDisk:
- * @driver: Pointer to qemu driver struct
- * @src: disk source
- * @name: The domain name
- *
- * Increase ref count and add the domain name into the list which
- * records all the domains that use the shared device if the entry
- * already exists, otherwise add a new entry.
- */
-int
-qemuAddSharedDisk(virQEMUDriverPtr driver,
- virDomainDiskDefPtr disk,
- const char *name)
+static int
+qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
+ const char *key,
+ const char *name)
+{
+ qemuSharedDeviceEntryPtr entry = NULL;
+ int idx;
+
+ if (!(entry = virHashLookup(driver->sharedDevices, key)))
+ return -1;
+
+ /* Nothing to do if the shared disk is not recored in the table. */
+ if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
+ return 0;
+
+ if (entry->ref != 1)
+ VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
+ else
+ ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
+
+ return 0;
+}
+
+
+static int
+qemuAddRemoveSharedDiskInternal(virQEMUDriverPtr driver,
+ virDomainDiskDefPtr disk,
+ const char *name,
+ bool addDisk)
{
char *key = NULL;
int ret = -1;
@@ -1670,17 +1686,21 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
qemuDriverLock(driver);
- if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0)
- goto cleanup;
-
if (!(key = qemuGetSharedDeviceKey(virDomainDiskGetSource(disk))))
goto cleanup;
- if (qemuSharedDeviceEntryInsert(driver, key, name) < 0)
- goto cleanup;
+ if (addDisk) {
+ if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0)
+ goto cleanup;
- ret = 0;
+ if (qemuSharedDeviceEntryInsert(driver, key, name) < 0)
+ goto cleanup;
+ } else {
+ if (qemuSharedDeviceEntryRemove(driver, key, name) < 0)
+ goto cleanup;
+ }
+ ret = 0;
cleanup:
qemuDriverUnlock(driver);
VIR_FREE(key);
@@ -1688,6 +1708,24 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
}
+/* qemuAddSharedDisk:
+ * @driver: Pointer to qemu driver struct
+ * @src: disk source
+ * @name: The domain name
+ *
+ * Increase ref count and add the domain name into the list which
+ * records all the domains that use the shared device if the entry
+ * already exists, otherwise add a new entry.
+ */
+int
+qemuAddSharedDisk(virQEMUDriverPtr driver,
+ virDomainDiskDefPtr disk,
+ const char *name)
+{
+ return qemuAddRemoveSharedDiskInternal(driver, disk, name, true);
+}
+
+
static bool
qemuIsSharedHostdev(virDomainHostdevDefPtr hostdev)
{
@@ -1721,30 +1759,6 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev)
}
-static int
-qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
- const char *key,
- const char *name)
-{
- qemuSharedDeviceEntryPtr entry = NULL;
- int idx;
-
- if (!(entry = virHashLookup(driver->sharedDevices, key)))
- return -1;
-
- /* Nothing to do if the shared disk is not recored in the table. */
- if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
- return 0;
-
- if (entry->ref != 1)
- VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
- else
- ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
-
- return 0;
-}
-
-
static int
qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev,
@@ -1799,7 +1813,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
* which is only valid for block disk and scsi host device.
*/
if (dev->type == VIR_DOMAIN_DEVICE_DISK)
- return qemuAddSharedDisk(driver, dev->data.disk, name);
+ return qemuAddRemoveSharedDiskInternal(driver, dev->data.disk,
+ name, true);
else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
name, true);
@@ -1813,32 +1828,10 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
const char *name)
{
- char *key = NULL;
- int ret = -1;
-
- if (virStorageSourceIsEmpty(disk->src) ||
- !disk->src->shared ||
- !virStorageSourceIsBlockLocal(disk->src))
- return 0;
-
- qemuDriverLock(driver);
-
- if (!(key = qemuGetSharedDeviceKey(virDomainDiskGetSource(disk))))
- goto cleanup;
-
- if (qemuSharedDeviceEntryRemove(driver, key, name) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- qemuDriverUnlock(driver);
- VIR_FREE(key);
- return ret;
+ return qemuAddRemoveSharedDiskInternal(driver, disk, name, false);
}
-
-
/* qemuRemoveSharedDevice:
* @driver: Pointer to qemu driver struct
* @device: The device def
@@ -1854,7 +1847,8 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver,
const char *name)
{
if (dev->type == VIR_DOMAIN_DEVICE_DISK)
- return qemuRemoveSharedDisk(driver, dev->data.disk, name);
+ return qemuAddRemoveSharedDiskInternal(driver, dev->data.disk,
+ name, false);
else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
name, false);
--
2.21.0
7:55 a.m.
New subject: [libvirt] [PATCH v1 2/3] qemu_conf.c: introduce qemuAddRemoveSharedDiskInternal
On 9/4/19 1:06 AM, Daniel Henrique Barboza wrote:
Following the same idea of avoid code repetition from the
previous patch, this commit introduces a new function that
aggregates the functions of qemuAddSharedDisk() and
qemuRemoveSharedDisk() into a single place, using a flag to
switch between add/remove operations.
Both qemuAddSharedDisk() and qemuRemoveSharedDisk() are
public, so keep them around to avoid changing other files
due to an internal qemu_conf.c refactory.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 130 +++++++++++++++++++++----------------------
1 file changed, 62 insertions(+), 68 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a583440807..422f0d743d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1646,19 +1646,35 @@ qemuSharedDeviceEntryInsert(virQEMUDriverPtr driver,
}
-/* qemuAddSharedDisk:
- * @driver: Pointer to qemu driver struct
- * @src: disk source
- * @name: The domain name
- *
- * Increase ref count and add the domain name into the list which
- * records all the domains that use the shared device if the entry
- * already exists, otherwise add a new entry.
- */
-int
-qemuAddSharedDisk(virQEMUDriverPtr driver,
- virDomainDiskDefPtr disk,
- const char *name)
+static int
+qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
+ const char *key,
+ const char *name)
+{
+ qemuSharedDeviceEntryPtr entry = NULL;
+ int idx;
+
+ if (!(entry = virHashLookup(driver->sharedDevices, key)))
+ return -1;
+
+ /* Nothing to do if the shared disk is not recored in the table. */
+ if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
+ return 0;
+
+ if (entry->ref != 1)
+ VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
+ else
+ ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
+
+ return 0;
+}
+
+
+static int
+qemuAddRemoveSharedDiskInternal(virQEMUDriverPtr driver,
+ virDomainDiskDefPtr disk,
+ const char *name,
+ bool addDisk)
This can be named qemuSharedDiskAddRemoveInternal().
Michal
6:06 p.m.
New subject: [libvirt] [PATCH v1 3/3] qemu_conf.c: introduce qemuAddRemoveSharedDeviceInternal
After the previous commits, qemuAddSharedDevice() and
qemuRemoveSharedDevice() are now the same code with a different
flag to call the internal functions.
This patch aggregates the common code into a new function called
qemuAddRemoveSharedDeviceInternal() to further reduce
code repetition. Both qemuAddSharedDevice() and
qemuRemoveSharedDevice() are kept since they are public
functions used elsewhere.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 422f0d743d..6ce2d10ac7 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1793,6 +1793,26 @@ qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
}
+static int
+qemuAddRemoveSharedDeviceInternal(virQEMUDriverPtr driver,
+ virDomainDeviceDefPtr dev,
+ const char *name,
+ bool addDevice)
+{
+ /* Currently the only conflicts we have to care about for
+ * the shared disk and shared host device is "sgio" setting,
+ * which is only valid for block disk and scsi host device.
+ */
+ if (dev->type == VIR_DOMAIN_DEVICE_DISK)
+ return qemuAddRemoveSharedDiskInternal(driver, dev->data.disk,
+ name, addDevice);
+ else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
+ return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
+ name, addDevice);
+ else
+ return 0;
+}
+
/* qemuAddSharedDevice:
* @driver: Pointer to qemu driver struct
@@ -1808,18 +1828,7 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
virDomainDeviceDefPtr dev,
const char *name)
{
- /* Currently the only conflicts we have to care about for
- * the shared disk and shared host device is "sgio" setting,
- * which is only valid for block disk and scsi host device.
- */
- if (dev->type == VIR_DOMAIN_DEVICE_DISK)
- return qemuAddRemoveSharedDiskInternal(driver, dev->data.disk,
- name, true);
- else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
- return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
- name, true);
- else
- return 0;
+ return qemuAddRemoveSharedDeviceInternal(driver, dev, name, true);
}
@@ -1846,14 +1855,7 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver,
virDomainDeviceDefPtr dev,
const char *name)
{
- if (dev->type == VIR_DOMAIN_DEVICE_DISK)
- return qemuAddRemoveSharedDiskInternal(driver, dev->data.disk,
- name, false);
- else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
- return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
- name, false);
- else
- return 0;
+ return qemuAddRemoveSharedDeviceInternal(driver, dev, name, false);
}
--
2.21.0
7:55 a.m.
New subject: [libvirt] [PATCH v1 3/3] qemu_conf.c: introduce qemuAddRemoveSharedDeviceInternal
On 9/4/19 1:06 AM, Daniel Henrique Barboza wrote:
After the previous commits, qemuAddSharedDevice() and
qemuRemoveSharedDevice() are now the same code with a different
flag to call the internal functions.
This patch aggregates the common code into a new function called
qemuAddRemoveSharedDeviceInternal() to further reduce
code repetition. Both qemuAddSharedDevice() and
qemuRemoveSharedDevice() are kept since they are public
functions used elsewhere.
No functional change was made.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 422f0d743d..6ce2d10ac7 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1793,6 +1793,26 @@ qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
}
+static int
+qemuAddRemoveSharedDeviceInternal(virQEMUDriverPtr driver,
Again, qemuSharedDeviceAddRemoveInternal()?
I've made all the changes and keep them locally. Just let me know how
you feel about my suggestions so that I can ACK these and push them.
Michal
8:08 a.m.
New subject: [libvirt] [PATCH v1 3/3] qemu_conf.c: introduce qemuAddRemoveSharedDeviceInternal
On 9/9/19 9:55 AM, Michal Privoznik wrote:
On 9/4/19 1:06 AM, Daniel Henrique Barboza wrote:
> After the previous commits, qemuAddSharedDevice() and
> qemuRemoveSharedDevice() are now the same code with a different
> flag to call the internal functions.
>
> This patch aggregates the common code into a new function called
> qemuAddRemoveSharedDeviceInternal() to further reduce
> code repetition. Both qemuAddSharedDevice() and
> qemuRemoveSharedDevice() are kept since they are public
> functions used elsewhere.
>
> No functional change was made.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 422f0d743d..6ce2d10ac7 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1793,6 +1793,26 @@
> qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
> }
> +static int
> +qemuAddRemoveSharedDeviceInternal(virQEMUDriverPtr driver,
Again, qemuSharedDeviceAddRemoveInternal()?
I've made all the changes and keep them locally. Just let me know how
you feel about my suggestions so that I can ACK these and push them.
I'm fine with the changes but it seems like Pavel already pushed the
patches. Feel free to create a patch of your own with these changes
and I'll ack/review it no problem.
Thanks,
DHB
Michal
7:53 a.m.
On Tue, Sep 03, 2019 at 08:06:04PM -0300, Daniel Henrique Barboza wrote:
Stumbled on this while changing qemu_conf.c for other
reasons. Moved code around here and there to avoid
repetition.
Final result is 23 less lines for the same functionality,
which is nothing to write home about but seems okay
to contribute.
Daniel Henrique Barboza (3):
qemu_conf.c: introduce qemuAddRemoveSharedHostdevInternal
qemu_conf.c: introduce qemuAddRemoveSharedDiskInternal
qemu_conf.c: introduce qemuAddRemoveSharedDeviceInternal
src/qemu/qemu_conf.c | 193 +++++++++++++++++++------------------------
1 file changed, 85 insertions(+), 108 deletions(-)
Feel free to ignore my comment about the Internal name as it makes sense
with the last patch. I'll fix the parameter to be on its own line and
push it, thanks.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
1901
days inactive
1907
days old
9 comments
3 participants
participants (3)
-
Daniel Henrique Barboza -
Michal Privoznik -
Pavel Hrdina