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@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