Re: [PATCH 3/9] Store USB port path as an array of integers

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

 




On 08/12/2015 10:52 AM, Ján Tomko wrote:
> In preparation to tracking which USB addresses are occupied.
> Introduce two helper functions for printing the port path
> as a string and appending it to a virBuffer.
> ---
>  src/conf/domain_addr.c   | 25 +++++++++++++++++++++++++
>  src/conf/domain_addr.h   |  8 ++++++++
>  src/conf/domain_conf.c   | 29 +++++++++++++++--------------
>  src/conf/domain_conf.h   |  4 +++-
>  src/libvirt_private.syms |  2 ++
>  src/qemu/qemu_command.c  |  3 ++-
>  6 files changed, 55 insertions(+), 16 deletions(-)
> 


Hmm.. interesting we don't have any tests using dotted notation port
values.  Although I did try and find it was successful on the xml parse
and print, it may be "useful" to create a test that uses 2, 3, and 4 dot
octets... and 5 for a test failure.

In order to test I modified
tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter.xml and
tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-redir-filter.xml to replace
a port address with a 4 dot octet.

> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 9883c4f..a5d142d 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1173,3 +1173,28 @@ virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs,
>      VIR_FREE(str);
>      return ret;
>  }
> +
> +
> +void
> +virDomainUSBAddressGetPortBuf(virBufferPtr buf,
> +                              unsigned int *port)

Perhaps virDomainUSBAddressPrintPortBuf ??

> +{
> +    size_t i;
> +
> +    for (i = 0; i < VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH; i++) {
> +        if (port[i] == 0)
> +            break;
> +        virBufferAsprintf(buf, "%u.", port[i]);
> +    }
> +    virBufferTrim(buf, ".", -1);
> +}
> +
> +char *
> +virDomainUSBAddressGetPortString(unsigned int *port)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virDomainUSBAddressGetPortBuf(&buf, port);
> +    if (virBufferCheckError(&buf) < 0)
> +        return NULL;

Peeking ahead to callers (none here, but future patches) - there needs
to be either NULLSTR(portstr) for all or a decision to fail if NULL is
returned.

