[libvirt] [PATCH] Using qemu with sheepdog pool

[Eric Blake]

On 01/22/2014 08:21 AM, joel SIMOES wrote:
> From: Joel SIMOES <joel.simoes at laposte.net>

A bit more detail in the commit message body might be nice.

> 
> ---
>  src/qemu/qemu_conf.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index ac53f6d..dfafcdc 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1185,6 +1185,56 @@ int qemuDriverAllocateID(virQEMUDriverPtr driver)
>      return virAtomicIntInc(&driver->nextvmid);
>  }
>  
> +
> +static int
> +qemuAddSheepPoolSourceHost(virDomainDiskDefPtr def,
> +                           virStoragePoolDefPtr pooldef)
> +{
> +    int ret = -1;
> +    char **tokens = NULL;
> +
> +    /* Only support one host */
> +    if (pooldef->source.nhost != 1) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Expected exactly 1 host for the storage pool"));
> +        goto cleanup;
> +    }
> +
> +    /* iscsi pool only supports one host */

Incorrect copy-and-paste?

> +    def->nhosts = 1;
> +
> +    if (VIR_ALLOC_N(def->hosts, def->nhosts) < 0)
> +        goto cleanup;
> +
> +    if (VIR_STRDUP(def->hosts[0].name, pooldef->source.hosts[0].name) < 0)
> +        goto cleanup;
> +
> +    if (virAsprintf(&def->hosts[0].port, "%d",
> +                    pooldef->source.hosts[0].port ?
> +                    pooldef->source.hosts[0].port :
> +                    7000) < 0)
> +        goto cleanup;
> +
> +    
> +
> +    
> +

Too much whitespace, including trailing whitespace.  Ah, _this_ is the
patch where you later posted an unthreaded cleanup patch; it's better to
squash that into this and post as a v2.


> -
> -    if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) {
> +    
> +    if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI && !(def->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT && pooldef->type == VIR_STORAGE_POOL_SHEEPDOG) ) {


Long line.  I'd wrap it as:

   if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI &&
      !(def->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT &&
        pooldef->type == VIR_STORAGE_POOL_SHEEPDOG)) {
> +     
>          virReportError(VIR_ERR_XML_ERROR, "%s",
>                         _("disk source mode is only valid when "
> -                         "storage pool is of iscsi type"));
> +                         "storage pool is of iscsi type or only direct for sheepdog "));

Trailing whitespace in the message printed to the user.

> -    case VIR_STORAGE_POOL_MPATH:
> -    case VIR_STORAGE_POOL_RBD:
>      case VIR_STORAGE_POOL_SHEEPDOG:
> +        def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;
> +        // force direct mode

We prefer /**/ comments.

I think you're on the right track, and wish I knew more about sheepdog
to actually test these patches.  But hopefully this review has been
helpful, and I look forward to seeing a cleaner v2.

-- 
Eric Blake   eblake 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: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140124/d760108e/attachment-0001.sig>