Alexander Nusov wrote:
> This patch adds support for automatic VNC port assignment for bhyve guests.
>
> ---
> src/bhyve/bhyve_command.c | 9 +++++++++
> src/bhyve/bhyve_driver.c | 5 +++++
> src/bhyve/bhyve_utils.h | 3 +++
> 3 files changed, 17 insertions(+)
Hi Alexander,
Thanks for implementing this! Overall it looks good, two comments:
* It doesn't take into account domains that use VNC with port
explicitly specified. For example, if I start a domain with VNC
configuration "port=5900 autoport=no", I will not be able
to start a domain with "autoport=yes" because it will try to
to 5900 and will fail to bind.
Looking at the qemu driver code, it seems it's done by calling
virPortAllocatorSetUsed().
I'm also wondering how it's handled for autostarting domains, e.g.
if I have vm1[5900], vm2[5901], vm3[autoport], vm4[autoport],
they'll start fine in that order, but will fail to start if going
this way: vm3, vm4, vm1, vm2
* Nitpick: Commit message titles are usually don't end with a period.
Also, they're usually prefixed with the relevant subsystem, so
this one could be "bhyve: Add support for VNC autoport feature"
for example (you might glance through 'git log' for examples)
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index eae5cb3ca..4d35a05c5 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -339,6 +339,9 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
> virBuffer opt = VIR_BUFFER_INITIALIZER;
> virDomainGraphicsListenDefPtr glisten = NULL;
> bool escapeAddr;
> + unsigned short port;
> +
> + bhyveConnPtr driver = conn->privateData;
>
> if (!(bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM) ||
> def->os.bootloader ||
> @@ -401,6 +404,12 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
> virBufferAdd(&opt, glisten->address, -1);
> }
>
> + if (graphics->data.vnc.autoport) {
> + if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
> + return -1;
> + graphics->data.vnc.port = port;
> + }
> +
> virBufferAsprintf(&opt, ":%d", graphics->data.vnc.port);
> break;
> default:
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index ed2221a35..bffeea7d9 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -52,6 +52,7 @@
> #include "viraccessapicheck.h"
> #include "virhostcpu.h"
> #include "virhostmem.h"
> +#include "virportallocator.h"
> #include "conf/domain_capabilities.h"
>
> #include "bhyve_conf.h"
> @@ -1219,6 +1220,7 @@ bhyveStateCleanup(void)
> virObjectUnref(bhyve_driver->closeCallbacks);
> virObjectUnref(bhyve_driver->domainEventState);
> virObjectUnref(bhyve_driver->config);
> + virObjectUnref(bhyve_driver->remotePorts);
>
> virMutexDestroy(&bhyve_driver->lock);
> VIR_FREE(bhyve_driver);
> @@ -1265,6 +1267,9 @@ bhyveStateInitialize(bool privileged,
> if (!(bhyve_driver->domainEventState = virObjectEventStateNew()))
> goto cleanup;
>
> + if (!(bhyve_driver->remotePorts = virPortAllocatorNew(_("display"), 5900, 65535, 0)))
> + goto cleanup;
> +
> bhyve_driver->hostsysinfo = virSysinfoRead();
>
> if (!(bhyve_driver->config = virBhyveDriverConfigNew()))
> diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
> index db50e012a..8ad2698d4 100644
> --- a/src/bhyve/bhyve_utils.h
> +++ b/src/bhyve/bhyve_utils.h
> @@ -28,6 +28,7 @@
> # include "virdomainobjlist.h"
> # include "virthread.h"
> # include "virclosecallbacks.h"
> +# include "virportallocator.h"
>
> # define BHYVE_AUTOSTART_DIR SYSCONFDIR "/libvirt/bhyve/autostart"
> # define BHYVE_CONFIG_DIR SYSCONFDIR "/libvirt/bhyve"
> @@ -58,6 +59,8 @@ struct _bhyveConn {
>
> virCloseCallbacksPtr closeCallbacks;
>
> + virPortAllocatorPtr remotePorts;
> +
> unsigned bhyvecaps;
> unsigned grubcaps;
> };
Roman Bogorodskiy
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
