[libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

jcfaracco@gmail.com posted 2 patches 12 weeks ago

[libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

Posted by jcfaracco@gmail.com 12 weeks ago
From: Julio Faracco <jcfaracco@gmail.com>

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.

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 include/libvirt/libvirt-domain.h | 10 +++++
 src/driver-hypervisor.h          |  1 +
 src/libvirt-domain.c             |  7 +++-
 src/openvz/openvz_driver.c       | 30 +++++++++-----
 src/qemu/qemu_driver.c           | 67 +++++++++++++++++++++++++++-----
 src/remote/remote_protocol.x     |  1 +
 src/remote_protocol-structs      |  1 +
 src/test/test_driver.c           |  1 +
 tools/virsh-domain.c             | 25 +++++++++++-
 9 files changed, 120 insertions(+), 23 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index a2f007568c..cc3d6ed0ca 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1554,6 +1554,7 @@ int                     virDomainGetMaxVcpus    (virDomainPtr domain);
 int                     virDomainGetSecurityLabel (virDomainPtr domain,
                                                    virSecurityLabelPtr seclabel);
 char *                  virDomainGetHostname    (virDomainPtr domain,
+                                                 unsigned int source,
                                                  unsigned int flags);
 int                     virDomainGetSecurityLabelList (virDomainPtr domain,
                                                        virSecurityLabelPtr* seclabels);
@@ -4790,6 +4791,15 @@ typedef struct _virTypedParameter virMemoryParameter;
  */
 typedef virMemoryParameter *virMemoryParameterPtr;
 
+typedef enum {
+    VIR_DOMAIN_HOSTNAME_SRC_LEASE = 0, /* Parse DHCP lease file */
+    VIR_DOMAIN_HOSTNAME_SRC_AGENT = 1, /* Query qemu guest agent */
+
+# ifdef VIR_ENUM_SENTINELS
+    VIR_DOMAIN_HOSTNAME_SRC_LAST
+# endif
+} virDomainHostnameSource;
+
 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/driver-hypervisor.h b/src/driver-hypervisor.h
index 4afd8f6ec5..6338a3ab73 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -156,6 +156,7 @@ typedef char *
 
 typedef char *
 (*virDrvDomainGetHostname)(virDomainPtr domain,
+                           unsigned int source,
                            unsigned int flags);
 
 typedef unsigned long long
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 51fb79cddd..6aa093d8af 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11017,6 +11017,7 @@ virDomainGetDiskErrors(virDomainPtr dom,
 /**
  * virDomainGetHostname:
  * @domain: a domain object
+ * @source: one of the virDomainHostnameSource constants
  * @flags: extra flags; not used yet, so callers should always pass 0
  *
  * Get the hostname for that domain.
@@ -11028,7 +11029,9 @@ virDomainGetDiskErrors(virDomainPtr dom,
  * NULL if there was an error.
  */
 char *
-virDomainGetHostname(virDomainPtr domain, unsigned int flags)
+virDomainGetHostname(virDomainPtr domain,
+                     unsigned int source,
+                     unsigned int flags)
 {
     virConnectPtr conn;
 
@@ -11043,7 +11046,7 @@ virDomainGetHostname(virDomainPtr domain, unsigned int flags)
 
     if (conn->driver->domainGetHostname) {
         char *ret;
-        ret = conn->driver->domainGetHostname(domain, flags);
+        ret = conn->driver->domainGetHostname(domain, source, flags);
         if (!ret)
             goto error;
         return ret;
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index e07b3b302d..363eb5522a 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -295,7 +295,9 @@ openvzSetDiskQuota(virDomainDefPtr vmdef,
 
 
 static char *
-openvzDomainGetHostname(virDomainPtr dom, unsigned int flags)
+openvzDomainGetHostname(virDomainPtr dom,
+                        unsigned int source,
+                        unsigned int flags)
 {
     char *hostname = NULL;
     struct openvz_driver *driver = dom->conn->privateData;
@@ -305,15 +307,25 @@ openvzDomainGetHostname(virDomainPtr dom, unsigned int flags)
     if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
         return NULL;
 
-    hostname = openvzVEGetStringParam(dom, "hostname");
-    if (hostname == NULL)
-        goto cleanup;
+    switch (source) {
+    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"),
+                       source);
+        break;
     }
 
  cleanup:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 669c12d6ca..afcd5e8571 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20085,11 +20085,17 @@ qemuConnectGetCPUModelNames(virConnectPtr conn,
 
 static char *
 qemuDomainGetHostname(virDomainPtr dom,
+                      unsigned int source,
                       unsigned int flags)
 {
     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);
@@ -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 (source) {
+    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"),
+                               source);
+
+            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/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 23e42d17b1..8d09f605da 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -1467,6 +1467,7 @@ struct remote_domain_get_cpu_stats_ret {
 
 struct remote_domain_get_hostname_args {
     remote_nonnull_domain dom;
+    unsigned int source;
     unsigned int flags;
 };
 
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 9ad7a857e0..dd9d1ec6fa 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -3543,4 +3543,5 @@ enum remote_procedure {
         REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418,
         REMOTE_PROC_CONNECT_SET_IDENTITY = 419,
         REMOTE_PROC_DOMAIN_AGENT_SET_RESPONSE_TIMEOUT = 420,
+        REMOTE_PROC_DOMAIN_GET_HOSTNAME = 421,
 };
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5883722d60..0cbda81c67 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -2028,6 +2028,7 @@ testDomainReset(virDomainPtr dom,
 
 static char *
 testDomainGetHostname(virDomainPtr domain,
+                      unsigned int source G_GNUC_UNUSED,
                       unsigned int flags)
 {
     char *ret = NULL;
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 6be9780836..141b141a10 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'")},
     {.name = NULL}
 };
 
@@ -11678,21 +11682,38 @@ cmdDomHostname(vshControl *ctl, const vshCmd *cmd)
     char *hostname;
     virDomainPtr dom;
     bool ret = false;
+    const char *sourcestr = NULL;
+    int source = -1; /* 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")) {
+            source = VIR_DOMAIN_HOSTNAME_SRC_LEASE;
+        } else if (STREQ(sourcestr, "agent")) {
+            source = VIR_DOMAIN_HOSTNAME_SRC_AGENT;
+        } else {
+            vshError(ctl, _("Unknown data source '%s'"), sourcestr);
+            goto error;
+        }
+    }
+
+    hostname = virDomainGetHostname(dom, source, 0);
     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;
 }
-- 
2.20.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

Posted by Peter Krempa 12 weeks ago
On Sun, Nov 24, 2019 at 17:19:48 -0300, jcfaracco@gmail.com wrote:
> From: Julio Faracco <jcfaracco@gmail.com>
> 
> 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.
> 
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  include/libvirt/libvirt-domain.h | 10 +++++
>  src/driver-hypervisor.h          |  1 +
>  src/libvirt-domain.c             |  7 +++-
>  src/openvz/openvz_driver.c       | 30 +++++++++-----
>  src/qemu/qemu_driver.c           | 67 +++++++++++++++++++++++++++-----
>  src/remote/remote_protocol.x     |  1 +
>  src/remote_protocol-structs      |  1 +
>  src/test/test_driver.c           |  1 +
>  tools/virsh-domain.c             | 25 +++++++++++-
>  9 files changed, 120 insertions(+), 23 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index a2f007568c..cc3d6ed0ca 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1554,6 +1554,7 @@ int                     virDomainGetMaxVcpus    (virDomainPtr domain);
>  int                     virDomainGetSecurityLabel (virDomainPtr domain,
>                                                     virSecurityLabelPtr seclabel);
>  char *                  virDomainGetHostname    (virDomainPtr domain,
> +                                                 unsigned int source,
>                                                   unsigned int flags);

Public API must never be changed.

NACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

Posted by Peter Krempa 12 weeks ago
On Sun, Nov 24, 2019 at 21:24:07 +0100, Peter Krempa wrote:
> On Sun, Nov 24, 2019 at 17:19:48 -0300, jcfaracco@gmail.com wrote:
> > From: Julio Faracco <jcfaracco@gmail.com>
> > 
> > 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.
> > 
> > Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> > ---
> >  include/libvirt/libvirt-domain.h | 10 +++++
> >  src/driver-hypervisor.h          |  1 +
> >  src/libvirt-domain.c             |  7 +++-
> >  src/openvz/openvz_driver.c       | 30 +++++++++-----
> >  src/qemu/qemu_driver.c           | 67 +++++++++++++++++++++++++++-----
> >  src/remote/remote_protocol.x     |  1 +
> >  src/remote_protocol-structs      |  1 +
> >  src/test/test_driver.c           |  1 +
> >  tools/virsh-domain.c             | 25 +++++++++++-
> >  9 files changed, 120 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > index a2f007568c..cc3d6ed0ca 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -1554,6 +1554,7 @@ int                     virDomainGetMaxVcpus    (virDomainPtr domain);
> >  int                     virDomainGetSecurityLabel (virDomainPtr domain,
> >                                                     virSecurityLabelPtr seclabel);
> >  char *                  virDomainGetHostname    (virDomainPtr domain,
> > +                                                 unsigned int source,
> >                                                   unsigned int flags);
> 
> Public API must never be changed.

More specifically we must never add or change parameters of existing
APIs and data types as other applications using libvirt would fail to
compile after the change.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

Posted by Julio Faracco 12 weeks ago
Thanks for your explanation, Peter!

I will focus on LXC side then...

Em dom., 24 de nov. de 2019 às 18:34, Peter Krempa
<pkrempa@redhat.com> escreveu:
>
> On Sun, Nov 24, 2019 at 21:24:07 +0100, Peter Krempa wrote:
> > On Sun, Nov 24, 2019 at 17:19:48 -0300, jcfaracco@gmail.com wrote:
> > > From: Julio Faracco <jcfaracco@gmail.com>
> > >
> > > 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.
> > >
> > > Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> > > ---
> > >  include/libvirt/libvirt-domain.h | 10 +++++
> > >  src/driver-hypervisor.h          |  1 +
> > >  src/libvirt-domain.c             |  7 +++-
> > >  src/openvz/openvz_driver.c       | 30 +++++++++-----
> > >  src/qemu/qemu_driver.c           | 67 +++++++++++++++++++++++++++-----
> > >  src/remote/remote_protocol.x     |  1 +
> > >  src/remote_protocol-structs      |  1 +
> > >  src/test/test_driver.c           |  1 +
> > >  tools/virsh-domain.c             | 25 +++++++++++-
> > >  9 files changed, 120 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > index a2f007568c..cc3d6ed0ca 100644
> > > --- a/include/libvirt/libvirt-domain.h
> > > +++ b/include/libvirt/libvirt-domain.h
> > > @@ -1554,6 +1554,7 @@ int                     virDomainGetMaxVcpus    (virDomainPtr domain);
> > >  int                     virDomainGetSecurityLabel (virDomainPtr domain,
> > >                                                     virSecurityLabelPtr seclabel);
> > >  char *                  virDomainGetHostname    (virDomainPtr domain,
> > > +                                                 unsigned int source,
> > >                                                   unsigned int flags);
> >
> > Public API must never be changed.
>
> More specifically we must never add or change parameters of existing
> APIs and data types as other applications using libvirt would fail to
> compile after the change.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

Posted by Michal Privoznik 12 weeks ago
On 11/24/19 10:33 PM, Julio Faracco wrote:
> Thanks for your explanation, Peter!
> 
> I will focus on LXC side then...
> 

You may still implement the API for other drivers too. The way we 
addressed this problem in the past is to create new API and have the old 
one call the new one. That's why we have "virSomething WithFlags()" APIs 
- because simply when introducing virSomething() API we did not think of 
adding flags. But if you then look how virSomething is implemented at 
driver level (say qemu driver) then you will see virSomething() is a 
very thin wrapper that calls virSomethingWithFlags(..., 0);.


For instance virDomainCreate() and virDomainCreateWithFlags() and 
qemuDomainCreate() and qemuDomainCreateWithFlags().

So you will need to add a new API and then have the old call the new one 
with sensible @source. Since the sensible value can differ among drivers 
I guess we can have VIR_DOMAIN_HOSTNAME_SRC_DEFAULT which would mean 
"hypervisor driver default as in the most sensible source chosen 
automatically by driver".


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] driver: Include source parameter to virDomainGetHostname

Posted by Daniel P. Berrangé 12 weeks ago
On Mon, Nov 25, 2019 at 05:06:31PM +0100, Michal Privoznik wrote:
> On 11/24/19 10:33 PM, Julio Faracco wrote:
> > Thanks for your explanation, Peter!
> > 
> > I will focus on LXC side then...
> > 
> 
> You may still implement the API for other drivers too. The way we addressed
> this problem in the past is to create new API and have the old one call the
> new one. That's why we have "virSomething WithFlags()" APIs - because simply
> when introducing virSomething() API we did not think of adding flags. But if
> you then look how virSomething is implemented at driver level (say qemu
> driver) then you will see virSomething() is a very thin wrapper that calls
> virSomethingWithFlags(..., 0);.
> 
> 
> For instance virDomainCreate() and virDomainCreateWithFlags() and
> qemuDomainCreate() and qemuDomainCreateWithFlags().
> 
> So you will need to add a new API and then have the old call the new one
> with sensible @source. Since the sensible value can differ among drivers I
> guess we can have VIR_DOMAIN_HOSTNAME_SRC_DEFAULT which would mean
> "hypervisor driver default as in the most sensible source chosen
> automatically by driver".

This API already has flags, so the alternative to adding a "source"
parameter is to reuse the flags parameter to express the desired
data source. This is what we do the virDomainReboot/Shutdown methods.

The separate source parameter is the approach for virDomainGetInterfaces.
While it is certainly nicer to have a separate source, I think reusing the
flags parameter is acceptable given the API design we have.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list