[libvirt] [PATCH v5 2/2] qemu: support to drop disk with 'optional' startupPolicy

[Martin Kletzander]

On 07/31/2013 09:51 AM, Guannan Ren wrote:
> Go through disks of guest, if one disk doesn't exist or its backing
> chain is broken, with 'optional' startupPolicy, for CDROM and Floppy
> we only discard its source path definition in xml, for disks we drop
> it from disk list and free it.
> ---
>  include/libvirt/libvirt.h.in |  1 +
>  src/qemu/qemu_domain.c       | 77 +++++++++++++++++++++++++++++++++++---------
>  2 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index c0eb25b..3888ba5 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -4728,6 +4728,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn,
>   */
>  typedef enum {
>      VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */
> +    VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START = 1,

s/HARDDISK/DISK/ to make it consistent

>  
>  #ifdef VIR_ENUM_SENTINELS
>      VIR_DOMAIN_EVENT_DISK_CHANGE_LAST
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 1ff802c..5da6e18 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2027,13 +2027,59 @@ cleanup:
>  }
>  
>  static int
> +qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver,
> +                                  virDomainObjPtr vm,
> +                                  virDomainDiskDefPtr disk,
> +                                  size_t *nextdisk)
> +{
> +    char uuid[VIR_UUID_STRING_BUFLEN];
> +    virDomainEventPtr event = NULL;
> +    virDomainDiskDefPtr del_disk = NULL;
> +
> +    virUUIDFormat(vm->def->uuid, uuid);
> +
> +    VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') "
> +              "due to inaccessible source '%s'",
> +              disk->dst, vm->def->name, uuid, disk->src);
> +
> +    if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
> +        disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> +
> +        event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
> +                                                   disk->info.alias,
> +                                                   VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
> +       /* For cdrom or floppy, we only remove its source definition
> +        * So the nextdisk need to point to next disk.
> +        */
> +        (*nextdisk)++;
> +        VIR_FREE(disk->src);
> +    } else {
> +        event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
> +                                                   disk->info.alias,
> +                                                   VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START);
> +
> +        if (!(del_disk = virDomainDiskRemoveByName(vm->def, disk->src))) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("no source device %s"), disk->src);
> +            return -1;
> +        }
> +        virDomainDiskDefFree(del_disk);
> +    }
> +
> +    if (event)
> +        qemuDomainEventQueue(driver, event);
> +
> +    return 0;
> +}
> +
> +static int
>  qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
>                                   virDomainObjPtr vm,
>                                   virDomainDiskDefPtr disk,
> -                                 bool cold_boot)
> +                                 bool cold_boot,
> +                                 size_t *nextdisk)
>  {
>      char uuid[VIR_UUID_STRING_BUFLEN];
> -    virDomainEventPtr event = NULL;
>      int startupPolicy = disk->startupPolicy;
>  
>      virUUIDFormat(vm->def->uuid, uuid);
> @@ -2057,16 +2103,8 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
>      }
>  
>      virResetLastError();

This should really be one function up, especially when I requested it as
a condition for ACK.

> -    VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') "
> -              "due to inaccessible source '%s'",
> -              disk->dst, vm->def->name, uuid, disk->src);
> -
> -    event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias,
> -                                               VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
> -    if (event)
> -        qemuDomainEventQueue(driver, event);
> -
> -    VIR_FREE(disk->src);
> +    if (qemuDomainCheckRemoveOptionalDisk(driver, vm, disk, nextdisk) < 0)
> +        goto error;
>  
>      return 0;
>  
> @@ -2082,21 +2120,28 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
>      int ret = -1;
>      size_t i;
>      virDomainDiskDefPtr disk;
> +    size_t count = vm->def->ndisks;
> +    size_t nextdisk = 0;
> +

Definitely drop all this nextdisk nonsense, you can clean it up with:

ssize_t i;
for(i = ndisks - 1; i >= 0; i--) ...

then you don't have to pass it to any underlying function, or you can
pass 'i' and call virDomainDiskRemove() instead of RemoveByName.

looking forward to v2 (or v6),
Martin