[libvirt] [PATCH v1 0/3] small code repetition purge on qemu_conf.c
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
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@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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@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
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@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
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@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
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@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
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@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
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@redhat.com>
participants (3)
-
Daniel Henrique Barboza -
Michal Privoznik -
Pavel Hrdina