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

[Daniel P. Berrangé]

On Fri, Jan 08, 2021 at 05:55:55PM -0500, Matt Coleman wrote:
> > On Nov 26, 2020, at 9:48 AM, Daniel P. Berrangé <berrange at redhat.com> 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()?

As long as its validated somewhere in the flow that's ok.

Centralizing validation in the virDomainDefValidateCallback could be
something to consider in future. This does validation at the time the
XML is parsed, so you can keep the later logic cleaner. 

> 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
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|