[libvirt] [PATCH v2] driver: Include source as a flag to virDomainGetHostname
Michal Privoznik
mprivozn at redhat.com
Tue Dec 3 16:31:42 UTC 2019
On 12/2/19 4:24 AM, Julio Faracco wrote:
> There is a lots of possibilities to retrieve hostname information from
> domain. Libvirt could use lease information from dnsmasq to get current
> hostname too. QEMU supports QEMU-agent but it can use lease source. See
> 'domifaddr' as an example.
>
> This commit still adds lease options for QEMU. It will get the first
> hostname available from domain networks.
>
> This case, every driver has a default section inside switch to keep
> compatibility. So, if someone call 'domhostname' without specifying
> source, it will get the default option.
>
> Signed-off-by: Julio Faracco <jcfaracco at gmail.com>
> ---
> include/libvirt/libvirt-domain.h | 5 +++
> src/lxc/lxc_driver.c | 71 ++++++++++++++++++++++++++++++++
> src/openvz/openvz_driver.c | 30 ++++++++++----
> src/qemu/qemu_driver.c | 69 ++++++++++++++++++++++++++-----
> tools/virsh-domain.c | 25 ++++++++++-
> 5 files changed, 178 insertions(+), 22 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index a2f007568c..b37f33d5d0 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4790,6 +4790,11 @@ typedef struct _virTypedParameter virMemoryParameter;
> */
> typedef virMemoryParameter *virMemoryParameterPtr;
>
> +typedef enum {
> + VIR_DOMAIN_HOSTNAME_SRC_LEASE = (1 << 0), /* Parse DHCP lease file */
> + VIR_DOMAIN_HOSTNAME_SRC_AGENT = (1 << 1), /* Query qemu guest agent */
> +} virDomainHostnameSource;
> +
You need to document that virDomainGetHostname() uses these new flags.
And also virDomainGetHostnameFlags would probably be a better name for
the enum then. And since the enum is changing its name its members
should reflect that too.
> typedef enum {
> VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE = 0, /* Parse DHCP lease file */
> VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT = 1, /* Query qemu guest agent */
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 826bf074e3..3221b06261 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -5321,6 +5321,76 @@ lxcDomainGetCPUStats(virDomainPtr dom,
> return ret;
> }
>
> +static char *
> +lxcDomainGetHostname(virDomainPtr dom,
> + unsigned int flags)
> +{
> + virDomainObjPtr vm = NULL;
> + char macaddr[VIR_MAC_STRING_BUFLEN];
> + g_autoptr(virNetwork) network = NULL;
> + virNetworkDHCPLeasePtr *leases = NULL;
> + int n_leases;
> + size_t i, j;
> + char *hostname = NULL;
A lot of these are used in the for() loop. Might be worth declaring them
for better readability.
> +
> + virCheckFlags(VIR_DOMAIN_HOSTNAME_SRC_LEASE |
> + VIR_DOMAIN_HOSTNAME_SRC_AGENT, NULL);
Since AGENT is not supported by this API you can leave it out from here
and virCheckFlags() will produce sensible error.
> +
> + if (!(vm = lxcDomObjFromDomain(dom)))
> + return NULL;
> +
> + if (virDomainGetHostnameEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
This looks like a good place to check if the domain is alive. Inactive
domains don't have hostnames, do they?
> + switch (flags) {
Huh, are these two flags mutually exclusive? I know that you will end up
with just one flag here (and thus may remove the switch() completely),
but usually flags are tested for like this:
if (flags & FLAG1) { ... }
if (flags & FLAG2) { ... }
If we want to make the flags exclusive (which is okay), then you'll also
need VIR_EXCLUSIVE_FLAGS_RET() and it could go right after
virCheckFlags() call. Again, I know it's not needed for lxc case, but
looks like qemu driver will need it.
> + default:
> + case VIR_DOMAIN_HOSTNAME_SRC_LEASE:
> + for (i = 0; i < vm->def->nnets; i++) {
> + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> + continue;
> +
> + virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
> + virObjectUnref(network);
If you'd declare @network here this unref won't be needed.
> + network = virNetworkLookupByName(dom->conn,
> + vm->def->nets[i]->data.network.name);
There are two problems with this:
1) it breaks split daemon - dom->conn is not the correct connection
object. You'll need to call virGetConnectNetwork() to obtain a
connection object to the network driver and then pass it to this
function (note, you don't have to obtain the connection object inside
this loop, it can be done outside). I know we have a couple of bad
examples in our code and I will be proposing a fix shortly.
2) The function can return NULL. Unlikely, but can happen.
> +
> + if ((n_leases = virNetworkGetDHCPLeases(network, macaddr,
> + &leases, 0)) < 0)
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("There is no available hostname %d"),
> + flags);
No need to overwirte the sensible error message produced by the API.
Also, missing goto cleaup perhaps?
> +
> + for (j = 0; j < n_leases; j++) {
> + virNetworkDHCPLeasePtr lease = leases[j];
> + if (lease->hostname) {
> + hostname = g_strdup(lease->hostname);
> +
> + for (j = 0; j < n_leases; j++)
> + virNetworkDHCPLeaseFree(leases[j]);
> +
> + VIR_FREE(leases);
> +
> + goto cleanup;
> + }
> + }
> +
> + for (j = 0; j < n_leases; j++)
> + virNetworkDHCPLeaseFree(leases[j]);
> +
> + VIR_FREE(leases);
What if these two loops were joined into a single one?
for () {
virNetworkDHCPLeasePtr lease = leases[j];
if (lease->hostname)
hostname = g_stdup();
virNetworkDHCPleaseFree(lease);
}
VIR_FREE(leases);
if (hostname)
break;
> + }
> + break;
> + case VIR_DOMAIN_HOSTNAME_SRC_AGENT:
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> + _("Unknown hostname data source %d"),
> + flags);
> + break;
This can be removed.
> + }
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return hostname;
> +}
> > static int
Small nit-pick - we have two spaces in between functions in this file.
Please keep it that way.
> lxcNodeGetFreePages(virConnectPtr conn,
> @@ -5467,6 +5537,7 @@ static virHypervisorDriver lxcHypervisorDriver = {
> .domainSetMetadata = lxcDomainSetMetadata, /* 1.1.3 */
> .domainGetMetadata = lxcDomainGetMetadata, /* 1.1.3 */
> .domainGetCPUStats = lxcDomainGetCPUStats, /* 1.2.2 */
> + .domainGetHostname = lxcDomainGetHostname, /* 5.9.0 */
This has to be 6.0.0 now, sorry.
> .nodeGetMemoryParameters = lxcNodeGetMemoryParameters, /* 0.10.2 */
> .nodeSetMemoryParameters = lxcNodeSetMemoryParameters, /* 0.10.2 */
> .domainSendProcessSignal = lxcDomainSendProcessSignal, /* 1.0.1 */
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index e07b3b302d..c9f8255f19 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -301,19 +301,31 @@ openvzDomainGetHostname(virDomainPtr dom, unsigned int flags)
> struct openvz_driver *driver = dom->conn->privateData;
> virDomainObjPtr vm;
>
> - virCheckFlags(0, NULL);
> + virCheckFlags(VIR_DOMAIN_HOSTNAME_SRC_LEASE |
> + VIR_DOMAIN_HOSTNAME_SRC_AGENT, NULL);
> +
Again, if openvz doesn't really support these flags then you don't need
to change this file at all.
> if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
> return NULL;
>
> - hostname = openvzVEGetStringParam(dom, "hostname");
> - if (hostname == NULL)
> - goto cleanup;
> + switch (flags) {
> + default:
> + hostname = openvzVEGetStringParam(dom, "hostname");
> + if (hostname == NULL)
> + goto cleanup;
>
> - /* vzlist prints an unset hostname as '-' */
> - if (STREQ(hostname, "-")) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("Hostname of '%s' is unset"), vm->def->name);
> - VIR_FREE(hostname);
> + /* vzlist prints an unset hostname as '-' */
> + if (STREQ(hostname, "-")) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("Hostname of '%s' is unset"), vm->def->name);
> + VIR_FREE(hostname);
> + }
> + break;
> + case VIR_DOMAIN_HOSTNAME_SRC_AGENT:
> + case VIR_DOMAIN_HOSTNAME_SRC_LEASE:
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> + _("Unknown hostname data source %d"),
> + flags);
> + break;
> }
>
> cleanup:
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b5300241a8..928f75cafe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20090,9 +20090,15 @@ qemuDomainGetHostname(virDomainPtr dom,
Most of my comments to LXC driver apply here also. Imagine them here.
> virQEMUDriverPtr driver = dom->conn->privateData;
> virDomainObjPtr vm = NULL;
> qemuAgentPtr agent;
> + char macaddr[VIR_MAC_STRING_BUFLEN];
> + g_autoptr(virNetwork) network = NULL;
> + virNetworkDHCPLeasePtr *leases = NULL;
> + int n_leases;
> + size_t i, j;
> char *hostname = NULL;
>
> - virCheckFlags(0, NULL);
> + virCheckFlags(VIR_DOMAIN_HOSTNAME_SRC_LEASE |
> + VIR_DOMAIN_HOSTNAME_SRC_AGENT, NULL);
>
> if (!(vm = qemuDomainObjFromDomain(dom)))
> return NULL;
> @@ -20100,21 +20106,62 @@ qemuDomainGetHostname(virDomainPtr dom,
> if (virDomainGetHostnameEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> - if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> - goto cleanup;
> -
> if (virDomainObjCheckActive(vm) < 0)
> goto endjob;
>
> - if (!qemuDomainAgentAvailable(vm, true))
> - goto endjob;
> + switch (flags) {
> + default:
> + case VIR_DOMAIN_HOSTNAME_SRC_AGENT:
> + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> + goto cleanup;
>
> - agent = qemuDomainObjEnterAgent(vm);
> - ignore_value(qemuAgentGetHostname(agent, &hostname));
> - qemuDomainObjExitAgent(vm, agent);
> + if (!qemuDomainAgentAvailable(vm, true))
> + goto endjob;
>
> - endjob:
> - qemuDomainObjEndAgentJob(vm);
> + agent = qemuDomainObjEnterAgent(vm);
> + ignore_value(qemuAgentGetHostname(agent, &hostname));
> + qemuDomainObjExitAgent(vm, agent);
> +
> + endjob:
> + qemuDomainObjEndAgentJob(vm);
> + break;
> + case VIR_DOMAIN_HOSTNAME_SRC_LEASE:
> + for (i = 0; i < vm->def->nnets; i++) {
> + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> + continue;
> +
> + virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
> + virObjectUnref(network);
> + network = virNetworkLookupByName(dom->conn,
> + vm->def->nets[i]->data.network.name);
> +
> + if ((n_leases = virNetworkGetDHCPLeases(network, macaddr,
> + &leases, 0)) < 0)
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("There is no available hostname %d"),
> + flags);
> +
> + for (j = 0; j < n_leases; j++) {
> + virNetworkDHCPLeasePtr lease = leases[j];
> + if (lease->hostname) {
> + hostname = g_strdup(lease->hostname);
> +
> + for (j = 0; j < n_leases; j++)
> + virNetworkDHCPLeaseFree(leases[j]);
> +
> + VIR_FREE(leases);
> +
> + goto cleanup;
> + }
> + }
> +
> + for (j = 0; j < n_leases; j++)
> + virNetworkDHCPLeaseFree(leases[j]);
> +
> + VIR_FREE(leases);
> + }
> + break;
> + }
>
> cleanup:
> virDomainObjEndAPI(&vm);
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 6be9780836..01de6b633a 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11669,6 +11669,10 @@ static const vshCmdInfo info_domhostname[] = {
>
> static const vshCmdOptDef opts_domhostname[] = {
> VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
> + {.name = "source",
> + .type = VSH_OT_STRING,
> + .flags = VSH_OFLAG_NONE,
> + .help = N_("address source: 'lease' or 'agent'")},
Not a show stopper, but may be nice to write a completer for that ;-)
It can be really simple one, just return a dynamically allocated string
list containing these two modes.
> {.name = NULL}
> };
>
> @@ -11678,21 +11682,38 @@ cmdDomHostname(vshControl *ctl, const vshCmd *cmd)
> char *hostname;
> virDomainPtr dom;
> bool ret = false;
> + const char *sourcestr = NULL;
> + int flags = 0; /* Use default value. Drivers can have its own default. */
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
> - hostname = virDomainGetHostname(dom, 0);
> + if (vshCommandOptStringReq(ctl, cmd, "source", &sourcestr) < 0)
> + goto error;
> +
> + if (sourcestr) {
> + if (STREQ(sourcestr, "lease")) {
> + flags |= VIR_DOMAIN_HOSTNAME_SRC_LEASE;
> + } else if (STREQ(sourcestr, "agent")) {
> + flags |= VIR_DOMAIN_HOSTNAME_SRC_AGENT;
> + } else {
> + vshError(ctl, _("Unknown data source '%s'"), sourcestr);
> + goto error;
> + }
> + }
> +
> + hostname = virDomainGetHostname(dom, flags);
> if (hostname == NULL) {
> vshError(ctl, "%s", _("failed to get hostname"));
> goto error;
> }
>
> vshPrint(ctl, "%s\n", hostname);
> +
> + VIR_FREE(hostname);
> ret = true;
>
> error:
> - VIR_FREE(hostname);
> virshDomainFree(dom);
> return ret;
> }
>
Don't forget to update the manpage. Also, documenting this change in
news.xml would be nice ;-)
Looking forward to v3.
Michal
More information about the libvir-list
mailing list