Re: [libvirt] [PATCH v2] driver: Include source as a flag to virDomainGetHostname

On 12/2/19 4:24 AM, Julio Faracco wrote: 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. A lot of these are used in the for() loop. Might be worth declaring them for better readability. Since AGENT is not supported by this API you can leave it out from here and virCheckFlags() will produce sensible error. This looks like a good place to check if the domain is alive. Inactive domains don't have hostnames, do they? 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. If you'd declare @network here this unref won't be needed. 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. No need to overwirte the sensible error message produced by the API. Also, missing goto cleaup perhaps? 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; This can be removed. Small nit-pick - we have two spaces in between functions in this file. Please keep it that way. This has to be 6.0.0 now, sorry. 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; > + 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; > + 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); > + } > + 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. Don't forget to update the manpage. Also, documenting this change in news.xml would be nice ;-) Looking forward to v3. Michal