Re: [libvirt] [PATCH 10/40] Simplify the Xen domain lookup driver methods

Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Unconditionally invoke the xenHypervisorLookupDomainByID, > xenHypervisorLookupDomainByUUID or xenDaemonLookupByName > for looking up domains. Fallback to xenXMDomainLookupByUUID > and xenXMDomainLookupByName for legacy XenD without inactive > domain support > > Do you think there are any Xen installations running such an old xend toolstack, and if so wanting to use e.g. libvirt 1.0.6? Seems all of the XEND_CONFIG_VERSION_3_0_3 logic could be removed from the code. > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > src/xen/xen_driver.c | 99 +++++++++++-------------------------------------- > src/xen/xend_internal.c | 89 -------------------------------------------- > src/xen/xend_internal.h | 14 ------- > src/xen/xs_internal.c | 62 ------------------------------- > src/xen/xs_internal.h | 2 - > 5 files changed, 22 insertions(+), 244 deletions(-) > > I spent some time testing this one and didn't notice any problems.

> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c > index 82058b7..080045c 100644 > --- a/src/xen/xen_driver.c > +++ b/src/xen/xen_driver.c > @@ -601,38 +601,17 @@ xenUnifiedDomainCreateXML(virConnectPtr conn, > return xenDaemonCreateXML(conn, xmlDesc); > } > > -/* Assumption made in underlying drivers: > - * If the domain is "not found" and there is no other error, then > - * the Lookup* functions return a NULL but do not set virterror. > - */ > static virDomainPtr > xenUnifiedDomainLookupByID(virConnectPtr conn, int id) > { > - xenUnifiedPrivatePtr priv = conn->privateData; > - virDomainPtr ret; > + virDomainPtr ret = NULL; > > - /* Reset any connection-level errors in virterror first, in case > - * there is one hanging around from a previous call. > - */ > - virConnResetLastError(conn); > + ret = xenHypervisorLookupDomainByID(conn, id); > > - /* Try hypervisor/xenstore combo. */ > - if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET]) { > - ret = xenHypervisorLookupDomainByID(conn, id); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > - > - /* Try xend. */ > - if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) { > - ret = xenDaemonLookupByID(conn, id); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > + if (!ret && virGetLastError() == NULL) > + virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); > > - /* Not found. */ > - virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); > - return NULL; > + return ret; > } > > static virDomainPtr > @@ -642,35 +621,20 @@ xenUnifiedDomainLookupByUUID(virConnectPtr conn, > xenUnifiedPrivatePtr priv = conn->privateData; > virDomainPtr ret; > > - /* Reset any connection-level errors in virterror first, in case > - * there is one hanging around from a previous call. > - */ > - virConnResetLastError(conn); > - > - /* Try hypervisor/xenstore combo. */ > - if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET]) { > - ret = xenHypervisorLookupDomainByUUID(conn, uuid); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > - > - /* Try xend. */ > - if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) { > - ret = xenDaemonLookupByUUID(conn, uuid); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > + ret = xenHypervisorLookupDomainByUUID(conn, uuid); > > /* Try XM for inactive domains. */ > - if (priv->opened[XEN_UNIFIED_XM_OFFSET]) { > - ret = xenXMDomainLookupByUUID(conn, uuid); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > + if (!ret) { > + if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) > + ret = xenXMDomainLookupByUUID(conn, uuid); > + else > + return xenDaemonLookupByUUID(conn, uuid); > } > > - /* Not found. */ > - virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); > - return NULL; > + if (!ret && virGetLastError() == NULL) > + virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); > + > + return ret; > } > > static virDomainPtr > @@ -680,35 +644,16 @@ xenUnifiedDomainLookupByName(virConnectPtr conn, > xenUnifiedPrivatePtr priv = conn->privateData; > virDomainPtr ret; > > - /* Reset any connection-level errors in virterror first, in case > - * there is one hanging around from a previous call. > - */ > - virConnResetLastError(conn); > - > - /* Try xend. */ > - if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) { > - ret = xenDaemonLookupByName(conn, name); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > - > - /* Try xenstore for inactive domains. */ > - if (priv->opened[XEN_UNIFIED_XS_OFFSET]) { > - ret = xenStoreLookupByName(conn, name); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > + ret = xenDaemonLookupByName(conn, name); > > /* Try XM for inactive domains. */ > - if (priv->opened[XEN_UNIFIED_XM_OFFSET]) { > + if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) > ret = xenXMDomainLookupByName(conn, name); > - if (ret || conn->err.code != VIR_ERR_OK) > - return ret; > - } > > - /* Not found. */ > - virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); > - return NULL; > + if (!ret && virGetLastError() == NULL) > + virReportError(VIR_ERR_NO_DOMAIN, __FUNCTION__); > + > + return ret; > } > > > @@ -719,7 +664,7 @@ xenUnifiedDomainIsActive(virDomainPtr dom) > int ret = -1; > > /* ID field in dom may be outdated, so re-lookup */ > - currdom = xenUnifiedDomainLookupByUUID(dom->conn, dom->uuid); >

