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

[Eric Blake]

On 09/11/2012 08:11 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> 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 at redhat.com>
> ---
>  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.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120913/0f8e62e0/attachment-0001.sig>