Re: [PATCH 06/11] hyperv: attach virtual disks when defining domains

> On Nov 26, 2020, at 9:48 AM, Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > On Tue, Nov 24, 2020 at 02:48:35PM -0500, Matt Coleman wrote: >> + g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit); > > Validate disk->info.type == DRIVE before accessing this field otherwise > the union contents is undefined. >> + if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) >> + return -1; >> + >> + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", >> + "Microsoft:Hyper-V:Virtual Hard Disk") < 0) >> + return -1; > > The disk->device can be disk, or cdrom or floppy. So if you're hadcoding disk, then > validate disk->device matches. The logic in hypervDomainAttachStorageVolume() ensures that it only calls hypervDomainAttachVirtualDisk() if disk->device is equal to VIR_DOMAIN_DISK_DEVICE_DISK. Should I add another check inside hypervDomainAttachVirtualDisk() or is it acceptable to validate within hypervDomainAttachStorageVolume() as long as no other functions call hypervDomainAttachVirtualDisk()?

If this is acceptable, I could also validate disk->info.type inside hypervDomainAttachStorageVolume(), which would cover the other two critiques. >> + VIR_DEBUG("Now attaching disk image '%s' with address %d to bus %d of type %d", >> + disk->src->path, disk->info.addr.drive.unit, disk->info.addr.drive.controller, disk->bus); > > Don't access the disk->info.addr until its type is validated