Re: [libvirt] [PATCH 11/20] Introduce a API for creating QEMU capabilities for a binary

On 09/11/2012 08:11 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Introduce a qemuCapsNewForBinary() API which creates a new > QEMU capabilities object, populated with data relating to > a specific QEMU binary. The qemuCaps object is also given > a timestamp, which makes it possible to detect when the > cached capabilities for a binary are out of date > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > src/qemu/qemu_capabilities.c | 142 +++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_capabilities.h | 3 + > 2 files changed, 145 insertions(+) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 97aeac7..e8b5797 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -181,6 +181,9 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, > struct _qemuCaps { > virObject object; > > + char *binary; > + time_t mtime; Do we care about subsecond resolution (that is, should this be struct timespec instead of time_t)? We already have gnulib functions imported that guarantee we can extract timespec from any stat() call, even on systems limited to second resolution. mtime can be faked; do we want to use ctime instead? > +qemuCapsPtr qemuCapsNewForBinary(const char *binary) > +{ > + /* We would also want to check faccessat if we cared about ACLs, > + * but we don't. */ > + if (stat(binary, &sb) < 0) { > + virReportSystemError(errno, _("Cannot check QEMU binary %s"), > + binary); > + goto error; > + } > + if (!(S_ISREG(sb.st_mode) && (sb.st_mode & 0111) != 0)) { Poor-man's access(binary, X_OK), but I can live with it (not to mention it is copied from somewhere else, and this is more of a refactoring). On the other hand... > + errno = S_ISDIR(sb.st_mode) ? EISDIR : EACCES; > + virReportSystemError(errno, _("QEMU binary %s is not executable"), > + binary); > + goto error; > + } > + caps->mtime = sb.st_mtime; Here, if you include "stat-time.h" and use my struct-timespec hint, this would be caps->mtime = get_stat_mtime(&st). And again, should we be favoring ctime instead? > + > + /* Make sure the binary we are about to try exec'ing exists. > + * Technically we could catch the exec() failure, but that's > + * in a sub-process so it's hard to feed back a useful error. > + */ > + if (!virFileIsExecutable(binary)) { > + goto error; > + } ...Isn't this just repeating the open-coded st.st_mode&0111 and S_ISDIR checks done earlier? Can we simplify the code by doing this check alone, and dropping the open-coding?

> + > + /* S390 and probably other archs do not support no-acpi - Code copying, but s/archs/arches/ while you are here. > + > +bool qemuCapsIsValid(qemuCapsPtr caps) > +{ > + struct stat sb; > + > + if (!caps->binary) > + return true; > + > + if (stat(caps->binary, &sb) < 0) > + return false; > + > + return sb.st_mtime == caps->mtime; with my earlier comments, this would be: return get_stat_[mc]time(&sb) == caps->[mc]time This is mostly copying existing code in preparation for deleting the original sites in favor of this code in later patches, so it's up to you whether to tweak push as-is with a later cleanup commit, or to post a v2 that fixes my comments in one commit. Weak ACK.