> + currdom = xenHypervisorLookupDomainByUUID(dom->conn, dom->uuid); > > if (currdom) { > ret = currdom->id == -1 ? 0 : 1; > diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c > index 2e6a47e..4ad30fa 100644 > --- a/src/xen/xend_internal.c > +++ b/src/xen/xend_internal.c > @@ -855,63 +855,6 @@ xenDaemonDomainLookupByName_ids(virConnectPtr xend, > } > > > -/** > - * xenDaemonDomainLookupByID: > - * @xend: A xend instance > - * @id: The id of the domain > - * @name: return value for the name if not NULL > - * @uuid: return value for the UUID if not NULL > - * > - * This method looks up the name of a domain based on its id > - * > - * Returns the 0 on success; -1 (with errno) on error > - */ > -int > -xenDaemonDomainLookupByID(virConnectPtr xend, > - int id, > - char **domname, > - unsigned char *uuid) > -{ > - const char *name = NULL; > - struct sexpr *root; > - > - memset(uuid, 0, VIR_UUID_BUFLEN); > - > - root = sexpr_get(xend, "/xend/domain/%d?detail=1", id); > - if (root == NULL) > - goto error; > - > - name = sexpr_node(root, "domain/name"); > - if (name == NULL) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("domain information incomplete, missing name")); > - goto error; > - } > - if (domname) { > - *domname = strdup(name); > - if (*domname == NULL) { > - virReportOOMError(); > - goto error; > - } > - } > - > - if (sexpr_uuid(uuid, root, "domain/uuid") < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("domain information incomplete, missing uuid")); > - goto error; > - } > - > - sexpr_free(root); > - return 0; > - > -error: > - sexpr_free(root); > - if (domname) > - VIR_FREE(*domname); > - return -1; > -} > - > - > static int > xend_detect_config_version(virConnectPtr conn) > { > @@ -1863,38 +1806,6 @@ xenDaemonNodeGetTopology(virConnectPtr conn, virCapsPtr caps) > > > /** > - * xenDaemonLookupByID: > - * @conn: pointer to the hypervisor connection > - * @id: the domain ID number > - * > - * Try to find a domain based on the hypervisor ID number > - * > - * Returns a new domain object or NULL in case of failure > - */ > -virDomainPtr > -xenDaemonLookupByID(virConnectPtr conn, int id) > -{ > - char *name = NULL; > - unsigned char uuid[VIR_UUID_BUFLEN]; > - virDomainPtr ret; > - > - if (xenDaemonDomainLookupByID(conn, id, &name, uuid) < 0) { > - goto error; > - } > - > - ret = virGetDomain(conn, name, uuid); > - if (ret == NULL) goto error; > - > - ret->id = id; > - VIR_FREE(name); > - return ret; > - > - error: > - VIR_FREE(name); > - return NULL; > -} > - > -/** > * xenDaemonDomainSetVcpusFlags: > * @domain: pointer to domain object > * @nvcpus: the new number of virtual CPUs for this domain > diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h > index 5f82f04..e8713a7 100644 > --- a/src/xen/xend_internal.h > +++ b/src/xen/xend_internal.h > @@ -69,19 +69,6 @@ int xenDaemonDomainLookupByName_ids(virConnectPtr xend, > const char *name, unsigned char *uuid); > > > -/** > - * \brief Lookup the name of a domain > - * \param xend A xend instance > - * \param id The id of the domain > - * \param name pointer to store a copy of the name > - * \param uuid pointer to store a copy of the uuid > - * > - * This method looks up the name/uuid of a domain > - */ > -int xenDaemonDomainLookupByID(virConnectPtr xend, > - int id, > - char **name, unsigned char *uuid); > - > > virDomainDefPtr > xenDaemonDomainFetch(virConnectPtr xend, > @@ -153,7 +140,6 @@ extern struct xenUnifiedDriver xenDaemonDriver; > int xenDaemonInit (void); > > virDomainPtr xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc); > -virDomainPtr xenDaemonLookupByID(virConnectPtr conn, int id); > virDomainPtr xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid); > virDomainPtr xenDaemonLookupByName(virConnectPtr conn, const char *domname); > int xenDaemonDomainMigratePrepare (virConnectPtr dconn, char **cookie, int *cookielen, const char *uri_in, char **uri_out, unsigned long flags, const char *dname, unsigned long resource); > diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c > index dbb4ae4..7926535 100644 > --- a/src/xen/xs_internal.c > +++ b/src/xen/xs_internal.c > @@ -580,68 +580,6 @@ xenStoreListDomains(virConnectPtr conn, int *ids, int maxids) > return ret; > } > > -/** > - * xenStoreLookupByName: > - * @conn: A xend instance > - * @name: The name of the domain > - * > - * Try to lookup a domain on the Xen Store based on its name. > - * > - * Returns a new domain object or NULL in case of failure > - */ > -virDomainPtr > -xenStoreLookupByName(virConnectPtr conn, const char *name) > -{ > - virDomainPtr ret = NULL; > - unsigned int num, i, len; > - long id = -1; > - char **idlist = NULL, *endptr; > - char prop[200], *tmp; > - int found = 0; > - struct xend_domain *xenddomain = NULL; > - xenUnifiedPrivatePtr priv = conn->privateData; > - > - if (priv->xshandle == NULL) > - return NULL; > - > - idlist = xs_directory(priv->xshandle, 0, "/local/domain", &num); > - if (idlist == NULL) > - goto done; > - > - for (i = 0; i < num; i++) { > - id = strtol(idlist[i], &endptr, 10); > - if ((endptr == idlist[i]) || (*endptr != 0)) { > - goto done; > - } > -#if 0 > - if (virConnectCheckStoreID(conn, (int) id) < 0) > - continue; > -#endif > - snprintf(prop, 199, "/local/domain/%s/name", idlist[i]); > - prop[199] = 0; > - tmp = xs_read(priv->xshandle, 0, prop, &len); > - if (tmp != NULL) { > - found = STREQ(name, tmp); > - VIR_FREE(tmp); > - if (found) > - break; > - } > - } > - if (!found) > - goto done; > - > - ret = virGetDomain(conn, name, NULL); > - if (ret == NULL) > - goto done; > - > - ret->id = id; > - > -done: > - VIR_FREE(xenddomain); > - VIR_FREE(idlist); > - > - return ret; > -} > > /** > * xenStoreDomainShutdown: > diff --git a/src/xen/xs_internal.h b/src/xen/xs_internal.h > index 29f0165..fc7798d 100644 > --- a/src/xen/xs_internal.h > +++ b/src/xen/xs_internal.h > @@ -43,8 +43,6 @@ int xenStoreNumOfDomains (virConnectPtr conn); > int xenStoreListDomains (virConnectPtr conn, > int *ids, > int maxids); > -virDomainPtr xenStoreLookupByName(virConnectPtr conn, > - const char *name); > unsigned long xenStoreGetMaxMemory (virDomainPtr domain); > int xenStoreDomainSetMemory (virDomainPtr domain, > unsigned long memory); > > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list