From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The Xen driver had a number of error reports which passed a constant string without format specifiers and was missing "%s". Furthermore the errors were related to failing system calls, but virReportSystemError was not used. So the only useful piece of info (the errno) was being discarded --- cfg.mk | 2 +- src/xen/xen_hypervisor.c | 114 ++++++++++++++++++++++------------------------ 2 files changed, 55 insertions(+), 61 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4225336..46331fd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -540,6 +540,7 @@ msg_gen_function += virReportError msg_gen_function += virReportErrorHelper msg_gen_function += virReportSystemError msg_gen_function += virSecurityReportError +msg_gen_function += virXenError msg_gen_function += virXenInotifyError msg_gen_function += virXenStoreError msg_gen_function += virXendError @@ -552,7 +553,6 @@ msg_gen_function += xenXMError # so that they are translatable. # msg_gen_function += fprintf # msg_gen_function += testError -# msg_gen_function += virXenError # msg_gen_function += vshPrint # msg_gen_function += vshError diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index b4ec579..9747010 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -526,9 +526,15 @@ static int lock_pages(void *addr, size_t len) { #ifdef __linux__ - return mlock(addr, len); + if (mlock(addr, len) < 0) { + virReportSystemError(errno, + _("Unable to lock %zu bytes of memory"), + len); + return -1; + } + return 0; #elif defined(__sun) - return 0; + return 0; #endif } @@ -536,9 +542,15 @@ static int unlock_pages(void *addr, size_t len) { #ifdef __linux__ - return munlock(addr, len); + if (munlock(addr, len) < 0) { + virReportSystemError(errno, + _("Unable to unlock %zu bytes of memory"), + len); + return -1; + } + return 0; #elif defined(__sun) - return 0; + return 0; #endif } @@ -900,21 +912,18 @@ xenHypervisorDoV0Op(int handle, xen_op_v0 * op) hc.op = __HYPERVISOR_dom0_op; hc.arg[0] = (unsigned long) op; - if (lock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + if (lock_pages(op, sizeof(dom0_op_t)) < 0) return -1; - } ret = ioctl(handle, xen_ioctl_hypercall_cmd, (unsigned long) &hc); if (ret < 0) { - virXenError(VIR_ERR_XEN_CALL, " ioctl %d", - xen_ioctl_hypercall_cmd); + virReportSystemError(errno, + _("Unable to issue hypervisor ioctl %d"), + xen_ioctl_hypercall_cmd); } - if (unlock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " releasing"); + if (unlock_pages(op, sizeof(dom0_op_t)) < 0) ret = -1; - } if (ret < 0) return -1; @@ -942,21 +951,18 @@ xenHypervisorDoV1Op(int handle, xen_op_v1* op) hc.op = __HYPERVISOR_dom0_op; hc.arg[0] = (unsigned long) op; - if (lock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + if (lock_pages(op, sizeof(dom0_op_t)) < 0) return -1; - } ret = ioctl(handle, xen_ioctl_hypercall_cmd, (unsigned long) &hc); if (ret < 0) { - virXenError(VIR_ERR_XEN_CALL, " ioctl %d", - xen_ioctl_hypercall_cmd); + virReportSystemError(errno, + _("Unable to issue hypervisor ioctl %d"), + xen_ioctl_hypercall_cmd); } - if (unlock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " releasing"); + if (unlock_pages(op, sizeof(dom0_op_t)) < 0) ret = -1; - } if (ret < 0) return -1; @@ -985,21 +991,18 @@ xenHypervisorDoV2Sys(int handle, xen_op_v2_sys* op) hc.op = __HYPERVISOR_sysctl; hc.arg[0] = (unsigned long) op; - if (lock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + if (lock_pages(op, sizeof(dom0_op_t)) < 0) return -1; - } ret = ioctl(handle, xen_ioctl_hypercall_cmd, (unsigned long) &hc); if (ret < 0) { - virXenError(VIR_ERR_XEN_CALL, " sys ioctl %d", - xen_ioctl_hypercall_cmd); + virReportSystemError(errno, + _("Unable to issue hypervisor ioctl %d"), + xen_ioctl_hypercall_cmd); } - if (unlock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " releasing"); + if (unlock_pages(op, sizeof(dom0_op_t)) < 0) ret = -1; - } if (ret < 0) return -1; @@ -1028,21 +1031,18 @@ xenHypervisorDoV2Dom(int handle, xen_op_v2_dom* op) hc.op = __HYPERVISOR_domctl; hc.arg[0] = (unsigned long) op; - if (lock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + if (lock_pages(op, sizeof(dom0_op_t)) < 0) return -1; - } ret = ioctl(handle, xen_ioctl_hypercall_cmd, (unsigned long) &hc); if (ret < 0) { - virXenError(VIR_ERR_XEN_CALL, " ioctl %d", - xen_ioctl_hypercall_cmd); + virReportSystemError(errno, + _("Unable to issue hypervisor ioctl %d"), + xen_ioctl_hypercall_cmd); } - if (unlock_pages(op, sizeof(dom0_op_t)) < 0) { - virXenError(VIR_ERR_XEN_CALL, " releasing"); + if (unlock_pages(op, sizeof(dom0_op_t)) < 0) ret = -1; - } if (ret < 0) return -1; @@ -1068,10 +1068,9 @@ virXen_getdomaininfolist(int handle, int first_domain, int maxids, int ret = -1; if (lock_pages(XEN_GETDOMAININFOLIST_DATA(dominfos), - XEN_GETDOMAININFO_SIZE * maxids) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + XEN_GETDOMAININFO_SIZE * maxids) < 0) return -1; - } + if (hv_versions.hypervisor > 1) { xen_op_v2_sys op; @@ -1123,10 +1122,9 @@ virXen_getdomaininfolist(int handle, int first_domain, int maxids, ret = op.u.getdomaininfolist.num_domains; } if (unlock_pages(XEN_GETDOMAININFOLIST_DATA(dominfos), - XEN_GETDOMAININFO_SIZE * maxids) < 0) { - virXenError(VIR_ERR_XEN_CALL, " release"); + XEN_GETDOMAININFO_SIZE * maxids) < 0) ret = -1; - } + return ret; } @@ -1747,10 +1745,9 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, if (hv_versions.hypervisor > 1) { xen_op_v2_dom op; - if (lock_pages(cpumap, maplen) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + if (lock_pages(cpumap, maplen) < 0) return -1; - } + memset(&op, 0, sizeof(op)); op.cmd = XEN_V2_OP_SETVCPUMAP; op.domain = (domid_t) id; @@ -1782,10 +1779,8 @@ virXen_setvcpumap(int handle, int id, unsigned int vcpu, ret = xenHypervisorDoV2Dom(handle, &op); VIR_FREE(new); - if (unlock_pages(cpumap, maplen) < 0) { - virXenError(VIR_ERR_XEN_CALL, " release"); + if (unlock_pages(cpumap, maplen) < 0) ret = -1; - } } else { cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ uint64_t *pm = &xen_cpumap; @@ -1879,10 +1874,9 @@ virXen_getvcpusinfo(int handle, int id, unsigned int vcpu, virVcpuInfoPtr ipt, ipt->cpu = op.u.getvcpuinfod5.online ? (int)op.u.getvcpuinfod5.cpu : -1; } if ((cpumap != NULL) && (maplen > 0)) { - if (lock_pages(cpumap, maplen) < 0) { - virXenError(VIR_ERR_XEN_CALL, " locking"); + if (lock_pages(cpumap, maplen) < 0) return -1; - } + memset(cpumap, 0, maplen); memset(&op, 0, sizeof(op)); op.cmd = XEN_V2_OP_GETVCPUMAP; @@ -1897,10 +1891,8 @@ virXen_getvcpusinfo(int handle, int id, unsigned int vcpu, virVcpuInfoPtr ipt, op.u.getvcpumapd5.cpumap.nr_cpus = maplen * 8; } ret = xenHypervisorDoV2Dom(handle, &op); - if (unlock_pages(cpumap, maplen) < 0) { - virXenError(VIR_ERR_XEN_CALL, " release"); + if (unlock_pages(cpumap, maplen) < 0) ret = -1; - } } } else { int mapl = maplen; @@ -2079,8 +2071,9 @@ xenHypervisorInit(struct xenHypervisorVersions *override_versions) */ hv_versions.hypervisor = -1; - virXenError(VIR_ERR_XEN_CALL, " ioctl %lu", - (unsigned long) IOCTL_PRIVCMD_HYPERCALL); + virReportSystemError(errno, + _("Unable to issue hypervisor ioctl %lu"), + (unsigned long)IOCTL_PRIVCMD_HYPERCALL); VIR_FORCE_CLOSE(fd); in_init = 0; return -1; @@ -2178,14 +2171,15 @@ xenHypervisorInit(struct xenHypervisorVersions *override_versions) goto done; } + /* * we failed to make the getdomaininfolist hypercall */ - - VIR_DEBUG("Failed to find any Xen hypervisor method"); hv_versions.hypervisor = -1; - virXenError(VIR_ERR_XEN_CALL, " ioctl %lu", - (unsigned long)IOCTL_PRIVCMD_HYPERCALL); + virReportSystemError(errno, + _("Unable to issue hypervisor ioctl %lu"), + (unsigned long)IOCTL_PRIVCMD_HYPERCALL); + VIR_DEBUG("Failed to find any Xen hypervisor method"); VIR_FORCE_CLOSE(fd); in_init = 0; VIR_FREE(ipt); -- 1.7.10.4