Peter Krempa wrote:
+ virBufferAsprintf(&opt, + ",%s=%s", + channel->target.name, + channel->source->data.nix.path); + }
Doesn't bhyve, similarly to qemu need to escape any occurence of ',' in the fiule name?
+ + if (virBufferUse(&opt) > 0) { + virCommandAddArg(cmd, "-s"); + virCommandAddArgFormat(cmd, "%d:0,virtio-console%s", + controller->info.addr.pci.slot, + virBufferContentAndReset(&opt));
This results in:
+-s 2:0,virtio-console,org.qemu.guest_agent.0=/var/run/libvirt/bhyve/bhyve.agent,org.qemu.guest_agent.1=/var/run/libvirt/bhyve/bhyve.agent-2 \
Since the paths are passed from the XML and AFAIU not actually validated to be devoid of ',' chars, not escaping them will break the output if the path contains a ','.
Very good catch, ',' needs to be escaped. I think not only in path, but in the port name too.
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 1d436da609..711d1b66ea 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -473,6 +473,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, virDomainShutoffReason reason) { + size_t i = 0; int ret = -1; g_autoptr(virCommand) cmd = NULL; bhyveDomainObjPrivate *priv = vm->privateData; @@ -512,6 +513,19 @@ virBhyveProcessStop(struct _bhyveConn *driver, } }
+ /* UNIX sockets cleanup */ + for (i = 0; i < vm->def->nchannels; i++) { + virDomainChrDef *channel = vm->def->channels[i]; + + if (channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX) + continue; + if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) + continue; + + if (virFileExists(channel->source->data.nix.path)) + virFileRemove(channel->source->data.nix.path, 0, 0);
Not a problem of this patch but the 3 code paths at the beginning of this function skip the rest of the cleanup so some resources such as allocated ports could stay around if the function fails.
That's also a very good one. I thought to address that in this patch (to be series), but it's actually deeper than I anticipated. Basically, making virBhyveProcessBuildDestroyCmd() a not critical failure and continuing with cleaning up uncovers a race between the main shutdown code and the one initiated by the monitor which results in nasty things like unreferring the monitor object twice. That is too big to address as a part of this change, so I'll fix that separately.