On 09/22/2017 11:12 AM, John Ferlan wrote: > > > On 09/21/2017 10:31 AM, Peter Krempa wrote: >> On Fri, Sep 15, 2017 at 20:30:09 -0400, John Ferlan wrote: >>> Since the secret information is really _virStorageSource specific >>> piece of data, let's create a privateData object for _virStorageSource >>> and move the @secinfo from _qemuDomainDiskPrivate into a new >>> _qemuDomainDiskSrcPrivate structure and manage it from there. >> >> This would be easier to review if you'd split the private data addition >> and the movement of the auth data to it. >> >>> >>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >>> --- >>> src/conf/domain_conf.c | 5 +++++ >>> src/conf/domain_conf.h | 1 + >>> src/qemu/qemu_command.c | 6 ++++-- >>> src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++------ >>> src/qemu/qemu_domain.h | 17 +++++++++++---- >>> src/qemu/qemu_hotplug.c | 11 +++++++--- >>> src/util/virstoragefile.c | 1 + >>> src/util/virstoragefile.h | 3 +++ >>> 8 files changed, 83 insertions(+), 15 deletions(-) >>> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> index 542d14ed6..a3900488f 100644 >>> --- a/src/conf/domain_conf.c >>> +++ b/src/conf/domain_conf.c >>> @@ -1720,6 +1720,11 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) >>> !(ret->privateData = xmlopt->privateData.diskNew())) >>> goto error; >>> >>> + if (xmlopt && >>> + xmlopt->privateData.diskSrcNew && >>> + !(ret->src->privateData = xmlopt->privateData.diskSrcNew())) >>> + goto error; >> >> This certainly is not enough. The virStorageSource structure needs to >> allocate it in it's helper. This will only fix the top level one and not >> the nested/chained ones for the backing store. >> > > Just so it's clear what you're asking... > > Create virStorageSourceNew or virDomainDiskSourceDefNew? > > Going with virStorageSourceNew would seem more correct; however, then it > should live in virstoragefile.c and thus wouldn't be able to use the > domain_conf infrastructure. Seems like it's own infrastructure would > need to be build. It would work with the virStorageSourceNewFromBacking* > which I believe you are referencing. > > Going with virDomainDiskSourceDefNew would follow the other models and > be able to live in domain_conf making it easy for the domain_conf > privateData manipulation code, but it just doesn't feel right. > > John > nevermind - I figured it out... Going with a domain_conf allocator for _virStorageSource is mechanism. It'll even be usable for hostdev. I have patches that I'll post tomorrow - just need to check that I've covered the various review comments. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
