[libvirt] [PATCH 0/3] Various build fixes
Fix some problems coverity and mingw build found out. Martin Kletzander (3): qemu: Add missing goto error in qemuRestoreCgroupState qemu: Free saved error in qemuDomainSetVcpusFlags util: Fix fallocate stubs for mingw build src/qemu/qemu_cgroup.c | 1 + src/qemu/qemu_driver.c | 1 + src/util/virfile.c | 38 +++++++++++++++++++++++++++++++------- 3 files changed, 33 insertions(+), 7 deletions(-) -- 2.2.0
Commit af2a1f05 tried clearly separating each condition in qemuRestoreCgroupState() for the sake of readability, however somehow one condition body was missing. That means that the body of the next condition got executed only if both of there were true, which is impossible, thus resulting in a dead code and a logic error. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1e383c4..164ad05 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -801,6 +801,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) if ((empty = virCgroupHasEmptyTasks(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) < 0) + goto error; if (!empty) goto error; -- 2.2.0
On 12/16/2014 12:13 PM, Martin Kletzander wrote:
Commit af2a1f05 tried clearly separating each condition in qemuRestoreCgroupState() for the sake of readability, however somehow one condition body was missing. That means that the body of the next condition got executed only if both of there were true, which is impossible, thus resulting in a dead code and a logic error.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1e383c4..164ad05 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -801,6 +801,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
if ((empty = virCgroupHasEmptyTasks(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) < 0) + goto error;
if (!empty) goto error;
Why not the shorter: if ((empty - virCgroupHasEmptyTasks(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) <= 0) goto error; ACK to either style of fix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 16, 2014 at 12:29:39PM -0700, Eric Blake wrote:
On 12/16/2014 12:13 PM, Martin Kletzander wrote:
Commit af2a1f05 tried clearly separating each condition in qemuRestoreCgroupState() for the sake of readability, however somehow one condition body was missing. That means that the body of the next condition got executed only if both of there were true, which is impossible, thus resulting in a dead code and a logic error.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1e383c4..164ad05 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -801,6 +801,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
if ((empty = virCgroupHasEmptyTasks(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) < 0) + goto error;
if (!empty) goto error;
Why not the shorter:
if ((empty - virCgroupHasEmptyTasks(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) <= 0) goto error;
ACK to either style of fix.
As I said in the commit message, I wanted to clearly separate the conditions for the sake of readability. But you're right, because that bit me right back, I'll go with '<='. Thanks for the review, I pushed the series 9also with the fix in 3/3). Martin
Commit e3435caf added cleanup code to qemuDomainSetVcpusFlags() that was not supposed to reset the error. Usual procedure was done, saving the error to temporary variable, but it was never free'd, but rather leaked. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7211d42..9aba4c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4781,6 +4781,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, err = virSaveLastError(); virCgroupSetCpusetMems(cgroup_temp, mem_mask); virSetError(err); + virFreeError(err); } if (!qemuDomainObjEndJob(driver, vm)) -- 2.2.0
On 12/16/2014 12:13 PM, Martin Kletzander wrote:
Commit e3435caf added cleanup code to qemuDomainSetVcpusFlags() that was not supposed to reset the error. Usual procedure was done, saving the error to temporary variable, but it was never free'd, but rather leaked.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+)
ACK
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7211d42..9aba4c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4781,6 +4781,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, err = virSaveLastError(); virCgroupSetCpusetMems(cgroup_temp, mem_mask); virSetError(err); + virFreeError(err); }
if (!qemuDomainObjEndJob(driver, vm))
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When any of the functions modified in commit 214c687b took false branch, the function itself used none of its parameters resulting in "unused parameter" error. Rewriting these functions to the stubs we use elsewhere should fix the problem. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virfile.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 4483cce..2076860 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1037,36 +1037,52 @@ safewrite(int fd, const void *buf, size_t count) return nwritten; } +#ifdef HAVE_POSIX_FALLOCATE static int safezero_posix_fallocate(int fd, off_t offset, off_t len) { -#ifdef HAVE_POSIX_FALLOCATE int ret = posix_fallocate(fd, offset, len); if (ret == 0) return 0; errno = ret; -#endif return -1; } +#else /* !HAVE_POSIX_FALLOCATE */ +static int +safezero_posix_fallocate(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED) +{ + return -1; +} +#endif /* !HAVE_POSIX_FALLOCATE */ +#if HAVE_SYS_SYSCALL_H && defined(SYS_fallocate) static int safezero_sys_fallocate(int fd, off_t offset, off_t len) { int rc = -1; -#if HAVE_SYS_SYSCALL_H && defined(SYS_fallocate) rc = syscall(SYS_fallocate, fd, 0, offset, len); -#else + return rc; +} +#else /* !HAVE_SYS_SYSCALL_H || !defined(SYS_fallocate) */ +static int +safezero_sys_fallocate(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED) +{ + int rc = -1; errno = ENOSYS; -#endif return rc; } +#endif /* !HAVE_SYS_SYSCALL_H || !defined(SYS_fallocate) */ +#ifdef HAVE_MMAP static int safezero_mmap(int fd, off_t offset, off_t len) { -#ifdef HAVE_MMAP int r; char *buf; static long pagemask; @@ -1095,9 +1111,17 @@ safezero_mmap(int fd, off_t offset, off_t len) /* fall back to writing zeroes using safewrite if mmap fails (for * example because of virtual memory limits) */ -#endif /* HAVE_MMAP */ return -1; } +#else /* !HAVE_MMAP */ +static int +safezero_mmap(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t lenATTRIBUTE_UNUSED) +{ + return -1 +} +#endif /* !HAVE_MMAP */ static int safezero_slow(int fd, off_t offset, off_t len) -- 2.2.0
On 12/16/2014 12:13 PM, Martin Kletzander wrote:
When any of the functions modified in commit 214c687b took false branch, the function itself used none of its parameters resulting in "unused parameter" error. Rewriting these functions to the stubs we use elsewhere should fix the problem.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virfile.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 4483cce..2076860 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1037,36 +1037,52 @@ safewrite(int fd, const void *buf, size_t count) return nwritten; }
+#ifdef HAVE_POSIX_FALLOCATE static int safezero_posix_fallocate(int fd, off_t offset, off_t len) { -#ifdef HAVE_POSIX_FALLOCATE int ret = posix_fallocate(fd, offset, len); if (ret == 0) return 0; errno = ret; -#endif return -1; } +#else /* !HAVE_POSIX_FALLOCATE */ +static int +safezero_posix_fallocate(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED) +{ + return -1;
Odd that we don't set errno on the fallback, but it's pre-existing, so not a problem in your patch.
+safezero_mmap(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t lenATTRIBUTE_UNUSED)
Missing a space; you'd still get a warning that 'lenATTRIBUTE_UNUSED' is not marked unused :) ACK with the space added. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/16/2014 12:13 PM, Martin Kletzander wrote:
When any of the functions modified in commit 214c687b took false branch, the function itself used none of its parameters resulting in "unused parameter" error. Rewriting these functions to the stubs we use elsewhere should fix the problem.
+#else /* !HAVE_MMAP */ +static int +safezero_mmap(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t lenATTRIBUTE_UNUSED)
Shoot. I caught (and you corrected) the missing space...
+{ + return -1
...but we missed the missing ';', which means this code still won't compile for someone without mmap. I'll push the obvious followup. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 16, 2014 at 12:51:22PM -0700, Eric Blake wrote:
On 12/16/2014 12:13 PM, Martin Kletzander wrote:
When any of the functions modified in commit 214c687b took false branch, the function itself used none of its parameters resulting in "unused parameter" error. Rewriting these functions to the stubs we use elsewhere should fix the problem.
+#else /* !HAVE_MMAP */ +static int +safezero_mmap(int fd ATTRIBUTE_UNUSED, + off_t offset ATTRIBUTE_UNUSED, + off_t lenATTRIBUTE_UNUSED)
Shoot. I caught (and you corrected) the missing space...
+{ + return -1
...but we missed the missing ';', which means this code still won't compile for someone without mmap. I'll push the obvious followup.
Oh, thanks for that. That's what I get for rushing into fixing things. I'll step away for a while now :) Martin
participants (2)
-
Eric Blake -
Martin Kletzander