Re: [libvirt] [PATCH 1/2] conf: Detect misconfiguration between disk bus and disk address
On Wed, Nov 16, 2016 at 09:09 AM +0100, Peter Krempa <pkrempa(a)redhat.com> wrote:
> +{
> + if (addressType == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> + return true;
> +
> + switch (bus) {
> + case VIR_DOMAIN_DISK_BUS_SATA:
> + case VIR_DOMAIN_DISK_BUS_SCSI:
> + case VIR_DOMAIN_DISK_BUS_FDC:
> + case VIR_DOMAIN_DISK_BUS_IDE:
> + return addressType == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
> + default:
We tend to use a full enumeration of the types rather than the default
case along with a typecast of the switched variable to the correct type
so that the compiler checks if a new enum value is added.
Okay, I will change it.
Just a small question (maybe I should create another thread for it):
What's the reason why we're using 'int' and not directly the enum for
e.g. the field 'bus' in the 'struct t _virDomainDiskDef'?
<code>
...
int bus; /* enum virDomainDiskBus */
...
</code>
> static int
> virDomainDiskDefValidate(const virDomainDiskDef *disk)
> {
> @@ -4681,6 +4713,20 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
> }
> }
>
> + /* Reject disks with a bus type that is not compatible with the
> + * given address type. The function considers only buses that are
> + * handled in common code. For other bus types it's not possible
> + * to decide compatibility in common code.
> + */
This comment is kind of redundant with the comment of the function.
Yep - but I've added it for clarity because someone could think (if he
looks only at the function name) the function decides the compatibility for
EVERY bus type.
But if you want to I will remove it.
Anyway, thanks for comment.
Marc