On 04/03/2013 11:50 AM, Ján Tomko wrote:
> Each bus (just one so far) is represented by an array
> with 32 slots where each slot is stored as an 8-bit integer
> where each bit represents a function.
>
> This makes operations with whole slots easier.
> ---
> src/qemu/qemu_command.c | 152 +++++++++++++++---------------------------------
> 1 file changed, 48 insertions(+), 104 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a16d5f1..e221c82 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1187,8 +1187,14 @@ cleanup:
>
> #define QEMU_PCI_ADDRESS_LAST_SLOT 32
> #define QEMU_PCI_ADDRESS_LAST_FUNCTION 8
> +
> +/*
> + * Each bit represents a function
> + * Each byte represents a slot
> + */
> +typedef uint8_t _qemuDomainPCIAddressBus[QEMU_PCI_ADDRESS_LAST_SLOT];
I'm not sure why _qemuDomainPCIAddressSet has a _ at the beginning, but
in general I think we frown on prepending _ to definitions that are
local to a file.
> struct _qemuDomainPCIAddressSet {
> - virHashTablePtr used;
> + _qemuDomainPCIAddressBus *used;
> virDevicePCIAddress lastaddr;
> };
>
> @@ -1269,7 +1275,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> if (!str)
> goto cleanup;
>
> - if (virHashLookup(addrs->used, str)) {
> + if (qemuDomainPCIAddressReserveAddr(addrs, addr) < 0) {
> if (info->addr.pci.function != 0) {
> virReportError(VIR_ERR_XML_ERROR,
> _("Attempted double use of PCI Address '%s' "
> @@ -1282,36 +1288,21 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> goto cleanup;
> }
>
> - VIR_DEBUG("Remembering PCI addr %s", str);
> - if (virHashAddEntry(addrs->used, str, str) < 0)
> - goto cleanup;
> - str = NULL;
> -
> if ((info->addr.pci.function == 0) &&
> (info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) {
> /* a function 0 w/o multifunction=on must reserve the entire slot */
> - virDevicePCIAddress tmp_addr = *addr;
> - unsigned int *func = &tmp_addr.function;
> -
> - for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> - str = qemuPCIAddressAsString(&tmp_addr);
> - if (!str)
> - goto cleanup;
> -
> - if (virHashLookup(addrs->used, str)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Attempted double use of PCI Address '%s' "
> - "(need \"multifunction='off'\" for device "
> - "on function 0)"),
> - str);
> - goto cleanup;
> - }
> -
> - VIR_DEBUG("Remembering PCI addr %s (multifunction=off for function 0)", str);
> - if (virHashAddEntry(addrs->used, str, str))
> - goto cleanup;
> - str = NULL;
> + ignore_value(qemuDomainPCIAddressReleaseAddr(addrs, addr));
> + if (qemuDomainPCIAddressReserveSlot(addrs, addr) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Attempted double use of PCI Address '%s' "
> + "(need \"multifunction='off'\" for device "
> + "on function 0)"),
This message isn't exactly correct - str contains the address for
function 0, but it's possible that it was one of the other functions
that caused the problem.
But there is yet another problem - qemuDomainPCIAddressReserveSlot()
itself has already reported the error. You may want to have that
function (and qemuDomainPCIAddressReserveAddr() not report any error,
but rely on the caller to report the error (since the caller will have
more context, such as (in this case) that multifunction was set to on).
> + str);
> + goto cleanup;
> }
> + VIR_DEBUG("Remembering PCI slot: %s (multifunction=off)", str);
> + } else {
> + VIR_DEBUG("Remembering PCI addr: %s", str);
> }
> ret = 0;
> cleanup:
> @@ -1375,13 +1366,6 @@ int qemuDomainAssignAddresses(virDomainDefPtr def,
> return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
> }
>
> -static void
> -qemuDomainPCIAddressSetFreeEntry(void *payload,
> - const void *name ATTRIBUTE_UNUSED)
> -{
> - VIR_FREE(payload);
> -}
> -
> qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def)
> {
> qemuDomainPCIAddressSetPtr addrs;
> @@ -1389,8 +1373,8 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def)
> if (VIR_ALLOC(addrs) < 0)
> goto no_memory;
>
> - if (!(addrs->used = virHashCreate(10, qemuDomainPCIAddressSetFreeEntry)))
> - goto error;
> + if (VIR_ALLOC_N(addrs->used, 1) < 0)
> + goto no_memory;
>
> if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0)
> goto error;
> @@ -1411,25 +1395,11 @@ error:
> static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr)
> {
> - char *str;
> - virDevicePCIAddress tmp_addr = *addr;
> - unsigned int *func = &(tmp_addr.function);
> -
> if (qemuPCIAddressCheck(addrs, addr) < 0)
> return -1;
>
> - for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> - str = qemuPCIAddressAsString(&tmp_addr);
> - if (!str)
> - return -1;
> -
> - if (virHashLookup(addrs->used, str)) {
> - VIR_FREE(str);
> - return -1;
> - }
> -
> - VIR_FREE(str);
> - }
> + if (addrs->used[addr->bus][addr->slot])
> + return -1;
>
> return 0;
> }
> @@ -1448,42 +1418,46 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
>
> VIR_DEBUG("Reserving PCI addr %s", str);
>
> - if (virHashLookup(addrs->used, str)) {
> + if (addrs->used[addr->bus][addr->slot] & 1 << addr->function) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("unable to reserve PCI address %s"), str);
> VIR_FREE(str);
> return -1;
> }
>
> - if (virHashAddEntry(addrs->used, str, str)) {
> - VIR_FREE(str);
> - return -1;
> - }
> + VIR_FREE(str);
>
> addrs->lastaddr = *addr;
> addrs->lastaddr.function = 0;
> addrs->lastaddr.multi = 0;
> + addrs->used[addr->bus][addr->slot] |= 1 << addr->function;
> return 0;
> }
>
> int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr)
> {
> - virDevicePCIAddress tmp_addr = *addr;
> - unsigned int *func = &tmp_addr.function;
> - unsigned int last;
> + char *str;
>
> - for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> - if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr) < 0)
> - goto cleanup;
> + if (qemuPCIAddressCheck(addrs, addr) < 0)
> + return -1;
> +
> + str = qemuPCIAddressAsString(addr);
> + if (!str)
> + return -1;
> +
> + VIR_DEBUG("Reserving PCI slot %s", str);
> +
> + if (addrs->used[addr->bus][addr->slot]) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to reserve PCI slot %s"), str);
> + VIR_FREE(str);
> + return -1;
> }
>
> + VIR_FREE(str);
> + addrs->used[addr->bus][addr->slot] = 0xFF;
> return 0;
> -
> -cleanup:
> - for (last = *func, *func = 0; *func < last; (*func)++)
> - qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr);
> - return -1;
> }
>
> int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
> @@ -1512,51 +1486,21 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
> int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr)
> {
> - char *str;
> - int ret;
> -
> if (qemuPCIAddressCheck(addrs, addr) < 0)
> return -1;
>
> - str = qemuPCIAddressAsString(addr);
> - if (!str)
> - return -1;
> -
> - ret = virHashRemoveEntry(addrs->used, str);
> -
> - VIR_FREE(str);
> -
> - return ret;
> + addrs->used[addr->bus][addr->slot] &= ~(1 << addr->function);
> + return 0;
> }
>
> int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr)
> {
> - char *str;
> - int ret = 0;
> - virDevicePCIAddress tmp_addr = *addr;
> - unsigned int *func = &tmp_addr.function;
> -
> if (qemuPCIAddressCheck(addrs, addr) < 0)
> return -1;
>
> - for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> - str = qemuPCIAddressAsString(&tmp_addr);
> - if (!str)
> - return -1;
> -
> - if (!virHashLookup(addrs->used, str)) {
> - VIR_FREE(str);
> - continue;
> - }
> -
> - VIR_FREE(str);
> -
> - if (qemuDomainPCIAddressReleaseAddr(addrs, &tmp_addr) < 0)
> - ret = -1;
> - }
> -
> - return ret;
> + addrs->used[addr->bus][addr->slot] = 0;
> + return 0;
> }
>
> void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
> @@ -1564,7 +1508,7 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
> if (!addrs)
> return;
>
> - virHashFree(addrs->used);
> + VIR_FREE(addrs->used);
> VIR_FREE(addrs);
> }
>
Otherwise looks okay.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list