On 01/05/2015 02:29 AM, Wang Rui wrote:
> We can update device and attach device(cdrom) with the boot order unchanged.
> But the boot order check method will report an error:
> "boot order %d is already used by another device"
>
> In fact the boot order is used by itself, we just want to keep it as before.
> This patch improves boot order check to distinguish the owner.
>
> Signed-off-by: Wang Rui <moon.wangrui@xxxxxxxxxx>
> Signed-off-by: Zhou Yimin <zhouyimin@xxxxxxxxxx>
> ---
> src/conf/device_conf.c | 13 +++++++++++++
> src/conf/device_conf.h | 12 ++++++++++++
> src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++----
> src/conf/domain_conf.h | 9 ---------
> 4 files changed, 52 insertions(+), 13 deletions(-)
>
Probably could have split this in two in order to do code motion fi...
3a. -> Move virDomainDeviceDriveAddress from domain_conf.h to
device_conf.h which just seems to be separate from the... op
3b. -> Rest of this patch and you will also need to modify
src/libvirt_private.syms in order to add virDeviceDriveAddressEqual;
"properly" since it's global.
I also think that there's some synergies with patch 6/8 especially with
the matching logic
Perhaps an update to the "boot order" text in the <hostdev> section of
formatdomain.html.in would be beneficial to describe these additional
checks.
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index 5ffe159..907a7b8 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -144,6 +144,19 @@ virDevicePCIAddressEqual(virDevicePCIAddress *addr1,
> return false;
> }
>
> +bool
> +virDeviceDriveAddressEqual(virDomainDeviceDriveAddressPtr addr1,
> + virDomainDeviceDriveAddressPtr addr2)
> +{
> + if (addr1->controller == addr2->controller &&
> + addr1->bus == addr2->bus &&
> + addr1->target == addr2->target &&
> + addr1->unit == addr2->unit) {
> + return true;
> + }
> + return false;
> +}
> +
> int
> virInterfaceLinkParseXML(xmlNodePtr node,
> virInterfaceLinkPtr lnk)
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index f067a35..afcf0ee 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -55,6 +55,15 @@ struct _virDevicePCIAddress {
> int multi; /* enum virDomainDeviceAddressPCIMulti */
> };
>
> +typedef struct _virDomainDeviceDriveAddress virDomainDeviceDriveAddress;
> +typedef virDomainDeviceDriveAddress *virDomainDeviceDriveAddressPtr;
> +struct _virDomainDeviceDriveAddress {
> + unsigned int controller;
> + unsigned int bus;
> + unsigned int target;
> + unsigned int unit;
> +};
> +
> typedef struct _virInterfaceLink virInterfaceLink;
> typedef virInterfaceLink *virInterfaceLinkPtr;
> struct _virInterfaceLink {
> @@ -74,6 +83,9 @@ int virDevicePCIAddressFormat(virBufferPtr buf,
> bool virDevicePCIAddressEqual(virDevicePCIAddress *addr1,
> virDevicePCIAddress *addr2);
>
> +bool virDeviceDriveAddressEqual(virDomainDeviceDriveAddressPtr addr1,
> + virDomainDeviceDriveAddressPtr addr2);
> +
> int virInterfaceLinkParseXML(xmlNodePtr node,
> virInterfaceLinkPtr lnk);
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index aafc05e..d2c4a0a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19953,12 +19953,35 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
> virDomainDeviceInfoPtr newinfo = opaque;
>
> if (info->bootIndex == newinfo->bootIndex) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("boot order %d is already used by another device"),
> - newinfo->bootIndex);
> - return -1;
> + /* check if the same boot order is used by another device or itself */
> + if (info->type != newinfo->type) {
> + goto error;
> + } else {
> + switch ((virDomainDeviceAddressType) newinfo->type) {
> + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
> + if (virDevicePCIAddressEqual(&info->addr.pci,
> + &newinfo->addr.pci))
> + return 0;
> + else
> + goto error;
> + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
> + if (virDeviceDriveAddressEqual(&info->addr.drive,
> + &newinfo->addr.drive))
> + return 0;
> + else
> + goto error;
> + default:
> + goto error;
This is difficult to read with all the if-then-else and goto's.
How about the following:
if (info->bootIndex == newinfo->bootIndex) {
/* check if the same boot order is used by another device or
itself */
if (info->type != newinfo->type)
goto error;
if ((newinfo->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
virDevicePCIAddressEqual(&info->addr.pci,
&newinfo->addr.pci)) ||
(newinfo->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE &&
virDeviceDriveAddressEqual(&info->addr.drive,
&newinfo->addr.drive)))
return 0;
goto error;
}
return 0;
error:
...
Even with this change - I'm concerned about the cases where 'type'
doesn't match and is not PCI or DRIVE (e.g., the two new error paths).
Trying to wrap my mind about the timing of the check and when the 'type'
and address value are filled in w/r/t if they're not present in the XML
when the guest starts, but I think this is only called in an attach,
update, or detach path so it should be ok... Although I see
virDomainDefCompatibleDevice is called in an LXC path, so while I assume
you've made sure the qemu path works right, what about the lxc path as
it relates to the device info type/address checks?
Since patch 6 changes the Iterate from just 'attach' to 'attach' or
'update', it seems you're trying to "cover" the update case, but may
have caused unforeseen issues with the attach case.
I think my 3b and 6 should be combined to make it easier to consider the
options.
In particular, it seems new restrictions are in place that bootIndex can
only work for PCI and DRIVE virDomainDeviceInfo address types. I think
the USB type is throwing me off a bit.
Just trying to make sure some already existing option isn't being
disallowed with the new changes. Adding in specific device address
checks based on types just raises that concern.
John
> + }
> + }
> }
> return 0;
> +
> + error:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("boot order %d is already used by another device"),
> + newinfo->bootIndex);
> + return -1;
> }
>
> int
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 57297cd..3bddc80 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -236,15 +236,6 @@ typedef enum {
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST
> } virDomainDeviceAddressType;
>
> -typedef struct _virDomainDeviceDriveAddress virDomainDeviceDriveAddress;
> -typedef virDomainDeviceDriveAddress *virDomainDeviceDriveAddressPtr;
> -struct _virDomainDeviceDriveAddress {
> - unsigned int controller;
> - unsigned int bus;
> - unsigned int target;
> - unsigned int unit;
> -};
> -
> typedef struct _virDomainDeviceVirtioSerialAddress virDomainDeviceVirtioSerialAddress;
> typedef virDomainDeviceVirtioSerialAddress *virDomainDeviceVirtioSerialAddressPtr;
> struct _virDomainDeviceVirtioSerialAddress {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list