[libvirt] [PATCH] util: use VIR_(APPEND|DELETE)_ELEMENT for pci/usb device lists
Eliminate memmove() by using VIR_*_ELEMENT API instead. In both pci and usb cases, the count that held the size of the list was unsigned int so it had to be changed to size_t. --- This is an alternate fix to the same problem fixed by https://www.redhat.com/archives/libvir-list/2013-July/msg00324.html (an off by one bug in the conditional in virUSBDeviceListSteal()). This version fixes it by converting from open coded memmoves to using VIR_(INSERT|DELETE)_ELEMENT. src/util/virpci.c | 18 +++--------------- src/util/virusb.c | 26 +++++++------------------- 2 files changed, 10 insertions(+), 34 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index e42f6fa..b715122 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -80,7 +80,7 @@ struct _virPCIDevice { struct _virPCIDeviceList { virObjectLockable parent; - unsigned int count; + size_t count; virPCIDevicePtr *devs; }; @@ -1670,11 +1670,9 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list, return -1; } - if (VIR_REALLOC_N(list->devs, list->count+1) < 0) + if (VIR_APPEND_ELEMENT(list->devs, list->count, dev, true) < 0) return -1; - list->devs[list->count++] = dev; - return 0; } @@ -1723,17 +1721,7 @@ virPCIDeviceListStealIndex(virPCIDeviceListPtr list, return NULL; ret = list->devs[idx]; - - if (idx != --list->count) { - memmove(&list->devs[idx], - &list->devs[idx + 1], - sizeof(*list->devs) * (list->count - idx)); - } - - if (VIR_REALLOC_N(list->devs, list->count) < 0) { - ; /* not fatal */ - } - + VIR_DELETE_ELEMENT(list->devs, idx, list->count); return ret; } diff --git a/src/util/virusb.c b/src/util/virusb.c index 3bcb07c..103c301 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -60,7 +60,7 @@ struct _virUSBDevice { struct _virUSBDeviceList { virObjectLockable parent; - unsigned int count; + size_t count; virUSBDevicePtr *devs; }; @@ -451,11 +451,9 @@ virUSBDeviceListAdd(virUSBDeviceListPtr list, return -1; } - if (VIR_REALLOC_N(list->devs, list->count+1) < 0) + if (VIR_APPEND_ELEMENT(list->devs, list->count, dev, true) < 0) return -1; - list->devs[list->count++] = dev; - return 0; } @@ -484,22 +482,12 @@ virUSBDeviceListSteal(virUSBDeviceListPtr list, int i; for (i = 0; i < list->count; i++) { - if (list->devs[i]->bus != dev->bus || - list->devs[i]->dev != dev->dev) - continue; - - ret = list->devs[i]; - - if (i != list->count--) - memmove(&list->devs[i], - &list->devs[i+1], - sizeof(*list->devs) * (list->count - i)); - - if (VIR_REALLOC_N(list->devs, list->count) < 0) { - ; /* not fatal */ + if (list->devs[i]->bus == dev->bus && + list->devs[i]->dev == dev->dev) { + ret = list->devs[i]; + VIR_DELETE_ELEMENT(list->devs, i, list->count); + break; } - - break; } return ret; } -- 1.7.11.7
On 05.07.2013 20:46, Laine Stump wrote:
Eliminate memmove() by using VIR_*_ELEMENT API instead.
In both pci and usb cases, the count that held the size of the list was unsigned int so it had to be changed to size_t. --- This is an alternate fix to the same problem fixed by
https://www.redhat.com/archives/libvir-list/2013-July/msg00324.html
(an off by one bug in the conditional in virUSBDeviceListSteal()).
This version fixes it by converting from open coded memmoves to using VIR_(INSERT|DELETE)_ELEMENT.
src/util/virpci.c | 18 +++--------------- src/util/virusb.c | 26 +++++++------------------- 2 files changed, 10 insertions(+), 34 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index e42f6fa..b715122 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -80,7 +80,7 @@ struct _virPCIDevice { struct _virPCIDeviceList { virObjectLockable parent;
- unsigned int count; + size_t count; virPCIDevicePtr *devs; };
@@ -1670,11 +1670,9 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list, return -1; }
- if (VIR_REALLOC_N(list->devs, list->count+1) < 0) + if (VIR_APPEND_ELEMENT(list->devs, list->count, dev, true) < 0) return -1;
- list->devs[list->count++] = dev; - return 0; }
@@ -1723,17 +1721,7 @@ virPCIDeviceListStealIndex(virPCIDeviceListPtr list, return NULL;
ret = list->devs[idx]; - - if (idx != --list->count) { - memmove(&list->devs[idx], - &list->devs[idx + 1], - sizeof(*list->devs) * (list->count - idx)); - } - - if (VIR_REALLOC_N(list->devs, list->count) < 0) { - ; /* not fatal */ - } - + VIR_DELETE_ELEMENT(list->devs, idx, list->count); return ret; }
diff --git a/src/util/virusb.c b/src/util/virusb.c index 3bcb07c..103c301 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -60,7 +60,7 @@ struct _virUSBDevice {
struct _virUSBDeviceList { virObjectLockable parent; - unsigned int count; + size_t count; virUSBDevicePtr *devs; };
@@ -451,11 +451,9 @@ virUSBDeviceListAdd(virUSBDeviceListPtr list, return -1; }
- if (VIR_REALLOC_N(list->devs, list->count+1) < 0) + if (VIR_APPEND_ELEMENT(list->devs, list->count, dev, true) < 0) return -1;
- list->devs[list->count++] = dev; - return 0; }
@@ -484,22 +482,12 @@ virUSBDeviceListSteal(virUSBDeviceListPtr list, int i;
for (i = 0; i < list->count; i++) { - if (list->devs[i]->bus != dev->bus || - list->devs[i]->dev != dev->dev) - continue; - - ret = list->devs[i]; - - if (i != list->count--) - memmove(&list->devs[i], - &list->devs[i+1], - sizeof(*list->devs) * (list->count - i)); - - if (VIR_REALLOC_N(list->devs, list->count) < 0) { - ; /* not fatal */ + if (list->devs[i]->bus == dev->bus && + list->devs[i]->dev == dev->dev) { + ret = list->devs[i]; + VIR_DELETE_ELEMENT(list->devs, i, list->count); + break; } - - break; } return ret; }
ACK Michal
On 07/08/2013 09:48 AM, Michal Privoznik wrote:
On 05.07.2013 20:46, Laine Stump wrote:
Eliminate memmove() by using VIR_*_ELEMENT API instead.
In both pci and usb cases, the count that held the size of the list was unsigned int so it had to be changed to size_t. --- This is an alternate fix to the same problem fixed by
https://www.redhat.com/archives/libvir-list/2013-July/msg00324.html
(an off by one bug in the conditional in virUSBDeviceListSteal()).
This version fixes it by converting from open coded memmoves to using VIR_(INSERT|DELETE)_ELEMENT.
ACK
Now that Michal has pushed the VIR_ALLOC OOM reporting patches (which were a prerequisite to this), I rebased to the new (old) arglist for VIR_APPEND_ELEMENT and pushed. Thanks!
participants (2)
-
Laine Stump -
Michal Privoznik