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@xxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
