Re: [PATCH 4/9] Implement translateDiskSourcePool

[Osier Yang <jyang@xxxxxxxxxx>]

On 2013年02月01日 01:42, John Ferlan wrote:
> On 01/30/2013 01:11 PM, Osier Yang wrote:
> > It iterates over all the domain disks, and translate the source of
> > all the disks of 'volume' type from storage pool/volume to the real
> > underlying source.
> >
> > Disks of type 'file', 'block', and 'dir' are supported now. Network
> > type is not supported yet, it will be another patch.
> >
> > src/storage/storage_driver.c:
> >    * New helper storagePoolObjFindByDiskSource to find the specified
> >      pool by name.
> >    * Implement translateDiskSourcePool as storageTranslateDomainDiskSourcePool
> > ---
> >   src/storage/storage_driver.c |   90 ++++++++++++++++++++++++++++++++++++++++++
> >   1 files changed, 90 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> > index e98c18c..9f60f2c 100644
> > --- a/src/storage/storage_driver.c
> > +++ b/src/storage/storage_driver.c
> > @@ -47,6 +47,8 @@
> >   #include "virlog.h"
> >   #include "virfile.h"
> >   #include "fdstream.h"
> > +#include "domain_conf.h"
> > +#include "domain_storage.h"
> >   #include "configmake.h"
> >
> >   #define VIR_FROM_THIS VIR_FROM_STORAGE
> > @@ -2367,6 +2369,88 @@ storageListAllPools(virConnectPtr conn,
> >       return ret;
> >   }
> >
> > +static virStoragePoolObjPtr
> > +storagePoolObjFindByDiskSource(virConnectPtr conn,
> > +                               const char *name)
> > +{
> > +    virStorageDriverStatePtr driver = conn->storagePrivateData;
> > +    virStoragePoolObjPtr pool = NULL;
> > +
> > +    storageDriverLock(driver);
> > +    pool = virStoragePoolObjFindByName(&driver->pools, name);
> > +    storageDriverUnlock(driver);
> > +
> > +    if (!pool) {
> > +        virReportError(VIR_ERR_NO_STORAGE_POOL,
> > +                       _("no storage pool with matching name '%s'"),
> > +                       name);
> > +        goto error;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(pool)) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       _("The specified pool '%s' is not active"),
> > +                       name);
> > +        goto error;
> > +    }
> > +
> > +    return pool;
> > +error:
> > +    if (pool)
> > +        virStoragePoolObjUnlock(pool);
> > +    return NULL;
> > +}
> > +
> > +static int
> > +storageTranslateDomainDiskSourcePool(virConnectPtr conn,
> > +                                     virDomainDefPtr def)
> > +{
> > +    virStoragePoolObjPtr pool = NULL;
> > +    virStorageVolDefPtr vol = NULL;
> > +    int i;
> > +    int ret = -1;
> > +
> > +    for (i = 0; i<  def->ndisks; i++) {
> > +        virDomainDiskDefPtr disk = def->disks[i];
> > +
> > +        if (disk->type != VIR_DOMAIN_DISK_TYPE_VOLUME)
> > +            continue;
> > +
> > +        if (!(pool = storagePoolObjFindByDiskSource(conn, disk->srcpool->pool)))
> > +            goto cleanup;
> > +
> > +        if (!(vol = virStorageVolDefFindByName(pool, disk->srcpool->volume))) {
> > +            virReportError(VIR_ERR_NO_STORAGE_VOL,
> > +                           _("no storage vol of specified pool with "
> > +                             "matching name '%s'"),
> > +                           disk->srcpool->volume);
> s/vol/volume
>
> Since you have the pool name, wouldn't it be better to state that volume
> "%s" was not found in pool "%s"?

Agreed.

> Although I now see you just hoisted the message from elsewhere.
>
> > +            goto cleanup;
> > +        }
> > +
> > +        switch (vol->type) {
> > +        case VIR_STORAGE_VOL_FILE:
> > +        case VIR_STORAGE_VOL_BLOCK:
> > +        case VIR_STORAGE_VOL_DIR:
> > +            disk->src = strdup(vol->target.path);
>
> And if !disk->src here what assumptions get broken later?

Hum, this inspires me notice vol->target.path can be NULL (it's not
mandatory in vol XML). And it should error out as long as the the disk
is not CD-ROM or Floppy.

But I don't think it's neccesary to check the return value of strdup.

Will post v2.

> > +            break;
> > +        case VIR_STORAGE_VOL_NETWORK:
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("Using network volume as disk source is not supported"));
> > +            goto cleanup;
> > +        }
> > +
> > +        virStoragePoolObjUnlock(pool);
> > +        pool = NULL;
> > +    }
> > +
> > +    ret = 0;
> > +
> > +cleanup:
> > +    if (pool)
> > +        virStoragePoolObjUnlock(pool);
> > +    return ret;
> > +}
> > +
> >   static virStorageDriver storageDriver = {
> >       .name = "storage",
> >       .open = storageOpen, /* 0.4.0 */
> > @@ -2423,8 +2507,14 @@ static virStateDriver stateDriver = {
> >       .reload = storageDriverReload,
> >   };
> >
> > +static virDomainStorageDriver domainStorageDriver = {
> > +    .translateDiskSourcePool = storageTranslateDomainDiskSourcePool,
> > +};
> > +
> > +
> >   int storageRegister(void) {
> >       virRegisterStorageDriver(&storageDriver);
> >       virRegisterStateDriver(&stateDriver);
> > +    virRegisterDomainStorageDriver(&domainStorageDriver);
> >       return 0;
> >   }
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list