On Thu, Sep 13, 2012 at 10:31:09AM -0600, Eric Blake wrote:
> On 09/11/2012 08:11 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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?
Sigh, I don't know what I was thinking. The original idea of open coding
it was that I needed to stat() the binary to get the modification time.
Calling virFileIsExecutable() would then repeat the stat() again. I was
trying to avoid that duplicated call. For unknown reasons I then left
the virFileIsExecutable() call in there.
To be honest this was all just premature optimization, so I'll remove
the open coded check and just live with the duplicated stat() call,
as this isn't performance critical.
>
> > +
> > + /* 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.
I'll delete the open coded calls. I don't think we need sub-second
time resolution though.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list