[libvirt] [PATCH v2 02/12] qemu: Introduce qemuDomainSecretPrepare and Destroy

[John Ferlan]

[...]

>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 26c19ff..24ed8ed 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -832,7 +832,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>>  
>>  int
>>  qemuGetDriveSourceString(virStorageSourcePtr src,
>> -                         virConnectPtr conn,
>> +                         qemuDomainSecretInfoPtr secinfo,
>>                           char **source)
>>  {
>>      int actualType = virStorageSourceGetActualType(src);
>> @@ -846,31 +846,6 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
>>      if (virStorageSourceIsEmpty(src))
>>          return 1;
>>  
>> -    if (conn) {
>> -        if (actualType == VIR_STORAGE_TYPE_NETWORK &&
>> -            src->auth &&
>> -            (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
>> -             src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
>> -            bool encode = false;
>> -            int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
>> -            const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
>> -            username = src->auth->username;
>> -
>> -            if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
>> -                /* qemu requires the secret to be encoded for RBD */
>> -                encode = true;
>> -                secretType = VIR_SECRET_USAGE_TYPE_CEPH;
>> -            }
>> -
>> -            if (!(secret = virSecretGetSecretString(conn,
>> -                                                    protocol,
>> -                                                    encode,
>> -                                                    src->auth,
>> -                                                    secretType)))
>> -                goto cleanup;
>> -        }
>> -    }
>> -

This whole pile is what moves into qemuDomainSecretDiskPrepare. The
check for 'conn' was here it seems was a result of commit id '816f0f93'
where qemuDomainSnapshotCreateSingleDiskActive calls
qemuGetDriveSourceString.

>>      switch ((virStorageType) actualType) {
>>      case VIR_STORAGE_TYPE_BLOCK:
>>      case VIR_STORAGE_TYPE_FILE:
>> @@ -881,6 +856,11 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
>>          break;
>>  
>>      case VIR_STORAGE_TYPE_NETWORK:
>> +        if (secinfo) {
>> +            username = secinfo->s.plain.username;
>> +            secret = secinfo->s.plain.secret;
>> +        }
>> +

[...]

>> +/* qemuDomainSecretDiskPrepare:
>> + * @conn: Pointer to connection
>> + * @disk: Pointer to a disk definition
>> + *
>> + * For the right disk, generate the qemuDomainSecretInfo structure.
>> + *
>> + * Returns 0 on success, -1 on failure
>> + */
>> +int
>> +qemuDomainSecretDiskPrepare(virConnectPtr conn,
>> +                            virDomainDiskDefPtr disk)
>> +{
>> +    virStorageSourcePtr src = disk->src;
>> +    qemuDomainSecretInfoPtr secinfo = NULL;
>> +
>> +    if (conn && !virStorageSourceIsEmpty(src) &&
>> +        virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK &&
>> +        src->auth &&
>> +        (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
>> +         src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
>> +
> 
> Is conn ever going to be NULL here?
> 

This code is the former qemuGetDriveSourceString code... So it was
mostly a "move" of code. I was being more cautious due to number of callers.

I did find one path that explicitly passes NULL:

testQemuHotplugAttach -> qemuDomainAttachDeviceDiskLive ->
qemuDomainAttachDeviceDiskLive ... which can call either
qemuDomainAttachVirtioDiskDevice or qemuDomainAttachSCSIDisk which would
call the qemuDomainSecretDiskPrepare

And one comment in qemuAutostartDomains that I suppose could be updated
to indicate the conn is also possibly needed for secrets.


>> +        qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>> +
>> +        if (VIR_ALLOC(secinfo) < 0)
>> +            return -1;
>> +
>> +        if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
>> +                                       src->auth) < 0)
>> +            goto error;
>> +
>> +        diskPriv->secinfo = secinfo;
>> +    }
>> +
>> +    return 0;
>> +
>> + error:
>> +    qemuDomainSecretInfoFree(&secinfo);
>> +    return -1;
>> +}
>> +
>> +

[...]

>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index ef8696b..692e3e7 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -148,7 +148,6 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
>>  /**
>>   * qemuDomainChangeEjectableMedia:
>>   * @driver: qemu driver structure
>> - * @conn: connection structure
>>   * @vm: domain definition
>>   * @disk: disk definition to change the source of
>>   * @newsrc: new disk source to change to
>> @@ -163,7 +162,6 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
>>   */
>>  int
>>  qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>> -                               virConnectPtr conn,
>>                                 virDomainObjPtr vm,
>>                                 virDomainDiskDefPtr disk,
>>                                 virStorageSourcePtr newsrc,
>> @@ -223,7 +221,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>>      } while (rc < 0);
>>  
>>      if (!virStorageSourceIsEmpty(newsrc)) {
>> -        if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0)
>> +        /* cdrom/floppy won't have secret */
>> +        if (qemuGetDriveSourceString(newsrc, NULL, &sourcestr) < 0)
>>              goto error;
>>  
> 
> Why not? Can't you have an rbd backed cdrom device?
> 

Trying to remember why I wrote that comment... Ahh I see... I must've
read the return virStorageSourceIsEmpty incorrectly when I changed that
code... good catch!

I will insert/squash in a:

    qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);

    and change NULL to diskPriv->secinfo



John

> ACK otherwise
> 
> - Cole
>