Re: [libvirt] [PATCH 5/5] qemu/rbd: improve rbd device specification

2011-10-12 Thread Daniel P. Berrange
On Mon, Sep 19, 2011 at 09:13:43PM -0700, Sage Weil wrote:
 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
 authenticate.  The id property is the Ceph/RBD user to authenticate as,
 and the domain property is a identifier (local to libvirt) for the Ceph
 cluster in question.  If both are specified, we look for a libvirt secret
 of type CEPH with matching id and domain properties.
 
 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 unit tests
 accordingly.
 
 Signed-off-by: Sage Weil s...@newdream.net
 ---
  src/qemu/qemu_command.c|  268 
 +++-
  .../qemuxml2argv-disk-drive-network-rbd.args   |6 +-
  .../qemuxml2argv-disk-drive-network-rbd.xml|1 +
  3 files changed, 157 insertions(+), 118 deletions(-)
 +static int buildRBDString(virConnectPtr conn,
 +  virDomainDiskDefPtr disk,
 +  virBufferPtr opt)
 +{
 +int i;
 +char idDomain[80];
 +virSecretPtr sec;
 +char *secret;
 +size_t secret_size;
 +
 +virBufferAsprintf(opt, rbd:%s, disk-src);
 +if (disk-authId) {
 +virBufferEscape(opt, :, :id=%s, disk-authId);
 +}
 +if (disk-authDomain) {
 +/* look up secret */
 +snprintf(idDomain, sizeof(idDomain), %s/%s, disk-authId,
 + disk-authDomain);
 +sec = virSecretLookupByUsage(conn,
 + VIR_SECRET_USAGE_TYPE_CEPH,
 + idDomain);

This is where you'd want to use  virSecretLookupByUUID.

 +if (sec) {
 +char *base64;
 +
 +secret = (char *)conn-secretDriver-getValue(sec, secret_size, 
 0,
 +   VIR_SECRET_GET_VALUE_INTERNAL_CALL);

No need for the cast to 'char *', since void * casts to anything in C.

But you do need to handle case of the return'd secret being 'NULL'
and return to the caller in that case.

 +/* qemu/librbd wants it base64 encoded */
 +base64_encode_alloc(secret, secret_size, base64);
 +virBufferEscape(opt, :, :key=%s:auth_supported=cephx\\;none,
 +base64);
 +VIR_FREE(base64);
 +VIR_FREE(secret);
 +virUnrefSecret(sec);
 +} else {
 +VIR_WARN(rbd id '%s' domain '%s' specified but secret not 
 found,
 + disk-authId, disk-authDomain);

You ought to use  qemuReportError() here and return to the caller

 +}
 +}
 +if (disk-nhosts  0) {
 +virBufferStrcat(opt, :mon_host=, NULL);
 +for (i = 0; i  disk-nhosts; ++i) {
 +if (i) {
 +virBufferStrcat(opt, \\;, NULL);
 +}
 +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;
 +}
 +

 @@ -1453,8 +1594,9 @@ 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);
 +buildRBDString(conn, disk, opt);
 +virBufferStrcat(opt, ,, NULL);
  break;
  case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
  if (disk-nhosts == 0)
 @@ -3597,24 +3715,11 @@ qemuBuildCommandLine(virConnectPtr conn,
  }
  break;
  case VIR_DOMAIN_DISK_PROTOCOL_RBD:
 -if (virAsprintf(file, rbd:%s,, disk-src)  0) {
 -goto no_memory;
 -}
 -for (j = 0 ; j  disk-nhosts ; j++) {
 -if (!has_rbd_hosts) {
 -virBufferAddLit(rbd_hosts, CEPH_ARGS=-m );
 -has_rbd_hosts = true;
 -} else {
 -virBufferAddLit(rbd_hosts, ,);
 -}
 -virDomainDiskHostDefPtr host = disk-hosts[j];
 -if (host-port) {
 -virBufferAsprintf(rbd_hosts, %s:%s,
 -  host-name,
 -  

Re: [libvirt] [PATCH 5/5] qemu/rbd: improve rbd device specification

2011-09-16 Thread Sage Weil
On Fri, 16 Sep 2011, Tommi Virtanen wrote:
 On Thu, Sep 15, 2011 at 13:52, Sage Weil s...@newdream.net wrote:
  +static int buildRBDString(virConnectPtr conn,
 ...
  +        /* look up secret */
  +        snprintf(idDomain, sizeof(idDomain), %s/%s, disk-authId,
  +                 disk-authDomain);
  +        sec = virSecretLookupByUsage(conn,
  +                                     VIR_SECRET_USAGE_TYPE_CEPH,
  +                                     idDomain);
 ...
  +            secret = (char *)conn-secretDriver-getValue(sec, 
  secret_size, 0,
  +                                   VIR_SECRET_GET_VALUE_INTERNAL_CALL);
  +            /* qemu/librbd wants it base64 encoded */
  +            base64_encode_alloc(secret, secret_size, base64);
  +            virBufferEscape(opt, :, 
  :key=%s:auth_supported=cephx\\;none,
  +                            base64);
 
 If I'm reading this right, that puts the ceph secret on the kvm
 command line. That's not good, that makes it visible to anyone on the
 host.

Yeah, we definitely want something better, but I wanted to make sure the 
overall approach is fine before doing something too annoying with 
temporary unlinked files or environment variables or something.

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

Re: [libvirt] [PATCH 5/5] qemu/rbd: improve rbd device specification

2011-09-16 Thread Tommi Virtanen
On Fri, Sep 16, 2011 at 10:01, Sage Weil s...@newdream.net wrote:
 If I'm reading this right, that puts the ceph secret on the kvm
 command line. That's not good, that makes it visible to anyone on the
 host.
 Yeah, we definitely want something better, but I wanted to make sure the
 overall approach is fine before doing something too annoying with
 temporary unlinked files or environment variables or something.

That sounds good. Except for the environment variables part; while
some effort is put into guarding the environment, there have been
several ways of reading other processes' environment, historically. I
wouldn't rely on environment to stay secret, ever.

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


Re: [libvirt] [PATCH 5/5] qemu/rbd: improve rbd device specification

2011-09-16 Thread Tommi Virtanen
On Thu, Sep 15, 2011 at 13:52, Sage Weil s...@newdream.net wrote:
 +static int buildRBDString(virConnectPtr conn,
...
 +        /* look up secret */
 +        snprintf(idDomain, sizeof(idDomain), %s/%s, disk-authId,
 +                 disk-authDomain);
 +        sec = virSecretLookupByUsage(conn,
 +                                     VIR_SECRET_USAGE_TYPE_CEPH,
 +                                     idDomain);
...
 +            secret = (char *)conn-secretDriver-getValue(sec, secret_size, 
 0,
 +                                   VIR_SECRET_GET_VALUE_INTERNAL_CALL);
 +            /* qemu/librbd wants it base64 encoded */
 +            base64_encode_alloc(secret, secret_size, base64);
 +            virBufferEscape(opt, :, :key=%s:auth_supported=cephx\\;none,
 +                            base64);

If I'm reading this right, that puts the ceph secret on the kvm
command line. That's not good, that makes it visible to anyone on the
host.

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