On 01/22/2014 08:21 AM, joel SIMOES wrote:
> From: Joel SIMOES <joel.simoes@xxxxxxxxxxx>
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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list