[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/7] qemu: move PCI address check out of qemuPCIAddressAsString



On 04/03/2013 11:50 AM, Ján Tomko wrote:
> Move bus and domain checks from qemuPCIAddressAsString to
> a separate function and add a check for function and slot
> so that we can switch from a hash table to an array.
>
> Remove redundant checks in qemuBuildDeviceAddressStr.
> ---
>  src/qemu/qemu_command.c | 111 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 68 insertions(+), 43 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8321dcd..a16d5f1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1193,17 +1193,43 @@ struct _qemuDomainPCIAddressSet {
>  };
>  
>  
> +/* Check the PCI address
> + * Returns -1 if the address is unusable
> + * 0 if it's OK.
> + */
> +static int qemuPCIAddressCheck(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED,
> +                               virDevicePCIAddressPtr addr)

How about naming this qemuPCIAddressValidate()? (This is especially good
since the verb "Check" is used elsewhere in this file to mean "check to
see if this is *in use*")

> +{
> +    if (addr->domain != 0) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Only PCI domain 0 is available"));
> +        return -1;
> +    }
> +    if (addr->bus != 0) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Only PCI bus 0 is available"));
> +        return -1;
> +    }
> +    if (addr->function >= QEMU_PCI_ADDRESS_LAST_FUNCTION) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Invalid PCI address: function must be < %u"),
> +                       QEMU_PCI_ADDRESS_LAST_FUNCTION);
> +        return -1;
> +    }
> +    if (addr->slot >= QEMU_PCI_ADDRESS_LAST_SLOT) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Invalid PCI address: slot must be < %u"),
> +                       QEMU_PCI_ADDRESS_LAST_SLOT);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +
>  static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
>  {
>      char *str;
>  
> -    if (addr->domain != 0 ||
> -        addr->bus != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Only PCI domain 0 and bus 0 are available"));
> -        return NULL;
> -    }
> -

Yes, definitely by the time we are converting this to a string it should
have already been validated.

>      if (virAsprintf(&str, "%d:%d:%d.%d",
>                      addr->domain,
>                      addr->bus,
> @@ -1222,7 +1248,8 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>                                   void *opaque)
>  {
>      int ret = -1;
> -    char *addr = NULL;
> +    char *str = NULL;
> +    virDevicePCIAddressPtr addr = &info->addr.pci;
>      qemuDomainPCIAddressSetPtr addrs = opaque;
>  
>      if ((info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
> @@ -1235,57 +1262,60 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>          return 0;
>      }
>  
> -    addr = qemuPCIAddressAsString(&info->addr.pci);
> -    if (!addr)
> +    if (qemuPCIAddressCheck(addrs, addr) < 0)
> +        return -1;
> +
> +    str = qemuPCIAddressAsString(addr);
> +    if (!str)
>          goto cleanup;

I prefer putting the assignment into the if condition:

     if (!(str = qemuPCIAddressAsString(addr)))
         goto cleanup;

>  
> -    if (virHashLookup(addrs->used, addr)) {
> +    if (virHashLookup(addrs->used, str)) {
>          if (info->addr.pci.function != 0) {
>              virReportError(VIR_ERR_XML_ERROR,
>                             _("Attempted double use of PCI Address '%s' "
>                               "(may need \"multifunction='on'\" for device on function 0)"),
> -                           addr);
> +                           str);
>          } else {
>              virReportError(VIR_ERR_XML_ERROR,
> -                           _("Attempted double use of PCI Address '%s'"), addr);
> +                           _("Attempted double use of PCI Address '%s'"), str);
>          }
>          goto cleanup;
>      }
>  
> -    VIR_DEBUG("Remembering PCI addr %s", addr);
> -    if (virHashAddEntry(addrs->used, addr, addr) < 0)
> +    VIR_DEBUG("Remembering PCI addr %s", str);
> +    if (virHashAddEntry(addrs->used, str, str) < 0)
>          goto cleanup;
> -    addr = NULL;
> +    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 = info->addr.pci;
> +        virDevicePCIAddress tmp_addr = *addr;
>          unsigned int *func = &tmp_addr.function;
>  
>          for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> -            addr = qemuPCIAddressAsString(&tmp_addr);
> -            if (!addr)
> +            str = qemuPCIAddressAsString(&tmp_addr);
> +            if (!str)
>                  goto cleanup;

Again, as long as you're modifying the lines, might as well stuff the
assignment into the if condition.


>  
> -            if (virHashLookup(addrs->used, addr)) {
> +            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)"),
> -                               addr);
> +                               str);
>                  goto cleanup;
>              }
>  
> -            VIR_DEBUG("Remembering PCI addr %s (multifunction=off for function 0)", addr);
> -            if (virHashAddEntry(addrs->used, addr, addr))
> +            VIR_DEBUG("Remembering PCI addr %s (multifunction=off for function 0)", str);
> +            if (virHashAddEntry(addrs->used, str, str))
>                  goto cleanup;
> -            addr = NULL;
> +            str = NULL;
>          }
>      }
>      ret = 0;
>  cleanup:
> -    VIR_FREE(addr);
> +    VIR_FREE(str);
>      return ret;
>  }
>  
> @@ -1385,6 +1415,9 @@ static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr addrs,


I just noticed that the (existing) comment for this function isn't
worded very well. As long as you're modifying things, could you fix that
too? (just s/the other/another/g)

Hmm, and now that I've suggested changing the name of
qemuPCIAddressCheck because of this function using the word "Check"
differently, I'm thinking *this* function could be better named as well.
How about qemuDomainPCIAddressInUse()? Also, I think it should return
true or false, not 0 or -1 (with associated adjustments in callers).

>      virDevicePCIAddress tmp_addr = *addr;
>      unsigned int *func = &(tmp_addr.function);
>  
> +    if (qemuPCIAddressCheck(addrs, addr) < 0)
> +        return -1;
> +

And as a matter of fact, I think you shouldn't be validating the PCI
address here - in two of the 3 callers, a fixed hard-coded pci address
is constructed (so you know that it's always valid), and in the 3rd
caller, it's being done inside a loop whose index self-limits the PCI
address to a valid range. (This is good, because if you left the call to
the validation in here, you would have to have a tri-state return value
to allow for failure as well as inuse/free).

>      for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
>          str = qemuPCIAddressAsString(&tmp_addr);
>          if (!str)
> @@ -1406,6 +1439,9 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
>  {
>      char *str;
>  
> +    if (qemuPCIAddressCheck(addrs, addr) < 0)
> +        return -1;
> +
>      str = qemuPCIAddressAsString(addr);
>      if (!str)
>          return -1;
> @@ -1479,6 +1515,9 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
>      char *str;
>      int ret;
>  
> +    if (qemuPCIAddressCheck(addrs, addr) < 0)
> +        return -1;
> +
>      str = qemuPCIAddressAsString(addr);
>      if (!str)
>          return -1;
> @@ -1498,6 +1537,9 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
>      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)
> @@ -1965,24 +2007,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
>                            virQEMUCapsPtr qemuCaps)
>  {
>      if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> -        if (info->addr.pci.domain != 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("Only PCI device addresses with domain=0 are supported"));
> -            return -1;
> -        }
> -        if (info->addr.pci.bus != 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("Only PCI device addresses with bus=0 are supported"));
> -            return -1;
> -        }
> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) {
> -            if (info->addr.pci.function > 7) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("The function of PCI device addresses must "
> -                                 "be less than 8"));
> -                return -1;
> -            }
> -        } else {
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) {
>              if (info->addr.pci.function != 0) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                                 _("Only PCI device addresses with function=0 "

Looks fine aside from the nits I listed above.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]