[libvirt] [PATCH v4 4/4] qemu/rbd: improve rbd device specification
Eric Blake
eblake at redhat.com
Mon Oct 31 20:02:26 UTC 2011
On 10/28/2011 03:19 PM, Josh Durgin wrote:
> From: Sage Weil<sage at newdream.net>
>
> This improves the support for qemu rbd devices by adding support for a few
> key features (e.g., authentication) and cleaning up the way in which
> rbd configuration options are passed to qemu.
>
> And<auth> member of the disk source xml specifies how librbd should
s/And/An/
> authenticate. The username attribute is the Ceph/RBD user to authenticate as.
> The usage or uuid attributes specify which secret to use. Usage is an
> arbitrary identifier local to libvirt.
>
> The old RBD support relied on setting an environment variable to
> communicate information to qemu/librbd. Instead, pass those options
> explicitly to qemu. Update the qemu argument parsing and tests
> accordingly.
>
> Signed-off-by: Sage Weil<sage at newdream.net>
> Signed-off-by: Josh Durgin<josh.durgin at dreamhost.com>
> ---
>
> This fixes the things Daniel mentioned.
Needs a v5, to fix memory management issues.
> @@ -1495,6 +1496,159 @@ qemuSafeSerialParamValue(const char *value)
> return 0;
> }
>
> +static int qemuBuildRBDString(virConnectPtr conn,
> + virDomainDiskDefPtr disk,
> + virBufferPtr opt)
Nit: We've been using this style:
static int
qemuBuildRBDString(virConnectPtr conn,
virDomainDiskDefPtr disk,
virBufferPtr opt)
> + if (sec) {
> + char *base64;
> +
> + secret = (char *)conn->secretDriver->getValue(sec,&secret_size, 0,
> + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
> + if (secret == NULL) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not get the value of the secret for username %s"),
> + disk->auth.username);
> + return -1;
Resource leak. You need to ensure virUnrefSecret(sec) gets called on
this path. I'd move that particular cleanup down into the cleanup:
label, and make this part goto cleanup instead of return -1.
> + }
> + /* qemu/librbd wants it base64 encoded */
> + base64_encode_alloc(secret, secret_size,&base64);
> + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
> + base64);
Need to check for allocation failure of base64_encode_alloc; otherwise,
you blindly passed NULL base64 to virBufferEscape, which is not portable.
> + VIR_FREE(base64);
> + VIR_FREE(secret);
> + virUnrefSecret(sec);
> + } else {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("rbd username '%s' specified but secret not found"),
> + disk->auth.username);
> + return -1;
> + }
> + }
> +
> + if (disk->nhosts> 0) {
> + virBufferStrcat(opt, ":mon_host=", NULL);
> + for (i = 0; i< disk->nhosts; ++i) {
> + if (i) {
> + virBufferStrcat(opt, "\\;", NULL);
virBufferAddLit(opt, "\\;")
is more efficient than virBufferStrcat().
> + }
> + if (disk->hosts[i].port) {
> + virBufferAsprintf(opt, "%s\\:%s",
> + disk->hosts[i].name,
> + disk->hosts[i].port);
> + } else {
> + virBufferAsprintf(opt, "%s", disk->hosts[i].name);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
> +{
> + char *port;
> + int ret;
> +
> + disk->nhosts++;
> + ret = VIR_REALLOC_N(disk->hosts, disk->nhosts);
> + if (ret< 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + port = strstr(hostport, "\\:");
> + if (port) {
> + *port = '\0';
> + port += 2;
> + disk->hosts[disk->nhosts-1].port = strdup(port);
> + } else {
> + disk->hosts[disk->nhosts-1].port = strdup("6789");
> + }
> + disk->hosts[disk->nhosts-1].name = strdup(hostport);
These three strdup() need to check for allocation failure.
> + return 0;
> +}
> +
> +/* disk->src initially has everything after the rbd: prefix */
> +static int qemuParseRBDString(virDomainDiskDefPtr disk)
> +{
> + char *options = NULL;
> + char *p, *e, *next;
> +
> + p = strchr(disk->src, ':');
> + if (p) {
> + options = strdup(p + 1);
> + *p = '\0';
Need to check for strdup() failure.
> + }
> +
> + /* options */
> + if (!options)
> + return 0; /* all done */
> +
> + p = options;
> + while (*p) {
> + /* find : delimiter or end of string */
> + for (e = p; *e&& *e != ':'; ++e) {
> + if (*e == '\\') {
> + e++;
> + if (*e == '\0')
> + break;
> + }
> + }
> + if (*e == '\0') {
> + next = e; /* last kv pair */
> + } else {
> + next = e + 1;
> + *e = '\0';
> + }
> +
> + if (STRPREFIX(p, "id=")) {
> + disk->auth.username = strdup(p + strlen("id="));
Check for allocation failure. Also, you might want to see if using
strndup() on the original string is easier than copying the string, just
so you can do in-place modifications of injecting NUL bytes for strdup()
to work.
> + }
> + if (STRPREFIX(p, "mon_host=")) {
> + char *h, *sep;
> +
> + h = p + strlen("mon_host=");
> + while (h< e) {
> + for (sep = h; sep< e; ++sep) {
> + if (*sep == '\\'&& (sep[1] == ',' ||
> + sep[1] == ';' ||
> + sep[1] == ' ')) {
> + *sep = '\0';
> + sep += 2;
> + break;
> + }
> + }
> + qemuAddRBDHost(disk, h);
Don't ignore the return value here.
> + h = sep;
> + }
> + }
> +
> + p = next;
> + }
> + return 0;
> +}
>
> char *
> qemuBuildDriveStr(virConnectPtr conn,
> @@ -1614,8 +1768,10 @@ qemuBuildDriveStr(virConnectPtr conn,
> disk->hosts->name, disk->hosts->port);
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_RBD:
> - /* TODO: set monitor hostnames */
> - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src);
> + virBufferStrcat(&opt, "file=", NULL);
virBufferAddLit(&opt, "file=") is more efficient.
> + if (qemuBuildRBDString(conn, disk,&opt)< 0)
> + goto error;
> + virBufferStrcat(&opt, ",", NULL);
virBufferAddChar(&opt, ',') is more efficient.
> @@ -5299,10 +5418,6 @@ static int qemuStringToArgvEnv(const char *args,
> const char *next;
>
> start = curr;
> - /* accept a space in CEPH_ARGS */
> - if (STRPREFIX(curr, "CEPH_ARGS=-m ")) {
> - start += strlen("CEPH_ARGS=-m ");
> - }
While we aren't generating CEPH_ARGS in the environment any more, I
don't think we should rip out the parsing code. That is, parsing should
be able to handle our older output, in addition to our new output.
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> @@ -0,0 +1,6 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
> +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
> +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
> +file=rbd:pool/image:id=myname:key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
That's a rather long line - I'd suggest using backslash-newline to break
it up, probably at each ':' (it's not the end of the world if you go
longer than 80 columns, if absolutely necessary, but life is easier when
everything just fits).
> +static virSecretPtr
> +fakeSecretLookupByUsage(virConnectPtr conn,
> + int usageType ATTRIBUTE_UNUSED,
> + const char *usageID)
> +{
> + virSecretPtr ret = NULL;
> + int err;
> + if (STRNEQ(usageID, "mycluster_myname"))
> + return NULL;
> + err = VIR_ALLOC(ret);
> + if (err< 0)
> + return NULL;
> + ret->magic = VIR_SECRET_MAGIC;
> + ret->conn = conn;
> + conn->refs++;
> + ret->refs = 1;
> + ret->usageID = strdup(usageID);
Check for strdup() failure.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list