[libvirt] [PATCH 08/11] Check whether pools are already active upon libvirtd startup

[Eric Blake]

On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
> When libvirt starts up all storage pools default to the inactive
> state, even if the underlying storage is already active on the
> host. This introduces a new API into the internal storage backend
> drivers that checks whether a storage pool is already active. If
> the pool is active at libvirtd startup, the volume list will be
> immediately populated.
> 

> +++ b/src/storage/storage_backend_mpath.c
> @@ -27,6 +27,8 @@
>  #include <stdio.h>
>  #include <dirent.h>
>  #include <fcntl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>

<sys/types.h> is generally redundant, now that POSIX 2008 requires most
headers to be self-contained.

> +static int
> +virStorageBackendMpathCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +                                bool *isActive)
> +{
> +    const char *path = "/dev/mpath";
> +
> +    *isActive = false;
> +
> +    struct stat sb;
> +    if (stat(path, &sb) == 0)
> +        *isActive = true;

General observation: using stat() for existence checks is generally
slower than access(,F_OK), because the kernel has to do more work to
populate the result buffer that you then ignore.  While what you have
works, maybe you should consider rewriting this to use access().

> +++ b/src/storage/storage_backend_scsi.c
> @@ -27,6 +27,9 @@
>  #include <stdio.h>
>  #include <dirent.h>
>  #include <fcntl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>

Why <sys/wait.h>?

> +++ b/src/storage/storage_driver.c
> @@ -68,17 +68,29 @@ storageDriverAutostart(virStorageDriverStatePtr driver) {
>  
>      for (i = 0 ; i < driver->pools.count ; i++) {
>          virStoragePoolObjPtr pool = driver->pools.objs[i];
> +        virStorageBackendPtr backend;
> +        bool started = false;
>  
>          virStoragePoolObjLock(pool);
> -        if (pool->autostart &&
> -            !virStoragePoolObjIsActive(pool)) {
> -            virStorageBackendPtr backend;
> -            if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
> -                VIR_ERROR(_("Missing backend %d"), pool->def->type);
> -                virStoragePoolObjUnlock(pool);
> -                continue;
> -            }
> +        if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
> +            VIR_ERROR(_("Missing backend %d"), pool->def->type);
> +            virStoragePoolObjUnlock(pool);
> +            continue;
> +        }
>  
> +        if (backend->checkPool &&
> +            backend->checkPool(NULL, pool, &started) < 0) {
> +            virErrorPtr err = virGetLastError();
> +            VIR_ERROR(_("Failed to initialize storage pool '%s': %s"),
> +                      pool->def->name, err ? err->message :
> +                      "no error message found");

Missing _() around that last string.

ACK to the overall idea, but you may want to respin this given my comments.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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