On Tue, 2016-05-17 at 14:24 -0400, Cole Robinson wrote:
>
> > + virDomainXMLOptionPtr xmlopt)
> > {
> > virDomainDiskDefPtr disk;
> > virDomainNetDefPtr net;
> > @@ -7803,11 +7805,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> > return -1;
> > /* vmdef has the pointer. Generic codes for vmdef will do all jobs
*/
> > dev->data.disk = NULL;
> > - if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
> > - if (virDomainDefAddImplicitDevices(vmdef) < 0)
> > - return -1;
>
> You removed the check on disk->bus here, and that concerns me a
> little. Can you please spend a few words explaining why this is
> safe?
I think the idea behind that check was 'adding a virtio disk doesn't need any
implied controller, but bus=scsi might, so only call AddImplicit for non-virtio'
However AddImplicit _must_ do the right thing here anyways, since for example
it is called every time we parse the XML, like reading it from disk on
libvirtd startup. So the check here was overly paranoid (but maybe it made
sense once)
Makes sense, thanks for the explanation.
ACK with the argument name changed.
--
Andrea Bolognani
Software Engineer - Virtualization Team