> +    return virBufferContentAndReset(&buf);
> +}
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 208635c..c6d8da7 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -240,4 +240,12 @@ virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs,
>                                   virDomainDeviceInfoPtr info)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> +void
> +virDomainUSBAddressGetPortBuf(virBufferPtr buf,
> +                              unsigned int *port)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +char *
> +virDomainUSBAddressGetPortString(unsigned int *port)
> +    ATTRIBUTE_NONNULL(1);
> +
>  #endif /* __DOMAIN_ADDR_H__ */
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f1e02e3..0526aee 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -33,6 +33,7 @@
>  #include "internal.h"
>  #include "virerror.h"
>  #include "datatypes.h"
> +#include "domain_addr.h"
>  #include "domain_conf.h"
>  #include "snapshot_conf.h"
>  #include "viralloc.h"
> @@ -3345,8 +3346,6 @@ virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
>  void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
>  {
>      VIR_FREE(info->alias);
> -    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> -        VIR_FREE(info->addr.usb.port);
>      memset(&info->addr, 0, sizeof(info->addr));
>      info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>      VIR_FREE(info->romfile);
> @@ -4329,9 +4328,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>          break;
>  
>      case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
> -        virBufferAsprintf(buf, " bus='%d' port='%s'",
> -                          info->addr.usb.bus,
> -                          info->addr.usb.port);
> +        virBufferAsprintf(buf, " bus='%d' port='",
> +                          info->addr.usb.bus);
> +        virDomainUSBAddressGetPortBuf(buf, info->addr.usb.port);
> +        virBufferAddLit(buf, "'");
>          break;
>  
>      case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
> @@ -4567,27 +4567,28 @@ virDomainDeviceUSBAddressParseXML(xmlNodePtr node,
>                                    virDomainDeviceUSBAddressPtr addr)
>  {
>      char *port, *bus, *tmp;
> -    unsigned int p;
>      int ret = -1;
> +    size_t i;
>  
>      memset(addr, 0, sizeof(*addr));
>  
>      port = virXMLPropString(node, "port");
>      bus = virXMLPropString(node, "bus");
>  
> -    if (port &&
> -        ((virStrToLong_uip(port, &tmp, 10, &p) < 0 || (*tmp != '\0' && *tmp != '.')) ||
> -         (*tmp == '.' && (virStrToLong_ui(tmp + 1, &tmp, 10, &p) < 0 || (*tmp != '\0' && *tmp != '.'))) ||
> -         (*tmp == '.' && (virStrToLong_ui(tmp + 1, &tmp, 10, &p) < 0 || (*tmp != '\0' && *tmp != '.'))) ||
> -         (*tmp == '.' && (virStrToLong_ui(tmp + 1, &tmp, 10, &p) < 0 || (*tmp != '\0'))))) {
> +    for (i = 0, tmp = port;
> +         tmp && *tmp != '\0' && i < VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH;
> +         i++) {
> +        if (virStrToLong_uip(tmp, &tmp, 10, &addr->port[i]) < 0)
> +            break;
> +        if (*tmp == '.')
> +            tmp++;
> +    }

My eyes hurt (from both algorithms), but I can see from testing this
works, although I think VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH is wrong
since 1.2.3.4.5 parsed successfully.

> +    if (tmp && *tmp != '\0') {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Cannot parse <address> 'port' attribute"));
>          goto cleanup;
>      }
>  
> -    addr->port = port;
> -    port = NULL;
> -
>      if (bus &&
>          virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 9762c4f..abd2cdb 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -294,11 +294,13 @@ struct _virDomainDeviceCcidAddress {
>      unsigned int slot;
>  };
>  
> +# define VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH 7
> +

7?  This is for the 4 octet usb port addr value, correct?

that is (from our docs)

type='usb'
    USB addresses have the following additional attributes: bus (a hex
value between 0 and 0xfff, inclusive), and port (a dotted notation of up
to four octets, such as 1.2 or 2.1.3.1).

so shouldn't this be 4? Or is that changing too?

With a few things cleaned up this is ACK-able it seems - would be nice
to have a test with the multi-octet port address even though it's not
'required'...

John
>  typedef struct _virDomainDeviceUSBAddress virDomainDeviceUSBAddress;
>  typedef virDomainDeviceUSBAddress *virDomainDeviceUSBAddressPtr;
>  struct _virDomainDeviceUSBAddress {
>      unsigned int bus;
> -    char *port;
> +    unsigned int port[VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH];
>  };
>  
>  typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index f1e5f48..5168230 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -106,6 +106,8 @@ virDomainPCIAddressSetFree;
>  virDomainPCIAddressSetGrow;
>  virDomainPCIAddressSlotInUse;
>  virDomainPCIAddressValidate;
> +virDomainUSBAddressGetPortBuf;
> +virDomainUSBAddressGetPortString;
>  virDomainVirtioSerialAddrAssign;
>  virDomainVirtioSerialAddrAutoAssign;
>  virDomainVirtioSerialAddrIsComplete;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 84cbfe1..4e77279 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2796,7 +2796,8 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
>                                                         VIR_DOMAIN_CONTROLLER_TYPE_USB,
>                                                         info->addr.usb.bus)))
>              goto cleanup;
> -        virBufferAsprintf(buf, ",bus=%s.0,port=%s", contAlias, info->addr.usb.port);
> +        virBufferAsprintf(buf, ",bus=%s.0,port=", contAlias);
> +        virDomainUSBAddressGetPortBuf(buf, info->addr.usb.port);
>      } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
>          if (info->addr.spaprvio.has_reg)
>              virBufferAsprintf(buf, ",reg=0x%llx", info->addr.spaprvio.reg);
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]