[libvirt] [PATCH 2/2] qemu: Unify generation of command line for virtio devices

[Andrea Bolognani]

On Tue, 2018-09-04 at 16:09 +0200, Ján Tomko wrote:
> On Fri, Aug 31, 2018 at 04:03:10PM +0200, Andrea Bolognani wrote:
> > +static char*
> > +qemuBuildVirtioDevStr(const virDomainDeviceInfo *info,
> > +                      const char *baseName)
[...]
> > +    virBufferAsprintf(&buf, "%s-%s", baseName, implName);
> 
> buf is used exactly once in this function, could have been just
> virAsprintf.
> 
> Or, even better, since all the calls are followed by adding the string
> to a buffer, just pass the buffer as the function argument.

I did it that way initially, but then I changed it to return
a char* to be consistent with other qemuBuild*DevStr(). I can
definitely change it back, but perhaps a different name would
be more appropriate at that point.

[...]
> > +        if (!(devStr = qemuBuildVirtioDevStr(&(disk->info), "virtio-blk")))
> 
> &disk->info is enough.
> 
> Or even simpler:
> disk->info.type

Okay, I'll go with the latter.

[...]
> > +        if (disk->iothread &&
> > +            (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW ||
> > +             disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
> > +            virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread);
> 
> The iothread change could be separated for an easier-to-read patch.

Agreed, it should have been that way to begin with.

> Also, formatting it unconditionally would be nicer - do we even need to
> check for the address type? We format it unconditionally for the
> virtio-scsi controller.

Not sure. I don't really know anything about iothreads,
so I purposefully steered clear of changing that part :)

[...]
> > +        virBufferAddStr(&buf, devStr);
> > +        VIR_FREE(devStr);
> > +    } else {
> > +        virBufferAddStr(&buf, nic);
> 
> virBufferAddStr(&buf, net->model);
> 
> Since we no longer use 'nic' unconditionally.

Okay.

[...]
> >     case VIR_DOMAIN_INPUT_TYPE_TABLET:
> > -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) ||
> > -            (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> > -             !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW))) {
> > -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                           _("virtio-tablet is not supported by this QEMU binary"));
> > -            goto error;
> > -        }
> 
> The capability check refactor is out of scope of this patch.
> Also, they should be in the *Validate functions, not the command line
> formatter.

Agreed, I'll split that part off.

[...]
> >     case VIR_DOMAIN_INPUT_TYPE_LAST:
> > -        break;
> > +    default:
> > +        virReportEnumRangeError(virDomainInputType,
> > +                                dev->type);
> > +        goto error;
> > +    }
> 
> Unrelated virReportEnumRangeError addition.

I'll make that a separate patch too.

[...]
> > +    if (dev->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> > +        virBufferAddLit(&buf, ",evdev=");
> > +        virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev);
> 
> Personally, I'd also separate all the changes that remove the additional
> attributes from the device name.

Agreed.

[...]
> > -    if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
> > -        virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s",
> > -                          dev->info.alias, dev->info.alias);
> > -    else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)
> > -        virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s",
> > -                          dev->info.alias, dev->info.alias);
> > -    else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO)
> > -        virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s",
> > -                          dev->info.alias, dev->info.alias);
> 
> Oh, fun. Not only we merged this copy & paste rng= attribute addition,
> it was later amended to add the id= attribute. Then broken into separate
> lines by a separate commit.

Well, at least now we're going to get rid of the duplication :)

-- 
Andrea Bolognani / Red Hat / Virtualization