[libvirt] [PATCH v2 1/8] domain_conf: Introduce chardev hotplug helpers

[Martin Kletzander]

On 06/06/2013 02:29 PM, Michal Privoznik wrote:
> For now, only these three helpers are needed:
> virDomainChrFind - to find a duplicate chardev within VM def
> virDomainChrInsert - wrapper for inserting a new chardev into VM def
> virDomainChrRemove - wrapper for removing chardev from VM def
> 
> There is, however, one internal helper as well:
> virDomainChrGetDomainPtrs which sets given pointers to one of
> vmdef->{parallels,serials,consoles,channels} based on passed
> chardev type.
> ---
>  src/conf/domain_conf.c   | 160 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |  11 ++++
>  src/libvirt_private.syms |   4 ++
>  3 files changed, 175 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a16ebd1..37f0ce9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10056,6 +10056,166 @@ virDomainLeaseRemove(virDomainDefPtr def,
>      return virDomainLeaseRemoveAt(def, i);
>  }
>  
> +static bool
> +virDomainChrEquals(virDomainChrDefPtr src,
> +                   virDomainChrDefPtr tgt)
> +{
> +    if (!src || !tgt)
> +        return src == tgt;
> +
> +    if (!virDomainChrSourceDefIsEqual(&src->source, &tgt->source))
> +        return false;
> +
> +    if (src->targetTypeAttr != tgt->targetTypeAttr)
> +        return false;
> +
> +    if (!src->targetTypeAttr)
> +        return true;
> +
> +    switch ((enum virDomainChrChannelTargetType) src->targetType) {

There should be one more check and switch above this one to
differentiate according to src->deviceType and tgt->deviceType.

> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
> +        return STREQ_NULLABLE(src->target.name, tgt->target.name);
> +        break;
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD:
> +        if (!src->target.addr || !tgt->target.addr)
> +            return src->target.addr == tgt->target.addr;
> +        return memcmp(src->target.addr, tgt->target.addr,
> +                      sizeof(*src->target.addr)) == 0;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE:
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST:
> +    default:


No need for default case in here.

> +        return true;
> +    }
> +}
> +
> +virDomainChrDefPtr
> +virDomainChrFind(virDomainDefPtr def,
> +                 virDomainChrDefPtr target)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nparallels; i++) {
> +        virDomainChrDefPtr chr = def->parallels[i];
> +
> +        if (virDomainChrEquals(chr, target))
> +            return chr;
> +    }
> +
> +    for (i = 0; i < def->nserials; i++) {
> +        virDomainChrDefPtr chr = def->serials[i];
> +
> +        if (virDomainChrEquals(chr, target))
> +            return chr;
> +    }
> +
> +
> +    for (i = 0; i < def->nconsoles; i++) {
> +        virDomainChrDefPtr chr = def->consoles[i];
> +
> +        if (virDomainChrEquals(chr, target))
> +            return chr;
> +    }
> +
> +    for (i = 0; i < def->nchannels; i++) {
> +        virDomainChrDefPtr chr = def->channels[i];
> +
> +        if (virDomainChrEquals(chr, target))
> +            return chr;
> +    }
> +

You're iterating over all devices in the domain, but you can check which
ones to check based on target->deviceType.

> +    return NULL;
> +}
> +
> +int
> +virDomainChrGetDomainPtrs(virDomainDefPtr vmdef,
> +                          virDomainChrDefPtr chr,
> +                          virDomainChrDefPtr ***arrPtr,
> +                          size_t **cntPtr)
> +{
> +    switch ((enum virDomainChrDeviceType) chr->deviceType) {
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
> +        *arrPtr = &vmdef->parallels;
> +        *cntPtr = &vmdef->nparallels;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
> +        *arrPtr = &vmdef->serials;
> +        *cntPtr = &vmdef->nserials;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> +        *arrPtr = &vmdef->consoles;
> +        *cntPtr = &vmdef->nconsoles;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
> +        *arrPtr = &vmdef->channels;
> +        *cntPtr = &vmdef->nchannels;
> +        break;
> +
> +    default:

I'd rather cast the switch argument, put a TYPE_LAST here and you don't
have to report the error then.

> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("Unsupported device type '%d'"),
> +                       chr->deviceType);
> +        return -1;
> +    }
> +    return 0;
> +}
> +

