[libvirt] [PATCH v4 1/3] Add a privileged field to storageDriverState

[John Ferlan]

On 07/16/2013 05:31 AM, Daniel P. Berrange wrote:
> On Tue, Jul 16, 2013 at 09:44:52AM +0100, Daniel P. Berrange wrote:
>> On Mon, Jul 15, 2013 at 04:26:10PM -0400, John Ferlan wrote:
>>> Use the privileged value in order to generate a connection which could
>>> be passed to the various storage backend drivers.
>>>
>>> In particular, the iSCSI driver will need a connect in order to perform
>>> pool authentication using the 'chap' secrets.  Additionally, the RBD backend
>>> utilizes the connection during pool refresh for pools using 'ceph' secrets.
>>> ---
>>>  src/conf/storage_conf.h      |  1 +
>>>  src/storage/storage_driver.c | 19 +++++++++++++++----
>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
>>> index fd9b2e7..62ff1fd 100644
>>> --- a/src/conf/storage_conf.h
>>> +++ b/src/conf/storage_conf.h
>>> @@ -354,6 +354,7 @@ struct _virStorageDriverState {
>>>  
>>>      char *configDir;
>>>      char *autostartDir;
>>> +    bool privileged;
>>>  };
>>>  
>>>  typedef struct _virStoragePoolSourceList virStoragePoolSourceList;
>>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>>> index a8eb731..f38acef 100644
>>> --- a/src/storage/storage_driver.c
>>> +++ b/src/storage/storage_driver.c
>>> @@ -68,6 +68,13 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver)
>>>  static void
>>>  storageDriverAutostart(virStorageDriverStatePtr driver) {
>>>      size_t i;
>>> +    virConnectPtr conn = NULL;
>>> +
>>> +    if (driverState->privileged)
>>> +        conn = virConnectOpen("qemu:///system");
>>> +    else
>>> +        conn = virConnectOpen("qemu:///session");
>>> +    /* Ignoring NULL conn - let backends decide */
>>
>> Nope, this doesn't fly. The storage driver is shared across many other
>> hypervisor drivers, and we can't assume that the QEMU driver is compiled
>> into libvirt.
>>
>> IIUC, the reason we need the connection is to access the secrets driver.
>> We should probably just make the storage driver directly call into the
>> secrets driver, instead of going via a virConnectPtr object.

Correct - we need access to the secrets driver via a virConnectPtr and
as I pointed out in my response yesterday the reason why I chose the
findPoolSources() as that vehicle.  It was going to be tricky and
controversial to do so in startPool() - first because "choosing" which
driver then ties the storage pool to that driver and second because of
the system vs. session connection.

In using findPoolSources(), I figured the theory would be that disks
within the pool would use the pool's authentication rather than having
their own defined.  I think that had to do with my reading of the <auth>
description in the domain disk definition, where disk defs can have the
<auth> attribute.

In the pool case, I figured one wouldn't have to put an <auth> on a disk
element, rather the disk could be in a pool that had the authentication.
 When first "found" is when the auth would take place rather than when
the pool started.

In the long run while Osier points out in his response that creating a
dependency on findPoolSources being run before startPool is not desired,
see:

https://www.redhat.com/archives/libvir-list/2013-July/msg00910.html

I wasn't viewing things that way.  In order for a source disk to be used
in the pool, how does an application discover it? I was assuming via the
virsh mechanisms or through virConnectFindStoragePoolSources().

Does it matter that the pool is authenticated or that the volumes using
the pool use the pool authentication?  A pool could be authenticated
"early" and never need it again. Elements in and added to or removed
from the pool after that authentication wouldn't be authenticated.  Is
that the design goal?  I don't have the answer in my head since I've
jumped into the middle of this...

> 
> Hmm, that is actually harder than I thought.
> 
> I think perhaps we need a  virConnectOpenDummy() which lets us open
> a connection to the secret's driver, without opening a hypervisor.
> 

Oh joy - what did I get myself into :-)  A secret backdoor, you hear a
gurgling brook to your east... xyzzy.. poof you found what you're
looking for.

John