Re: [libvirt] [PATCHv4 3/5] S390: QEMU driver support for CCW addresses

On Tue, Mar 05, 2013 at 04:44:21PM +0100, Viktor Mihajlovski wrote: > This commit adds the QEMU driver support for CCW addresses. The > current QEMU only allows virtio devices to be attached to the > CCW bus. We named the new capability indicating that support > QEMU_CAPS_VIRTIO_CCW accordingly. > > The fact that CCW devices can only be assigned to domains with a > machine type of s390-ccw-virtio requires a few extra checks for > machine type in qemu_command.c on top of querying > QEMU_CAPS_VIRTIO_{CCW|S390}. >

> +struct _qemuDomainCCWAddressSet { > + virHashTablePtr defined; Too much whitespace ^^^^^^^^^^^^^^^^

> + virDomainDeviceCCWAddress next; > +}; > + > +static char* > +qemuCCWAddressAsString(virDomainDeviceCCWAddressPtr addr) > +{ > + char *addrstr = NULL; > + > + if (virAsprintf(&addrstr, "%x.%x.%04x", Should we zero-pad the first two fields too, or is it common to only pad the last field in ccw addresses ?

> -static void > -qemuDomainAssignS390Addresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) > +static int > +qemuDomainCCWAddressAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED, > + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, > + virDomainDeviceInfoPtr info, > + void * data) > +{ > + return qemuDomainCCWAddressAssign(info, > + (qemuDomainCCWAddressSetPtr)data, You don't need to cast 'void *' in C.

ACK, since there's no bugs in my comments, just style issues.