[libvirt] [PATCH v3 6/9] qemu_command: Honour chardev alias assignment with a function

[Daniel P. Berrange]

On Tue, Jul 02, 2013 at 05:53:05PM +0200, Michal Privoznik wrote:
> The chardev alias assignment is going to be needed in a separate
> places, so it should be moved into a separate function rather
> than copying code randomly around.
> ---
>  src/qemu/qemu_command.c | 75 +++++++++++++++++++++++++++++++++++++++++++------
>  src/qemu/qemu_command.h |  3 ++
>  2 files changed, 70 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ba93233..903839f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
>      return 0;
>  }
>  
> +int
> +qemuAssignDeviceChrAlias(virDomainDefPtr def,
> +                         virDomainChrDefPtr chr,
> +                         ssize_t idx)
> +{
> +    const char *prefix = NULL;
> +    const char *prefix2 = NULL;
> +
> +    switch ((enum virDomainChrDeviceType) chr->deviceType) {
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
> +        prefix = "parallel";
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
> +        prefix = "serial";
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> +        prefix = "console";
> +        prefix2 = "serial";
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
> +        prefix = "channel";
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
> +        return -1;
> +    }
> +
> +    if (idx == -1) {
> +        virDomainChrDefPtr **arrPtr;
> +        size_t *cntPtr;
> +        size_t i;
> +        idx = 0;
> +
> +        virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr);
> +
> +        for (i = 0; i < *cntPtr; i++) {
> +            int thisidx;
> +            if (((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 0) &&
> +                (prefix2 &&
> +                 (thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix2)) < 0)) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("Unable to determine device index for character device"));
> +                return -1;
> +            }
> +            if (thisidx >= idx)
> +                idx = thisidx + 1;
> +        }
> +    }

The commit message describes this as a simple refactoring, but this
if (idx== -1) {...}  is all new functionality compared to what is
being replaced. I'm not too sure that this logic is correct
either when dealing with <console> with a 'serialXX' alias.

It kind of feels like this if() block should be a separate function

 "qemuGetNextChrDevIndex"

or something like that.

> +
> +    if (virAsprintf(&chr->info.alias, "%s%zd", prefix, idx) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    return 0;
> +}
>  
>  int
>  qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> @@ -947,20 +1006,20 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>              goto no_memory;
>      }
>      for (i = 0; i < def->nparallels; i++) {
> -        if (virAsprintf(&def->parallels[i]->info.alias, "parallel%d", i) < 0)
> -            goto no_memory;
> +        if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0)
> +            return -1;
>      }
>      for (i = 0; i < def->nserials; i++) {
> -        if (virAsprintf(&def->serials[i]->info.alias, "serial%d", i) < 0)
> -            goto no_memory;
> +        if (qemuAssignDeviceChrAlias(def, def->serials[i], i) < 0)
> +            return -1;
>      }
>      for (i = 0; i < def->nchannels; i++) {
> -        if (virAsprintf(&def->channels[i]->info.alias, "channel%d", i) < 0)
> -            goto no_memory;
> +        if (qemuAssignDeviceChrAlias(def, def->channels[i], i) < 0)
> +            return -1;
>      }
>      for (i = 0; i < def->nconsoles; i++) {
> -        if (virAsprintf(&def->consoles[i]->info.alias, "console%d", i) < 0)
> -            goto no_memory;
> +        if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0)
> +            return -1;
>      }
>      for (i = 0; i < def->nhubs; i++) {
>          if (virAsprintf(&def->hubs[i]->info.alias, "hub%d", i) < 0)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 2993448..e92c78a 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -260,6 +260,9 @@ int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
>  int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev, int idx);
>  int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller);
>  int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redirdev, int idx);
> +int qemuAssignDeviceChrAlias(virDomainDefPtr def,
> +                             virDomainChrDefPtr chr,
> +                             ssize_t idx);
>  
>  int
>  qemuParseKeywords(const char *str,


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|