[libvirt] [PATCH 6/8] conf: add compatiblity check for boot index when updating device

[John Ferlan]

On 01/05/2015 02:29 AM, Wang Rui wrote:
> Signed-off-by: Wang Rui <moon.wangrui at huawei.com>
> Signed-off-by: Zhou Yimin <zhouyimin at huawei.com>
> ---
>  src/conf/domain_conf.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 

Since there's no commit message - even more reason to combine with patch
3 (or my 3b)


> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d2c4a0a..5beb830 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19991,29 +19991,32 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>  {
>      virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
>  
> -    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH)
> -        return 0;
> -
> -    if (!virDomainDefHasUSB(def) &&
> -        STRNEQ(def->os.type, "exe") &&
> -        virDomainDeviceIsUSB(dev)) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("Device configuration is not compatible: "
> -                         "Domain has no USB bus support"));
> -        return -1;
> -    }
> -
> -    if (info && info->bootIndex > 0) {
> -        if (def->os.nBootDevs > 0) {
> +    switch (action) {
> +    case VIR_DOMAIN_DEVICE_ACTION_ATTACH:
> +        if (!virDomainDefHasUSB(def) &&
> +            STRNEQ(def->os.type, "exe") &&
> +            virDomainDeviceIsUSB(dev)) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("per-device boot elements cannot be used"
> -                             " together with os/boot elements"));
> +                           _("Device configuration is not compatible: "
> +                             "Domain has no USB bus support"));
>              return -1;
>          }
> -        if (virDomainDeviceInfoIterate(def,
> -                                       virDomainDeviceInfoCheckBootIndex,
> -                                       info) < 0)
> -            return -1;
> +    case VIR_DOMAIN_DEVICE_ACTION_UPDATE:
> +        if (info && info->bootIndex > 0) {
> +            if (def->os.nBootDevs > 0) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("per-device boot elements cannot be used"
> +                                 " together with os/boot elements"));
> +                return -1;
> +            }
> +            if (virDomainDeviceInfoIterate(def,
> +                                           virDomainDeviceInfoCheckBootIndex,
> +                                           info) < 0)
> +                return -1;
> +        }
> +        break;
> +    default:
> +        return 0;

The 'norm' of using default is to list all the cases - makes it easier
to determine when one is missing.  I think the switch in this case is
overkill.


I think this is better:

    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH ||
        action != VIR_DOMAIN_DEVICE_ACTION_UPDATE)
        return 0;

    if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
        !virDomainDefHasUSB(def) &&
        STRNEQ(def->os.type, "exe") &&
        virDomainDeviceIsUSB(dev)) {
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                       _("Device configuration is not compatible: "
                         "Domain has no USB bus support"));
        return -1;
    }

     if (info && info->bootIndex > 0) {
         if (def->os.nBootDevs > 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("per-device boot elements cannot be used"
                              " together with os/boot elements"));
             return -1;
         }
         if (virDomainDeviceInfoIterate(def,
                                        virDomainDeviceInfoCheckBootIndex,
                                        info) < 0)
             return -1;
     }


[sorry about the wrapping on the CheckBootIndex call - the point is all
you're doing is adding _UPDATE to the initial check... then adding the
"if (action == *_ATTACH)" for just the if USB check...


>      }
>  
>      return 0;
>