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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list