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