[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 7/8] qemu: Introduce qemuBuildChrDeviceStr



On 06/06/2013 02:30 PM, Michal Privoznik wrote:
> The function being introduced is responsible for creating command
> line argument for '-device' for given character device. Based on
> the chardev type, it calls appropriate qemuBuild.*ChrDeviceStr(),
> e.g.  qemuBuildSerialChrDeviceStr() for serial chardev and so on.
> ---
>  src/qemu/qemu_command.c | 196 ++++++++++++++++++++++++++++++++++++++----------
>  src/qemu/qemu_command.h |  11 ++-
>  2 files changed, 160 insertions(+), 47 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 39b9d24..ec44b4f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
[...]
> @@ -7936,11 +7921,9 @@ qemuBuildCommandLine(virConnectPtr conn,
>              virCommandAddArg(cmd, devstr);
>              VIR_FREE(devstr);
>  
> -            virCommandAddArg(cmd, "-device");
> -            if (!(devstr = qemuBuildVirtioSerialPortDevStr(console,
> -                                                           qemuCaps)))
> +            if (qemuBuildChrDeviceStr(&devstr, def, console, qemuCaps) < 0)
>                  goto error;
> -            virCommandAddArg(cmd, devstr);
> +            virCommandAddArgList(cmd, "-device", devstr, NULL);
>              VIR_FREE(devstr);
>              break;
>  

There seems to be still a lot of code duplication.  Can you move the
'-device' creation out of the switch since you created such universal
function?

> @@ -8638,11 +8621,12 @@ error:
>  /* This function generates the correct '-device' string for character
>   * devices of each architecture.
>   */
> -char *
> -qemuBuildChrDeviceStr(virDomainChrDefPtr serial,
> -                      virQEMUCapsPtr qemuCaps,
> -                      virArch arch,
> -                      char *machine)
> +static int
> +qemuBuildSerialChrDeviceStr(char **deviceStr,
> +                            virDomainChrDefPtr serial,
> +                            virQEMUCapsPtr qemuCaps,
> +                            virArch arch,
> +                            char *machine)
>  {
>      virBuffer cmd = VIR_BUFFER_INITIALIZER;
>  
> @@ -8674,7 +8658,7 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial,
>              }
>  
>              if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0)
> -               goto error;
> +                goto error;
>          }
>      }
>  
> @@ -8683,13 +8667,143 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial,
>          goto error;
>      }
>  
> -    return virBufferContentAndReset(&cmd);
> +    *deviceStr = virBufferContentAndReset(&cmd);
> +    return 0;
>  
> - error:
> +error:

We should unify this and write it in the HACKING file, but something
tells me we won't reach easy agreement.  Would anyone be against a rule
for labels having one space in front of them?  git doesn't use it after
function name when there's a space (plus it is the default indentation
in my editor for labels).

>      virBufferFreeAndReset(&cmd);
> -    return NULL;
> +    return -1;
>  }
>  
> +static int
> +qemuBuildParallelChrDeviceStr(char **deviceStr,
> +                              virDomainChrDefPtr chr)
> +{
> +    if (virAsprintf(deviceStr, "isa-parallel,chardev=char%s,id=%s",
> +                    chr->info.alias, chr->info.alias) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +static int
> +qemuBuildChannelChrDeviceStr(char **deviceStr,
> +                             virDomainChrDefPtr chr,
> +                             virQEMUCapsPtr qemuCaps)
> +{
> +    int ret = -1;
> +    char *addr = NULL;
> +    int port;
> +
> +    switch ((enum virDomainChrChannelTargetType) chr->targetType) {
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD:
> +
> +        addr = virSocketAddrFormat(chr->target.addr);
> +        if (!addr)
> +            return ret;
> +        port = virSocketAddrGetPort(chr->target.addr);
> +
> +        if (virAsprintf(deviceStr,
> +                        "user,guestfwd=tcp:%s:%i,chardev=char%s,id=user-%s",
> +                        addr, port, chr->info.alias, chr->info.alias) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +        break;
> +
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
> +        if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps)))
> +                goto cleanup;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE:
> +    case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST:
> +    default:

Delete the default case and let the compiler do the check for missing
target types.

It's pre-existing, but I guess we could get rid of those _NONE types
since those are not used anywhere, couldn't we?

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unsupported channel target type %d"),
> +                       chr->targetType);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(addr);
> +    return ret;
> +}
> +
> +static int
> +qemuBuildConsoleChrDeviceStr(char **deviceStr,
> +                             virDomainChrDefPtr chr,
> +                             virQEMUCapsPtr qemuCaps)
> +{
> +    int ret = -1;
> +
> +    switch (chr->targetType) {
> +    case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP:
> +    case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM:
> +        if (!(*deviceStr = qemuBuildSclpDevStr(chr)))
> +            goto cleanup;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
> +        if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps)))
> +            goto cleanup;
> +        break;
> +
> +    case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
> +        break;
> +
> +    default:

Cast the targetType to the appropriate enum and change this to _LAST.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unsupported console target type %d"),
> +                       chr->targetType);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
> +int
> +qemuBuildChrDeviceStr(char **deviceStr,
> +                      virDomainDefPtr vmdef,
> +                      virDomainChrDefPtr chr,
> +                      virQEMUCapsPtr qemuCaps)
> +{
> +    int ret = -1;
> +
> +    switch ((enum virDomainChrDeviceType) chr->deviceType) {
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
> +        ret = qemuBuildSerialChrDeviceStr(deviceStr, chr, qemuCaps,
> +                                          vmdef->os.arch,
> +                                          vmdef->os.machine);
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
> +        ret = qemuBuildParallelChrDeviceStr(deviceStr, chr);
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
> +        ret = qemuBuildChannelChrDeviceStr(deviceStr, chr, qemuCaps);
> +        break;
> +
> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> +        ret = qemuBuildConsoleChrDeviceStr(deviceStr, chr, qemuCaps);
> +        break;
> +
> +    default:

s/default/VIR_DOMAIN_CHR_DEVICE_TYPE_LAST/

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Chardev deviceType %d is not handled"),

I'd reword this to the usual 'unsupported device type %d'.

> +                       chr->deviceType);
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +

Unnecessary newline.

>  /*
>   * This method takes a string representing a QEMU command line ARGV set
>   * optionally prefixed by a list of environment variables. It then tries
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 900efd7..c925ebf 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -75,12 +75,11 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn,
>                                     qemuBuildCommandLineCallbacksPtr callbacks)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11);
>  
> -/* Generate string for arch-specific '-device' parameter */
> -char *
> -qemuBuildChrDeviceStr (virDomainChrDefPtr serial,
> -                       virQEMUCapsPtr qemuCaps,
> -                       virArch arch,
> -                       char *machine);
> +/* Generate '-device' string for chardev device */
> +int qemuBuildChrDeviceStr(char **deviceStr,

s/ /\n/

> +                          virDomainDefPtr vmdef,
> +                          virDomainChrDefPtr chr,
> +                          virQEMUCapsPtr qemuCaps);
>  
>  /* With vlan == -1, use netdev syntax, else old hostnet */
>  char * qemuBuildHostNetStr(virDomainNetDefPtr net,
> 

ACK with mentioned changes fixed (I don't care about the label of course).

Martin


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]