And with this function, you can iterate over those devices in
virDomainChrFind even more clearly.

> +int virDomainChrInsert(virDomainDefPtr vmdef,
> +                       virDomainChrDefPtr chr)
> +{
> +    virDomainChrDefPtr **arrPtr;
> +    size_t *cntPtr;
> +
> +    if (virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr) < 0)
> +        return -1;
> +
> +    if (VIR_REALLOC_N(*arrPtr, *cntPtr + 1) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    (*arrPtr)[*cntPtr] = chr;
> +    (*cntPtr)++;
> +

VIR_APPEND_ELEMENT could make this a bit shorter and readable.

> +    return 0;
> +}
> +
> +int virDomainChrRemove(virDomainDefPtr vmdef,
> +                       virDomainChrDefPtr chr)
> +{
> +    virDomainChrDefPtr **arrPtr;
> +    size_t i, *cntPtr;
> +
> +    if (virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr) < 0)
> +        return -1;
> +
> +    for (i = 0; i < *cntPtr; i++) {
> +        virDomainChrDefPtr tmp = (*arrPtr)[i];
> +
> +        if (virDomainChrEquals(tmp, chr))
> +            break;
> +    }
> +
> +    if (i == *cntPtr)
> +        return -1;
> +

This is another part of code you can de-duplicate since it is similar to
the virDomainChrFind (and deals with the problem mentioned there).

> +    virDomainChrDefFree((*arrPtr)[i]);
> +    if (*cntPtr > 1) {
> +        memmove(*arrPtr + i,
> +                *arrPtr + i + 1,
> +                sizeof(**arrPtr) * (*cntPtr - (i + 1)));
> +        ignore_value(VIR_REALLOC_N(*arrPtr, *cntPtr - 1));
> +    } else {
> +        VIR_FREE(*arrPtr);
> +    }
> +    (*cntPtr)--;
> +

And for this you should be able to use the VIR_DELETE_ELEMENT macro.

> +    return 0;
> +}
>  
>  char *
>  virDomainDefGetDefaultEmulator(virDomainDefPtr def,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3a71d6c..6d650ff 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2355,6 +2355,17 @@ virDomainLeaseDefPtr
>  virDomainLeaseRemove(virDomainDefPtr def,
>                       virDomainLeaseDefPtr lease);
>  
> +int virDomainChrGetDomainPtrs(virDomainDefPtr vmdef,

s/ /\n/

> +                              virDomainChrDefPtr chr,
> +                              virDomainChrDefPtr ***arrPtr,
> +                              size_t **cntPtr);
> +virDomainChrDefPtr virDomainChrFind(virDomainDefPtr def,

dtto

> +                                    virDomainChrDefPtr target);
> +int virDomainChrInsert(virDomainDefPtr vmdef,

dtto

> +                       virDomainChrDefPtr chr);
> +int virDomainChrRemove(virDomainDefPtr vmdef,

dtto

> +                       virDomainChrDefPtr chr);
> +
>  int virDomainSaveXML(const char *configDir,

pre-existing, but could be fixed.

>                       virDomainDefPtr def,
>                       const char *xml);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b93629f..b9ba731 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -77,6 +77,10 @@ virDomainChrConsoleTargetTypeToString;
>  virDomainChrDefForeach;
>  virDomainChrDefFree;
>  virDomainChrDefNew;
> +virDomainChrFind;
> +virDomainChrGetDomainPtrs;
> +virDomainChrInsert;
> +virDomainChrRemove;
>  virDomainChrSerialTargetTypeFromString;
>  virDomainChrSerialTargetTypeToString;
>  virDomainChrSourceDefCopy;
> 

Martin