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