<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <span dir="ltr"><<a href="mailto:jtomko@redhat.com" target="_blank">jtomko@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 01/23/2014 08:45 PM, Adam Walters wrote:<br>
> This patch adds a helper function, qemuAddRBDPoolSourceHost, and<br>
> implements the usage of this function to allow RBD storage pools in QEMU<br>
> domain XML.<br>
><br>
> The new function grabs RBD monitor hosts from the storage pool<br>
> definition and applies them to the domain's disk definition at runtime.<br>
> This function is used by my modifications to qemuTranslateDiskSourcePool<br>
> similar to the function used by the iSCSI code.<br>
><br>
> My modifications to qemuTranslateDiskSourcePool is based heavily on the<br>
> existing iSCSI code, but modified to support RBD install. It will place<br>
> all relevant information into the domain's disk definition at runtime to<br>
> allow access to a RBD storage pool.<br>
><br>
> Signed-off-by: Adam Walters <<a href="mailto:adam@pandorasboxen.com">adam@pandorasboxen.com</a>><br>
> ---<br>
>  src/qemu/qemu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++-<br>
>  1 file changed, 61 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c<br>
> index ac53f6d..b1a6bfe 100644<br>
> --- a/src/qemu/qemu_conf.c<br>
> +++ b/src/qemu/qemu_conf.c<br>
</div><div class="im">> @@ -1439,8 +1478,29 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,<br>
>         }<br>
>         break;<br>
><br>
> -    case VIR_STORAGE_POOL_MPATH:<br>
>      case VIR_STORAGE_POOL_RBD:<br>
> +        if (def->startupPolicy) {<br>
> +            virReportError(VIR_ERR_XML_ERROR, "%s",<br>
> +                           _("'startupPolicy' is only valid for "<br>
> +                             "'file' file volume"));<br>
> +            goto cleanup;<br>
> +        }<br>
<br>
</div>You should also check if srcpool->mode is DIRECT and document that it's valid<br>
for rbd pools in docs/<a href="http://formatdomain.html.in" target="_blank">formatdomain.html.in</a> in the description of the mode<br>
attribute.<br></blockquote><div><br></div><div>One of my early versions of this patch actually did check srcpool->mode (and set it</div><div>to DIRECT if it wasn't set yet), but it was requested that I remove that check since</div>
<div>there was only one mode for RBD pools. The removal of that code is what prompted</div><div>my modification to the secret type checking in another patch in this series. So which</div><div>is the correct method of implementation? Should pools that only have one access</div>
<div>mode check for srcpool->mode to be set (and set a default if there isn't a setting)?</div><div>My original thoughts were that the check would allow for a HOST access mode in the</div><div>future, which might allow use of the kernel rbd module (which creates block devices)</div>
<div>while also knowing the volume was served by Ceph.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
You could also extend the qemuxml2argvtest to check the generated QEMU command<br>
line (as we do for iscsi in qemuxml2argv-disk-source-pool-mode test).<br></blockquote><div><br></div><div>I will certainly look into doing this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im"><br>
> +<br>
> +        def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;<br>
> +        def->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD;<br>
> +<br>
> +        if (!(def->src = virStorageVolGetPath(vol)))<br>
> +            goto cleanup;<br>
> +<br>
> +        if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)<br>
> +            goto cleanup;<br>
> +<br>
> +        if (qemuAddRBDPoolSourceHost(def, pooldef) < 0)<br>
> +            goto cleanup;<br>
> +<br>
> +        break;<br>
> +<br>
> +    case VIR_STORAGE_POOL_MPATH:<br>
>      case VIR_STORAGE_POOL_SHEEPDOG:<br>
>      case VIR_STORAGE_POOL_GLUSTER:<br>
>      case VIR_STORAGE_POOL_LAST:<br>
><br>
<br>
</div>Jan<br>
<br>
</blockquote></div><br></div></div>