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