[libvirt] [PATCH 00/10] Work In Progress: Refactor dealing with disk source data
This is a work in progress snapshot of my current state to get rid of a few very ugly places where we deal with storage source information while labelling images. The goal of this series will be to get rid of the ugly places and additionally fix problem when block-copying a networked disk which may break badly. Peter Krempa (10): storage: encryption: Add deep copy function for storage encryption util: seclabel: Add deep copy function for device labels util: storagefile: Introduce helper to free storage source perms util: storagesource: Add helper to copy and free storage source seclabels util: storagefile: Add deep copy for struct virStorageSource qemu: cgroup: Add functions to set cgroup image stuff on individual imgs util: Don't require full disk definition when getting imagelabels security: Sanitize type of @migrated in virSecurityManagerRestoreAllLabel security: Rename virSecurityManagerSetImageLabel to *Disk* qemu: snapshot: Improve approach to deal with snapshot metadata src/conf/domain_conf.c | 14 --- src/conf/domain_conf.h | 3 - src/libvirt_private.syms | 6 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 4 +- src/qemu/qemu_cgroup.c | 92 ++++++++++------- src/qemu/qemu_cgroup.h | 5 + src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 121 ++++++---------------- src/qemu/qemu_hotplug.c | 16 +-- src/qemu/qemu_process.c | 2 +- src/security/security_apparmor.c | 8 +- src/security/security_dac.c | 24 ++--- src/security/security_driver.h | 10 +- src/security/security_manager.c | 12 +-- src/security/security_manager.h | 8 +- src/security/security_nop.c | 10 +- src/security/security_selinux.c | 20 ++-- src/security/security_stack.c | 12 +-- src/util/virseclabel.c | 23 +++++ src/util/virseclabel.h | 6 +- src/util/virstorageencryption.c | 40 ++++++++ src/util/virstorageencryption.h | 3 + src/util/virstoragefile.c | 217 +++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 10 ++ 25 files changed, 449 insertions(+), 221 deletions(-) -- 1.9.3
--- src/util/virstorageencryption.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/util/virstorageencryption.h | 3 +++ 2 files changed, 43 insertions(+) diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index 1306490..5a401b7 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -67,6 +67,46 @@ virStorageEncryptionFree(virStorageEncryptionPtr enc) } static virStorageEncryptionSecretPtr +virStorageEncryptionSecretCopy(const virStorageEncryptionSecret *src) +{ + virStorageEncryptionSecretPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + memcpy(ret, src, sizeof(*src)); + + return ret; +} + +virStorageEncryptionPtr +virStorageEncryptionCopy(const virStorageEncryption *src) +{ + virStorageEncryptionPtr ret; + size_t i; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (VIR_ALLOC_N(ret->secrets, src->nsecrets) < 0) + goto error; + + ret->nsecrets = src->nsecrets; + ret->format = src->format; + + for (i = 0; i < src->nsecrets; i++) { + if (!(ret->secrets[i] = virStorageEncryptionSecretCopy(src->secrets[i]))) + goto error; + } + + return ret; + + error: + virStorageEncryptionFree(ret); + return NULL; +} + +static virStorageEncryptionSecretPtr virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, xmlNodePtr node) { diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index f63c9ee..04641b1 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -61,6 +61,9 @@ struct _virStorageEncryption { virStorageEncryptionSecretPtr *secrets; }; +virStorageEncryptionPtr virStorageEncryptionCopy(const virStorageEncryption *src) + ATTRIBUTE_NONNULL(1); + void virStorageEncryptionFree(virStorageEncryptionPtr enc); virStorageEncryptionPtr virStorageEncryptionParseNode(xmlDocPtr xml, -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
--- src/util/virstorageencryption.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/util/virstorageencryption.h | 3 +++ 2 files changed, 43 insertions(+)
ACK. While the overall series might not be in a polished shape yet, you may want to push some of these earlier complete patches to minimize rebase churn. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/util/virseclabel.c | 23 +++++++++++++++++++++++ src/util/virseclabel.h | 6 +++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/util/virseclabel.c b/src/util/virseclabel.c index 5a4d78e..8f07de3 100644 --- a/src/util/virseclabel.c +++ b/src/util/virseclabel.c @@ -80,3 +80,26 @@ virSecurityDeviceLabelDefNew(const char *model) return seclabel; } + + +virSecurityDeviceLabelDefPtr +virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) +{ + virSecurityDeviceLabelDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->norelabel = src->norelabel; + ret->labelskip = src->labelskip; + + if (VIR_STRDUP(ret->model, src->model) < 0 || + VIR_STRDUP(ret->label, src->label) < 0) + goto error; + + return ret; + + error: + virSecurityDeviceLabelDefFree(ret); + return NULL; +} diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 9e970a5..b90d212 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -45,7 +45,7 @@ struct _virSecurityLabelDef { }; -/* Security configuration for domain */ +/* Security configuration for device */ typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef; typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr; struct _virSecurityDeviceLabelDef { @@ -61,6 +61,10 @@ virSecurityLabelDefNew(const char *model); virSecurityDeviceLabelDefPtr virSecurityDeviceLabelDefNew(const char *model); +virSecurityDeviceLabelDefPtr +virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) + ATTRIBUTE_NONNULL(1); + void virSecurityLabelDefFree(virSecurityLabelDefPtr def); void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def); -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
--- src/util/virseclabel.c | 23 +++++++++++++++++++++++ src/util/virseclabel.h | 6 +++++- 2 files changed, 28 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 09b5d10..a23ac6a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1451,6 +1451,17 @@ virStorageNetHostDefFree(size_t nhosts, } +static void +virStoragePermsFree(virStoragePermsPtr def) +{ + if (!def) + return; + + VIR_FREE(def->label); + VIR_FREE(def); +} + + virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts) @@ -1564,10 +1575,7 @@ virStorageSourceClear(virStorageSourcePtr def) virSecurityDeviceLabelDefFree(def->seclabels[i]); VIR_FREE(def->seclabels); } - if (def->perms) { - VIR_FREE(def->perms->label); - VIR_FREE(def->perms); - } + virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); virStorageNetHostDefFree(def->nhosts, def->hosts); -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/19/2014 09:46 AM, Peter Krempa wrote:
It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Why is it that patches 1-3 do not update libvirt_private.syms to add new src/util/vir* APIs, but some of the future patches do update, such as 4, 5, 7? John
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 09b5d10..a23ac6a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1451,6 +1451,17 @@ virStorageNetHostDefFree(size_t nhosts, }
+static void +virStoragePermsFree(virStoragePermsPtr def) +{ + if (!def) + return; + + VIR_FREE(def->label); + VIR_FREE(def); +} + + virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts) @@ -1564,10 +1575,7 @@ virStorageSourceClear(virStorageSourcePtr def) virSecurityDeviceLabelDefFree(def->seclabels[i]); VIR_FREE(def->seclabels); } - if (def->perms) { - VIR_FREE(def->perms->label); - VIR_FREE(def->perms); - } + virStoragePermsFree(def->perms); VIR_FREE(def->timestamps);
virStorageNetHostDefFree(def->nhosts, def->hosts);
On 06/19/14 20:57, John Ferlan wrote:
On 06/19/2014 09:46 AM, Peter Krempa wrote:
It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Why is it that patches 1-3 do not update libvirt_private.syms to add new src/util/vir* APIs, but some of the future patches do update, such as 4,
The functions added in 1-3 are used only in the utils module, especially in the storage source deep copy function and are not linked to from other modules and therefore the symbol export isn't really necessary. I might add them if you insist, but I don't expect the functions to be linked to other modules any soon.
5, 7?
John
Peter
On 06/20/2014 02:53 AM, Peter Krempa wrote:
On 06/19/14 20:57, John Ferlan wrote:
On 06/19/2014 09:46 AM, Peter Krempa wrote:
It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Why is it that patches 1-3 do not update libvirt_private.syms to add new src/util/vir* APIs, but some of the future patches do update, such as 4,
The functions added in 1-3 are used only in the utils module, especially in the storage source deep copy function and are not linked to from other modules and therefore the symbol export isn't really necessary.
I might add them if you insist, but I don't expect the functions to be linked to other modules any soon.
No insistence from me - just curious. It would only save you a step later on I suppose. Moot point now since they're pushed. John
On 06/20/14 12:09, John Ferlan wrote:
On 06/20/2014 02:53 AM, Peter Krempa wrote:
On 06/19/14 20:57, John Ferlan wrote:
On 06/19/2014 09:46 AM, Peter Krempa wrote:
It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Why is it that patches 1-3 do not update libvirt_private.syms to add new src/util/vir* APIs, but some of the future patches do update, such as 4,
The functions added in 1-3 are used only in the utils module, especially in the storage source deep copy function and are not linked to from other modules and therefore the symbol export isn't really necessary.
I might add them if you insist, but I don't expect the functions to be linked to other modules any soon.
No insistence from me - just curious. It would only save you a step later on I suppose. Moot point now since they're pushed.
I don't really plan on using those directly from other modules so it shouldn't be needed Peter
They will be reused to transfer disk labels from snapshotted disks to the new disk definitions. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 43 ++++++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 +++ 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 46c0f02..acb2ea3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1898,6 +1898,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; virStorageSourceClearBackingStore; +virStorageSourceCopySeclabels; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceNewFromBacking; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a23ac6a..d835917 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1496,6 +1496,29 @@ virStorageNetHostDefCopy(size_t nhosts, } +int +virStorageSourceCopySeclabels(virStorageSourcePtr to, + const virStorageSource *from) +{ + size_t i; + + if (VIR_ALLOC_N(to->seclabels, from->nseclabels) < 0) + return -1; + to->nseclabels = from->nseclabels; + + for (i = 0; i < to->nseclabels; i++) { + if (!(to->seclabels[i] = virSecurityDeviceLabelDefCopy(from->seclabels[i]))) + goto error; + } + + return 0; + + error: + virStorageSourceClearSeclabels(to); + return -1; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { @@ -1555,10 +1578,21 @@ virStorageSourceClearBackingStore(virStorageSourcePtr def) void -virStorageSourceClear(virStorageSourcePtr def) +virStorageSourceClearSeclabels(virStorageSourcePtr def) { size_t i; + if (def->seclabels) { + for (i = 0; i < def->nseclabels; i++) + virSecurityDeviceLabelDefFree(def->seclabels[i]); + VIR_FREE(def->seclabels); + } +} + + +void +virStorageSourceClear(virStorageSourcePtr def) +{ if (!def) return; @@ -1569,12 +1603,7 @@ virStorageSourceClear(virStorageSourcePtr def) virBitmapFree(def->features); VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); - - if (def->seclabels) { - for (i = 0; i < def->nseclabels; i++) - virSecurityDeviceLabelDefFree(def->seclabels[i]); - VIR_FREE(def->seclabels); - } + virStorageSourceClearSeclabels(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 34b3625..e686cef 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -317,6 +317,9 @@ void virStorageNetHostDefFree(size_t nhosts, virStorageNetHostDefPtr hosts); virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts); +void virStorageSourceClearSeclabels(virStorageSourcePtr def); +int virStorageSourceCopySeclabels(virStorageSourcePtr to, + const virStorageSource *from); void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
They will be reused to transfer disk labels from snapshotted disks to the new disk definitions. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 43 ++++++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 +++ 3 files changed, 40 insertions(+), 7 deletions(-)
+++ b/src/util/virstoragefile.c @@ -1496,6 +1496,29 @@ virStorageNetHostDefCopy(size_t nhosts, }
+int +virStorageSourceCopySeclabels(virStorageSourcePtr to, + const virStorageSource *from) +{ + size_t i; + + if (VIR_ALLOC_N(to->seclabels, from->nseclabels) < 0)
I'd feel safer if you start this function with either: virStorageSourceClearSeclabels(to); or: if (to->seclabels) return -1; to make sure that we are only ever copying labels onto a clean slate.
void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { @@ -1555,10 +1578,21 @@ virStorageSourceClearBackingStore(virStorageSourcePtr def)
void -virStorageSourceClear(virStorageSourcePtr def) +virStorageSourceClearSeclabels(virStorageSourcePtr def)
bikeshedding - maybe name this virStorageSourceSeclabelsClear (noun-verb "we are taking the StorageSourceSeclabels and clearing it"; not object-verb-noun "we are taking the StorageSource, and clearing its seclabels") The idea is on the right track, but we're starting to get into territory where cleaning up the series for v2 will be better. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/19/2014 07:46 AM, Peter Krempa wrote:
They will be reused to transfer disk labels from snapshotted disks to the new disk definitions. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 43 ++++++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 +++ 3 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 46c0f02..acb2ea3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1898,6 +1898,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; virStorageSourceClearBackingStore; +virStorageSourceCopySeclabels;
Another naming bikeshed - might be better as virStorageSourceSeclabelsCopy -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we have pointers to store disk source information and thus can easily exchange the structs behind we need a function to copy all the data. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 + 3 files changed, 147 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index acb2ea3..1c84777 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1898,6 +1898,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; virStorageSourceClearBackingStore; +virStorageSourceCopy; virStorageSourceCopySeclabels; virStorageSourceFree; virStorageSourceGetActualType; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d835917..394c9e2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1519,6 +1519,149 @@ virStorageSourceCopySeclabels(virStorageSourcePtr to, } +static virStorageTimestampsPtr +virStorageTimestampsCopy(const virStorageTimestamps *src) +{ + virStorageTimestampsPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + memcpy(ret, src, sizeof(*src)); + + return ret; +} + + +static virStoragePermsPtr +virStoragePermsCopy(const virStoragePerms *src) +{ + virStoragePermsPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->mode = src->mode; + ret->uid = src->uid; + ret->gid = src->gid; + + if (VIR_STRDUP(ret->label, src->label)) + goto error; + + return ret; + + error: + virStoragePermsFree(ret); + return NULL; +} + + +static virStorageSourcePoolDefPtr +virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) +{ + virStorageSourcePoolDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->voltype = src->voltype; + ret->pooltype = src->pooltype; + ret->actualtype = src->actualtype; + ret->mode = src->mode; + + if (VIR_STRDUP(ret->pool, src->pool) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0) + goto error; + + return ret; + + error: + virStorageSourcePoolDefFree(ret); + return NULL; +} + + +virStorageSourcePtr +virStorageSourceCopy(const virStorageSource *src, + bool backingChain) +{ + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = src->type; + ret->protocol = src->protocol; + ret->format = src->format; + ret->allocation = src->allocation; + ret->capacity = src->capacity; + ret->backingRelative = src->backingRelative; + + /* storage driver metadata are not copied */ + ret->drv = NULL; + + if (VIR_STRDUP(ret->path, src->path) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0 || + VIR_STRDUP(ret->driverName, src->driverName) < 0 || + VIR_STRDUP(ret->relPath, src->relPath) < 0 || + VIR_STRDUP(ret->relDir, src->relDir) < 0 || + VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->auth.username, src->auth.username) < 0) + goto error; + + if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + ret->nhosts = src->nhosts; + + if (!(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) + goto error; + + if (!(ret->features = virBitmapNewCopy(src->features))) + goto error; + + if (!(ret->encryption = virStorageEncryptionCopy(src->encryption))) + goto error; + + if (!(ret->perms = virStoragePermsCopy(src->perms))) + goto error; + + if (!(ret->timestamps = virStorageTimestampsCopy(src->timestamps))) + goto error; + + ret->auth.secretType = src->auth.secretType; + switch ((virStorageSecretType) src->auth.secretType) { + case VIR_STORAGE_SECRET_TYPE_NONE: + case VIR_STORAGE_SECRET_TYPE_LAST: + break; + + case VIR_STORAGE_SECRET_TYPE_UUID: + memcpy(ret->auth.secret.uuid, src->auth.secret.uuid, VIR_UUID_BUFLEN); + break; + + case VIR_STORAGE_SECRET_TYPE_USAGE: + if (VIR_STRDUP(ret->auth.secret.usage, src->auth.secret.usage) < 0) + goto error; + break; + } + + if (backingChain && src->backingStore) { + if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + true))) + goto error; + } + + if (virStorageSourceCopySeclabels(ret, src) < 0) + goto error; + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e686cef..fec1b00 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -327,6 +327,9 @@ int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceClearBackingStore(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, + bool backingChain) + ATTRIBUTE_NONNULL(1); #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
Now that we have pointers to store disk source information and thus can easily exchange the structs behind we need a function to copy all the data. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 + 3 files changed, 147 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index acb2ea3..1c84777 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1898,6 +1898,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; virStorageSourceClearBackingStore; +virStorageSourceCopy; virStorageSourceCopySeclabels;
Ah, back to my naming choices in 4/10 - observe how we have noun-verb in virStorageSourceAuthClear (clear the StorageSourceAuth), but object-verb-noun on virStorageSourceClearBackingStore (take the Storage Source, and clear its backingStore). It looks weird to have two styles, but at this point, it's a global search-and-replace pre-req patch if we want to make those two names consistent (if you go with virStorageSourceSeclabelsCopy, then it is the backingStore variant that needs the rename).
+static virStorageTimestampsPtr +virStorageTimestampsCopy(const virStorageTimestamps *src)
And here you did noun-verb, not object-verb-noun :)
+ +static virStorageSourcePoolDefPtr +virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
And again here. Looks like we have our majority-rules style.
+virStorageSourcePtr +virStorageSourceCopy(const virStorageSource *src, + bool backingChain) +{ + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = src->type; + ret->protocol = src->protocol; + ret->format = src->format; + ret->allocation = src->allocation; + ret->capacity = src->capacity; + ret->backingRelative = src->backingRelative; + + /* storage driver metadata are not copied */ + ret->drv = NULL;
Dead assignment given the VIR_ALLOC above; maybe you could just fold that into the /* comment */
+ + if (VIR_STRDUP(ret->path, src->path) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0 || + VIR_STRDUP(ret->driverName, src->driverName) < 0 || + VIR_STRDUP(ret->relPath, src->relPath) < 0 || + VIR_STRDUP(ret->relDir, src->relDir) < 0 || + VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->auth.username, src->auth.username) < 0) + goto error;
Dan has at times persuaded me that breaking this into one 'if' per VIR_STRDUP makes it easier to tell _which_ strdup failed when stepping through gdb execution; on the other hand, OOM is so unlikely to be the cause of a debug session, and you aren't mixing in any complex actions alongside the VIR_STRDUP, so I can live with this compressed form. I guess it's a judgment call of how complex each individual step is on whether it makes sense to make it easier to put breakpoints on a step and check intermediate state.
+ if (virStorageSourceCopySeclabels(ret, src) < 0) + goto error;
Huh, I wonder if we should make the seclabels contents a standalone struct, where we can manipulate it in isolation instead of always having to be part of a virStorageSourcePtr. What you have works; you may want to fold in my nits above, or may have to rebase this on top of changes in 4/10, but for now I'm okay with: ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add functions that will allow to set all the required cgroup stuff on individual images taking a virStorageSourcePtr. Also convert functions designed to setup whole backing chain to take advantage of the chagne. --- src/qemu/qemu_cgroup.c | 92 +++++++++++++++++++++++++++++++------------------- src/qemu/qemu_cgroup.h | 5 +++ 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index a31558f..df46d61 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -49,27 +49,37 @@ static const char *const defaultDeviceACL[] = { #define DEVICE_PTY_MAJOR 136 #define DEVICE_SND_MAJOR 116 -static int -qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) +int +qemuSetupImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool readonly) { - virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; int ret; - VIR_DEBUG("Process path %s for disk", path); - ret = virCgroupAllowDevicePath(priv->cgroup, path, - (disk->readonly ? VIR_CGROUP_DEVICE_READ + if (!virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + if (!src->path || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + VIR_DEBUG("Process path %s for disk", src->path); + + ret = virCgroupAllowDevicePath(priv->cgroup, src->path, + (readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW)); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - disk->readonly ? "r" : "rw", ret == 0); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", src->path, + readonly ? "r" : "rw", ret == 0); /* Get this for root squash NFS */ if (ret < 0 && virLastErrorIsSystemErrno(EACCES)) { - VIR_DEBUG("Ignoring EACCES for %s", path); + VIR_DEBUG("Ignoring EACCES for %s", src->path); virResetLastError(); ret = 0; } @@ -81,38 +91,51 @@ int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk) { - qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr next; - if (!virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; + for (next = disk->src; next; next = next->backingStore) { + if (qemuSetupImageCgroup(vm, next, disk->readonly) < 0) + return -1; - return virDomainDiskDefForeachPath(disk, true, qemuSetupDiskPathAllow, vm); + } + + return 0; } -static int -qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) +int +qemuTeardownImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src) { - virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; int ret; - VIR_DEBUG("Process path %s for disk", path); - ret = virCgroupDenyDevicePath(priv->cgroup, path, + if (!virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + if (!src->path || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + VIR_DEBUG("Process path %s for disk", src->path); + + ret = virCgroupDenyDevicePath(priv->cgroup, src->path, VIR_CGROUP_DEVICE_RWM); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", ret == 0); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, + "rwm", ret == 0); /* Get this for root squash NFS */ if (ret < 0 && virLastErrorIsSystemErrno(EACCES)) { - VIR_DEBUG("Ignoring EACCES for %s", path); + VIR_DEBUG("Ignoring EACCES for %s", src->path); virResetLastError(); ret = 0; } + return ret; } @@ -121,18 +144,17 @@ int qemuTeardownDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk) { - qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr next; - if (!virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; + for (next = disk->src; next; next = next->backingStore) { + if (qemuTeardownImageCgroup(vm, next) < 0) + return -1; + } - return virDomainDiskDefForeachPath(disk, - true, - qemuTeardownDiskPathDeny, - vm); + return 0; } + static int qemuSetupChrSourceCgroup(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainChrSourceDefPtr dev, diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 14404d1..3ee081e 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -29,8 +29,13 @@ # include "domain_conf.h" # include "qemu_conf.h" +int qemuSetupImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool readonly); int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk); +int qemuTeardownImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src); int qemuTeardownDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuSetupHostdevCGroup(virDomainObjPtr vm, -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
Add functions that will allow to set all the required cgroup stuff on
s/to set/setting/
individual images taking a virStorageSourcePtr. Also convert functions
s/taking/by taking/
designed to setup whole backing chain to take advantage of the chagne.
s/chagne/change/
--- src/qemu/qemu_cgroup.c | 92 +++++++++++++++++++++++++++++++------------------- src/qemu/qemu_cgroup.h | 5 +++ 2 files changed, 62 insertions(+), 35 deletions(-)
+int +qemuSetupImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool readonly) {
+ VIR_DEBUG("Process path %s for disk", src->path);
Might be worth mentioning the value of readonly in this debug statement (imagine reading a log to see which images got 'rw' vs. 'ro' permissions).
@@ -81,38 +91,51 @@ int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk) { - qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr next;
- if (!virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; + for (next = disk->src; next; next = next->backingStore) { + if (qemuSetupImageCgroup(vm, next, disk->readonly) < 0) + return -1;
- return virDomainDiskDefForeachPath(disk, true, qemuSetupDiskPathAllow, vm);
Not quite right (although not necessarily your fault - the existing code has the same bug, because it ignores the depth argument of qemuSetupDiskPathAllow). We want the active image to be 'rw' or 'ro' based on disk->readonly; but all of its backing files should be 'ro' without any decision needed. That is, permissions on the active layer should not determine what we do with lower layers, because lower layers should never change (well, there is a temporary exception during block-commit operations, but that's why we are writing this series - to make it easier to describe those temporary changes in the block-commit code). So the loop should be more like: for (next = disk->src; next; next = next->backingStore) { bool ro = next == disk->src ? disk->readonly : true; if (qemuSetupImageCgroup(vm, next, ro) < 0) return -1; } or: bool readonly = disk->readonly; for (next = disk->src; next; next = next->backingStore) { if (qemuSetupImageCgroup(vm, next, readonly) < 0) return -1; readonly = true; }
+int +qemuTeardownImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src) {
Not your fault that the existing code has two very similar call paths (one for grant 'rw'/'r', one for deny 'rwm'; and we never grant 'm') - but I think it might be a little cleaner if we have just ONE call path with a tri-state setting (change to one of read/write (grant 'rw' deny 'm'), readonly (grant 'r' deny 'wm'), or unaccessible (deny 'rwm'). That way, the intro code for returning early when cgroups is not mounted or when dealing with a network disk is not copied between two paths, and the code is a bit closer to our intended usage (particularly during block-commit - we are temporarily changing a backing file from readonly to read/write for the duration of the commit, then back to readonly at the end, which is different than making it unaccessible). I'd have to experiment with cgroup ACLs to see if granting a disk 'r' permission wipes out an earlier grant of 'rw', or if going from 'rw' to 'r' instead requires a deny of 'w'. I don't know if that makes any difference to the code (it may mean that we have to track the current mode, in order to make an efficient decision of how to move to readonly mode, and especially without going through a temporarily inaccessible state if the guest is live). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The image labels are stored in the virStorageSource struct. Convert the virDomainDiskDefGetSecurityLabelDef helper not to use the full disk def and move it appropriately. --- src/conf/domain_conf.c | 14 -------------- src/conf/domain_conf.h | 3 --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 +- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- src/util/virstoragefile.c | 15 +++++++++++++++ src/util/virstoragefile.h | 4 ++++ 8 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4114289..02c394f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19489,20 +19489,6 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) return seclabel; } -virSecurityDeviceLabelDefPtr -virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model) -{ - size_t i; - - if (def == NULL) - return NULL; - - for (i = 0; i < def->src->nseclabels; i++) { - if (STREQ_NULLABLE(def->src->seclabels[i]->model, model)) - return def->src->seclabels[i]; - } - return NULL; -} virSecurityDeviceLabelDefPtr virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a6ac95a..6779a41 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2519,9 +2519,6 @@ virSecurityLabelDefPtr virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model); virSecurityDeviceLabelDefPtr -virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model); - -virSecurityDeviceLabelDefPtr virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); typedef const char* (*virEventActionToStringFunc)(int type); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1c84777..4f075e5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -210,7 +210,6 @@ virDomainDiskCopyOnReadTypeToString; virDomainDiskDefAssignAddress; virDomainDiskDefForeachPath; virDomainDiskDefFree; -virDomainDiskDefGetSecurityLabelDef; virDomainDiskDefNew; virDomainDiskDeviceTypeToString; virDomainDiskDiscardTypeToString; @@ -1902,6 +1901,7 @@ virStorageSourceCopy; virStorageSourceCopySeclabels; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceGetSecurityLabelDef; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e40c5ec..7c4fc67 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2413,7 +2413,7 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, vmlabel->label) virParseOwnershipIds(vmlabel->label, uid, gid); - if ((disklabel = virDomainDiskDefGetSecurityLabelDef(disk, "dac")) && + if ((disklabel = virStorageSourceGetSecurityLabelDef(disk->src, "dac")) && disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9d5c25b..28f033d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -302,7 +302,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, uid_t user; gid_t group; - disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_DAC_NAME); if (disk_seclabel && disk_seclabel->norelabel) @@ -369,7 +369,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (secdef && secdef->norelabel) return 0; - disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_DAC_NAME); if (disk_seclabel && disk_seclabel->norelabel) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 228e5cb..0c34af8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1134,7 +1134,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (seclabel == NULL) return 0; - disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_SELINUX_NAME); if (seclabel->norelabel || (disk_seclabel && disk_seclabel->norelabel)) return 0; @@ -1202,7 +1202,7 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, virSecurityLabelDefPtr secdef = cbdata->secdef; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(cbdata->manager); - disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_SELINUX_NAME); if (disk_seclabel && disk_seclabel->norelabel) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 394c9e2..433ddc1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1519,6 +1519,21 @@ virStorageSourceCopySeclabels(virStorageSourcePtr to, } +virSecurityDeviceLabelDefPtr +virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, + const char *model) +{ + size_t i; + + for (i = 0; i < src->nseclabels; i++) { + if (STREQ_NULLABLE(src->seclabels[i]->model, model)) + return src->seclabels[i]; + } + + return NULL; +} + + static virStorageTimestampsPtr virStorageTimestampsCopy(const virStorageTimestamps *src) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index fec1b00..ccacdb2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -312,6 +312,10 @@ int virStorageFileGetLVMKey(const char *path, int virStorageFileGetSCSIKey(const char *path, char **key); +virSecurityDeviceLabelDefPtr +virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, + const char *model); + void virStorageNetHostDefClear(virStorageNetHostDefPtr def); void virStorageNetHostDefFree(size_t nhosts, virStorageNetHostDefPtr hosts); virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
The image labels are stored in the virStorageSource struct. Convert the virDomainDiskDefGetSecurityLabelDef helper not to use the full disk def and move it appropriately. --- src/conf/domain_conf.c | 14 -------------- src/conf/domain_conf.h | 3 --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 +- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- src/util/virstoragefile.c | 15 +++++++++++++++ src/util/virstoragefile.h | 4 ++++ 8 files changed, 25 insertions(+), 23 deletions(-)
+++ b/src/qemu/qemu_domain.c @@ -2413,7 +2413,7 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, vmlabel->label) virParseOwnershipIds(vmlabel->label, uid, gid);
- if ((disklabel = virDomainDiskDefGetSecurityLabelDef(disk, "dac")) && + if ((disklabel = virStorageSourceGetSecurityLabelDef(disk->src, "dac")) &&
Unrelated to your patch, but why is this an open-coded string...
disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9d5c25b..28f033d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -302,7 +302,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, uid_t user; gid_t group;
- disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_DAC_NAME);
...while this is SECURITY_DAC_NAME? Might be worth an independent cleanup. ACK. This one can be reshuffled to go in earlier, if desired. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/19/14 22:01, Eric Blake wrote:
On 06/19/2014 07:46 AM, Peter Krempa wrote:
The image labels are stored in the virStorageSource struct. Convert the virDomainDiskDefGetSecurityLabelDef helper not to use the full disk def and move it appropriately. --- src/conf/domain_conf.c | 14 -------------- src/conf/domain_conf.h | 3 --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 +- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- src/util/virstoragefile.c | 15 +++++++++++++++ src/util/virstoragefile.h | 4 ++++ 8 files changed, 25 insertions(+), 23 deletions(-)
+++ b/src/qemu/qemu_domain.c @@ -2413,7 +2413,7 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, vmlabel->label) virParseOwnershipIds(vmlabel->label, uid, gid);
- if ((disklabel = virDomainDiskDefGetSecurityLabelDef(disk, "dac")) && + if ((disklabel = virStorageSourceGetSecurityLabelDef(disk->src, "dac")) &&
Unrelated to your patch, but why is this an open-coded string...
disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9d5c25b..28f033d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -302,7 +302,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, uid_t user; gid_t group;
- disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_DAC_NAME);
...while this is SECURITY_DAC_NAME? Might be worth an independent cleanup.
SECURITY_DAC_NAME is defined in src/security/security_dac.c and thus not available to use in the qemu_domain code. Should we export it in the header of the security driver? Peter
On 06/20/2014 01:52 AM, Peter Krempa wrote:
On 06/19/14 22:01, Eric Blake wrote:
On 06/19/2014 07:46 AM, Peter Krempa wrote:
The image labels are stored in the virStorageSource struct. Convert the virDomainDiskDefGetSecurityLabelDef helper not to use the full disk def and move it appropriately. ---
+++ b/src/qemu/qemu_domain.c @@ -2413,7 +2413,7 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, vmlabel->label) virParseOwnershipIds(vmlabel->label, uid, gid);
- if ((disklabel = virDomainDiskDefGetSecurityLabelDef(disk, "dac")) && + if ((disklabel = virStorageSourceGetSecurityLabelDef(disk->src, "dac")) &&
Unrelated to your patch, but why is this an open-coded string...
disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9d5c25b..28f033d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -302,7 +302,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, uid_t user; gid_t group;
- disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, SECURITY_DAC_NAME);
...while this is SECURITY_DAC_NAME? Might be worth an independent cleanup.
SECURITY_DAC_NAME is defined in src/security/security_dac.c and thus not available to use in the qemu_domain code.
Should we export it in the header of the security driver?
Yeah, if other code is going to do actions based on driver names, then it's probably worth putting the macros for the driver names in a common header rather than buried in a .c file. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Also remove one spurious ATTRIBUTE_UNUSED guarding the @migrated argument. --- src/qemu/qemu_process.c | 2 +- src/security/security_apparmor.c | 2 +- src/security/security_dac.c | 6 +++--- src/security/security_driver.h | 2 +- src/security/security_manager.c | 2 +- src/security/security_manager.h | 2 +- src/security/security_nop.c | 2 +- src/security/security_selinux.c | 6 +++--- src/security/security_stack.c | 2 +- 9 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f1c0041..6af77c9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4419,7 +4419,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, - flags & VIR_QEMU_PROCESS_STOP_MIGRATED); + !!(flags & VIR_QEMU_PROCESS_STOP_MIGRATED)); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); for (i = 0; i < vm->def->ndisks; i++) { diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index ec8c101..ed9d192 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -550,7 +550,7 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int AppArmorRestoreSecurityAllLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - int migrated ATTRIBUTE_UNUSED) + bool migrated ATTRIBUTE_UNUSED) { int rc = 0; virSecurityLabelDefPtr secdef = diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 28f033d..6e5ccfa 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -351,7 +351,7 @@ static int virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk, - int migrated) + bool migrated) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; @@ -414,7 +414,7 @@ virSecurityDACRestoreSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk, 0); + return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk, false); } @@ -877,7 +877,7 @@ virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - int migrated) + bool migrated) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index ced1b92..879f63c 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -89,7 +89,7 @@ typedef int (*virSecurityDomainSetAllLabel) (virSecurityManagerPtr mgr, const char *stdin_path); typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, - int migrated); + bool migrated); typedef int (*virSecurityDomainGetProcessLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, pid_t pid, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 79edb07..715159c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -593,7 +593,7 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - int migrated) + bool migrated) { if (mgr->drv->domainRestoreSecurityAllLabel) { int ret; diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 81d3160..3cddcd2 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -99,7 +99,7 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, const char *stdin_path); int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - int migrated); + bool migrated); int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, pid_t pid, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 73e1ac1..a096ce2 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -139,7 +139,7 @@ static int virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_U static int virSecurityDomainRestoreAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED, - int migrated ATTRIBUTE_UNUSED) + bool migrated ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0c34af8..f5d67a9 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1124,7 +1124,7 @@ static int virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk, - int migrated) + bool migrated) { virSecurityLabelDefPtr seclabel; virSecurityDeviceLabelDefPtr disk_seclabel; @@ -1186,7 +1186,7 @@ virSecuritySELinuxRestoreSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - return virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, disk, 0); + return virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, disk, false); } @@ -1837,7 +1837,7 @@ virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int virtType) static int virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - int migrated ATTRIBUTE_UNUSED) + bool migrated) { virSecurityLabelDefPtr secdef; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e4b2db6..355c978 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -323,7 +323,7 @@ virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, static int virSecurityStackRestoreSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - int migrated) + bool migrated) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
Also remove one spurious ATTRIBUTE_UNUSED guarding the @migrated argument. --- src/qemu/qemu_process.c | 2 +- src/security/security_apparmor.c | 2 +- src/security/security_dac.c | 6 +++--- src/security/security_driver.h | 2 +- src/security/security_manager.c | 2 +- src/security/security_manager.h | 2 +- src/security/security_nop.c | 2 +- src/security/security_selinux.c | 6 +++--- src/security/security_stack.c | 2 +- 9 files changed, 13 insertions(+), 13 deletions(-)
ACK. This one can be reshuffled if you want to push it early. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
I'm going to add functions that will deal with individual image files rather than whole disks. Rename the security function to make room for the new one. --- src/libvirt_private.syms | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_hotplug.c | 16 ++++++++-------- src/security/security_apparmor.c | 6 +++--- src/security/security_dac.c | 14 +++++++------- src/security/security_driver.h | 8 ++++---- src/security/security_manager.c | 10 +++++----- src/security/security_manager.h | 6 +++--- src/security/security_nop.c | 8 ++++---- src/security/security_selinux.c | 10 +++++----- src/security/security_stack.c | 10 +++++----- 13 files changed, 52 insertions(+), 52 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4f075e5..c31b5bf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -911,10 +911,10 @@ virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; virSecurityManagerSetDaemonSocketLabel; +virSecurityManagerSetDiskLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetHugepages; virSecurityManagerSetImageFDLabel; -virSecurityManagerSetImageLabel; virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index fe2a5dc..38acdff 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1727,7 +1727,7 @@ static int virLXCControllerSetupDisk(virLXCControllerPtr ctrl, /* Labelling normally operates on src, but we need * to actually label the dst here, so hack the config */ def->src->path = dst; - if (virSecurityManagerSetImageLabel(securityDriver, ctrl->def, def) < 0) + if (virSecurityManagerSetDiskLabel(securityDriver, ctrl->def, def) < 0) goto cleanup; ret = 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 9380e8d..06f3e18 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3899,8 +3899,8 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, virDomainDiskDefPtr def = data->def->data.disk; char *tmpsrc = def->src->path; def->src->path = data->file; - if (virSecurityManagerSetImageLabel(data->driver->securityManager, - data->vm->def, def) < 0) { + if (virSecurityManagerSetDiskLabel(data->driver->securityManager, + data->vm->def, def) < 0) { def->src->path = tmpsrc; goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a733a0..22a8ca5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12098,8 +12098,8 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, } else if (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || - virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, disk) < 0) { + virSecurityManagerSetDiskLabel(driver->securityManager, + vm->def, disk) < 0) { goto cleanup; } @@ -14952,8 +14952,8 @@ qemuDomainBlockPivot(virConnectPtr conn, (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || - virSecurityManagerSetImageLabel(driver->securityManager, vm->def, - disk) < 0)) { + virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, + disk) < 0)) { disk->src->path = oldsrc; disk->src->format = oldformat; disk->src->backingStore = oldchain; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7289055..4590409 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -91,8 +91,8 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, vm, disk) < 0) goto cleanup; - if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, disk) < 0) { + if (virSecurityManagerSetDiskLabel(driver->securityManager, + vm->def, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", virDomainDiskGetSource(disk)); @@ -270,8 +270,8 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, vm, disk) < 0) goto cleanup; - if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, disk) < 0) { + if (virSecurityManagerSetDiskLabel(driver->securityManager, + vm->def, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", src); goto cleanup; @@ -509,8 +509,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, vm, disk) < 0) goto cleanup; - if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, disk) < 0) { + if (virSecurityManagerSetDiskLabel(driver->securityManager, + vm->def, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", src); goto cleanup; @@ -634,8 +634,8 @@ qemuDomainAttachUSBMassstorageDevice(virConnectPtr conn, vm, disk) < 0) goto cleanup; - if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, disk) < 0) { + if (virSecurityManagerSetDiskLabel(driver->securityManager, + vm->def, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", src); goto cleanup; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index ed9d192..c27ab47 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -696,8 +696,8 @@ AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr, /* Called when hotplugging */ static int -AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, virDomainDiskDefPtr disk) +AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, virDomainDiskDefPtr disk) { int rc = -1; char *profile_name = NULL; @@ -972,7 +972,7 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSecurityVerify = AppArmorSecurityVerify, - .domainSetSecurityImageLabel = AppArmorSetSecurityImageLabel, + .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel, .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = AppArmorSetSecurityDaemonSocketLabel, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 6e5ccfa..9760e6f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -321,9 +321,9 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, static int -virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) +virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); @@ -967,9 +967,9 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, /* XXX fixme - we need to recursively label the entire tree :-( */ if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR) continue; - if (virSecurityDACSetSecurityImageLabel(mgr, - def, - def->disks[i]) < 0) + if (virSecurityDACSetSecurityDiskLabel(mgr, + def, + def->disks[i]) < 0) return -1; } for (i = 0; i < def->nhostdevs; i++) { @@ -1273,7 +1273,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainSecurityVerify = virSecurityDACVerify, - .domainSetSecurityImageLabel = virSecurityDACSetSecurityImageLabel, + .domainSetSecurityDiskLabel = virSecurityDACSetSecurityDiskLabel, .domainRestoreSecurityImageLabel = virSecurityDACRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 879f63c..6a17a8e 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -60,9 +60,9 @@ typedef int (*virSecurityDomainSetSocketLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def); typedef int (*virSecurityDomainClearSocketLabel)(virSecurityManagerPtr mgr, virDomainDefPtr def); -typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); +typedef int (*virSecurityDomainSetDiskLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk); typedef int (*virSecurityDomainRestoreHostdevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, @@ -127,7 +127,7 @@ struct _virSecurityDriver { virSecurityDomainSecurityVerify domainSecurityVerify; - virSecurityDomainSetImageLabel domainSetSecurityImageLabel; + virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainSetDaemonSocketLabel domainSetSecurityDaemonSocketLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 715159c..f0e3ee1 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -367,14 +367,14 @@ int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) +int virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainDiskDefPtr disk) { - if (mgr->drv->domainSetSecurityImageLabel) { + if (mgr->drv->domainSetSecurityDiskLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, disk); + ret = mgr->drv->domainSetSecurityDiskLabel(mgr, vm, disk); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 3cddcd2..f083b3a 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -70,9 +70,9 @@ int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr def); int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr def); -int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); +int virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk); int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index a096ce2..7feeda6 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -75,9 +75,9 @@ static int virSecurityDomainClearSocketLabelNop(virSecurityManagerPtr mgr ATTRIB return 0; } -static int virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) +static int virSecurityDomainSetDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) { return 0; } @@ -206,7 +206,7 @@ virSecurityDriver virSecurityDriverNop = { .domainSecurityVerify = virSecurityDomainVerifyNop, - .domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop, + .domainSetSecurityDiskLabel = virSecurityDomainSetDiskLabelNop, .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, .domainSetSecurityDaemonSocketLabel = virSecurityDomainSetDaemonSocketLabelNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f5d67a9..a4c13a1 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1243,9 +1243,9 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, } static int -virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) +virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) { virSecuritySELinuxCallbackData cbdata; @@ -2240,7 +2240,7 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, def->disks[i]->dst); continue; } - if (virSecuritySELinuxSetSecurityImageLabel(mgr, + if (virSecuritySELinuxSetSecurityDiskLabel(mgr, def, def->disks[i]) < 0) return -1; } @@ -2426,7 +2426,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSecurityVerify = virSecuritySELinuxSecurityVerify, - .domainSetSecurityImageLabel = virSecuritySELinuxSetSecurityImageLabel, + .domainSetSecurityDiskLabel = virSecuritySELinuxSetSecurityDiskLabel, .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecuritySELinuxSetSecurityDaemonSocketLabel, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 355c978..63b2720 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -222,16 +222,16 @@ virSecurityStackReserveLabel(virSecurityManagerPtr mgr, static int -virSecurityStackSetSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) +virSecurityStackSetSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainDiskDefPtr disk) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerSetImageLabel(item->securityManager, vm, disk) < 0) + if (virSecurityManagerSetDiskLabel(item->securityManager, vm, disk) < 0) rc = -1; } @@ -578,7 +578,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSecurityVerify = virSecurityStackVerify, - .domainSetSecurityImageLabel = virSecurityStackSetSecurityImageLabel, + .domainSetSecurityDiskLabel = virSecurityStackSetSecurityDiskLabel, .domainRestoreSecurityImageLabel = virSecurityStackRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecurityStackSetDaemonSocketLabel, -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
I'm going to add functions that will deal with individual image files rather than whole disks. Rename the security function to make room for the new one. --- src/libvirt_private.syms | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_hotplug.c | 16 ++++++++-------- src/security/security_apparmor.c | 6 +++--- src/security/security_dac.c | 14 +++++++------- src/security/security_driver.h | 8 ++++---- src/security/security_manager.c | 10 +++++----- src/security/security_manager.h | 6 +++--- src/security/security_nop.c | 8 ++++---- src/security/security_selinux.c | 10 +++++----- src/security/security_stack.c | 10 +++++----- 13 files changed, 52 insertions(+), 52 deletions(-)
Mechanical; ACK. Can be applied now. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Until now we were changing information about the disk source via multiple steps of copying data. Now that we changed to a pointer to store the disk source we might use it to change the approach to track the data. Additionally this will allow proper tracking of the backing chain. --- Now the security data is copied, but I'm still not sure that's everything we need. src/qemu/qemu_driver.c | 113 ++++++++++++------------------------------------- 1 file changed, 27 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 22a8ca5..dd5de1a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12842,14 +12842,11 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, enum qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr newDiskSrc = NULL; + virStorageSourcePtr persistDiskSrc = NULL; char *device = NULL; char *source = NULL; - char *newsource = NULL; - virStorageNetHostDefPtr newhosts = NULL; - virStorageNetHostDefPtr persistHosts = NULL; - int format = snap->src->format; const char *formatStr = NULL; - char *persistSource = NULL; int ret = -1; int fd = -1; bool need_unlink = false; @@ -12863,24 +12860,22 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup; - /* XXX Here, we know we are about to alter disk->src->backingStore if - * successful, so we nuke the existing chain so that future commands will - * recompute it. Better would be storing the chain ourselves rather than - * reprobing, but this requires modifying domain_conf and our XML to fully - * track the chain across libvirtd restarts. */ - virStorageSourceClearBackingStore(disk->src); - if (virStorageFileInit(snap->src) < 0) goto cleanup; if (qemuGetDriveSourceString(snap->src, NULL, &source) < 0) goto cleanup; - if (VIR_STRDUP(newsource, snap->src->path) < 0) + if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) + goto cleanup; + + /* transfer seclabels from the old disk */ + if (!newDiskSrc->seclabels && + virStorageSourceCopySeclabels(newDiskSrc, disk->src) < 0) goto cleanup; if (persistDisk && - VIR_STRDUP(persistSource, snap->src->path) < 0) + !(persistDiskSrc = virStorageSourceCopy(newDiskSrc, false))) goto cleanup; switch ((virStorageType)snap->src->type) { @@ -12908,15 +12903,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, case VIR_STORAGE_TYPE_NETWORK: switch (snap->src->protocol) { case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - if (!(newhosts = virStorageNetHostDefCopy(snap->src->nhosts, - snap->src->hosts))) - goto cleanup; - - if (persistDisk && - !(persistHosts = virStorageNetHostDefCopy(snap->src->nhosts, - snap->src->hosts))) - goto cleanup; - break; default: @@ -12967,45 +12953,24 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ need_unlink = false; - VIR_FREE(disk->src->path); - virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); - - disk->src->path = newsource; - disk->src->format = format; - disk->src->type = snap->src->type; - disk->src->protocol = snap->src->protocol; - disk->src->nhosts = snap->src->nhosts; - disk->src->hosts = newhosts; - - newsource = NULL; - newhosts = NULL; + newDiskSrc->backingStore = disk->src; + disk->src = newDiskSrc; + newDiskSrc = NULL; if (persistDisk) { - VIR_FREE(persistDisk->src->path); - virStorageNetHostDefFree(persistDisk->src->nhosts, - persistDisk->src->hosts); - - persistDisk->src->path = persistSource; - persistDisk->src->format = format; - persistDisk->src->type = snap->src->type; - persistDisk->src->protocol = snap->src->protocol; - persistDisk->src->nhosts = snap->src->nhosts; - persistDisk->src->hosts = persistHosts; - - persistSource = NULL; - persistHosts = NULL; + persistDiskSrc->backingStore = persistDisk->src; + persistDisk->src = persistDiskSrc; + persistDiskSrc = NULL; } cleanup: if (need_unlink && virStorageFileUnlink(snap->src)) VIR_WARN("unable to unlink just-created %s", source); virStorageFileDeinit(snap->src); + virStorageSourceFree(newDiskSrc); + virStorageSourceFree(persistDiskSrc); VIR_FREE(device); VIR_FREE(source); - VIR_FREE(newsource); - VIR_FREE(persistSource); - virStorageNetHostDefFree(snap->src->nhosts, newhosts); - virStorageNetHostDefFree(snap->src->nhosts, persistHosts); return ret; } @@ -13015,21 +12980,15 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, static void qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr origdisk, virDomainDiskDefPtr disk, virDomainDiskDefPtr persistDisk, bool need_unlink) { - char *source = NULL; - char *persistSource = NULL; + virStorageSourcePtr tmp; struct stat st; ignore_value(virStorageFileInit(disk->src)); - if (VIR_STRDUP(source, origdisk->src->path) < 0 || - (persistDisk && VIR_STRDUP(persistSource, source) < 0)) - goto cleanup; - qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src, VIR_DISK_CHAIN_NO_ACCESS); if (need_unlink && @@ -13037,35 +12996,18 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virStorageFileUnlink(disk->src) < 0) VIR_WARN("Unable to remove just-created %s", disk->src->path); + virStorageFileDeinit(disk->src); + /* Update vm in place to match changes. */ - VIR_FREE(disk->src->path); - disk->src->path = source; - source = NULL; - disk->src->format = origdisk->src->format; - disk->src->type = origdisk->src->type; - disk->src->protocol = origdisk->src->protocol; - virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); - disk->src->nhosts = origdisk->src->nhosts; - disk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, - origdisk->src->hosts); + tmp = disk->src->backingStore; + virStorageSourceFree(disk->src); + disk->src = tmp; + if (persistDisk) { - VIR_FREE(persistDisk->src->path); - persistDisk->src->path = persistSource; - persistSource = NULL; - persistDisk->src->format = origdisk->src->format; - persistDisk->src->type = origdisk->src->type; - persistDisk->src->protocol = origdisk->src->protocol; - virStorageNetHostDefFree(persistDisk->src->nhosts, - persistDisk->src->hosts); - persistDisk->src->nhosts = origdisk->src->nhosts; - persistDisk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, - origdisk->src->hosts); + tmp = persistDisk->src->backingStore; + virStorageSourceFree(persistDisk->src); + persistDisk->src = tmp; } - - cleanup: - virStorageFileDeinit(disk->src); - VIR_FREE(source); - VIR_FREE(persistSource); } /* The domain is expected to be locked and active. */ @@ -13168,7 +13110,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } qemuDomainSnapshotUndoSingleDiskActive(driver, vm, - snap->def->dom->disks[i], vm->def->disks[i], persistDisk, need_unlink); -- 1.9.3
On 06/19/2014 07:46 AM, Peter Krempa wrote:
Until now we were changing information about the disk source via multiple steps of copying data. Now that we changed to a pointer to store the disk source we might use it to change the approach to track the data.
Additionally this will allow proper tracking of the backing chain. ---
Now the security data is copied, but I'm still not sure that's everything we need.
src/qemu/qemu_driver.c | 113 ++++++++++++------------------------------------- 1 file changed, 27 insertions(+), 86 deletions(-)
This patch touches just snapshot creation (good - doing it in pieces is the right way to go); but blockpull, blockcommit, and blockcopy also probably all could benefit from similar cleanups.
- if (VIR_STRDUP(newsource, snap->src->path) < 0) + if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) + goto cleanup; + + /* transfer seclabels from the old disk */ + if (!newDiskSrc->seclabels && + virStorageSourceCopySeclabels(newDiskSrc, disk->src) < 0) goto cleanup;
May need rebasing depending on if you rename things earlier in the series. But looks like the correct action for newDiskSrc.
if (persistDisk && - VIR_STRDUP(persistSource, snap->src->path) < 0) + !(persistDiskSrc = virStorageSourceCopy(newDiskSrc, false)))
Hmm, this copies the security labels into the persistDiskSrc that were already copied into newDiskSrc. But aren't security labels different between live and persistent XML? That is, when SELinux labels are generated at runtime, the live XML contains the generated label while the persistent one does not. I'm thinking that you probably need to hoise the clone into persistDiskSrc to occur before you copy seclables into newDiskSrc, and that you want then want to copy seclabels from persistDisk into persistDiskSrc rather than cloning those from newDiskSrc.
if (persistDisk) { - VIR_FREE(persistDisk->src->path); - virStorageNetHostDefFree(persistDisk->src->nhosts, - persistDisk->src->hosts); - - persistDisk->src->path = persistSource; - persistDisk->src->format = format; - persistDisk->src->type = snap->src->type; - persistDisk->src->protocol = snap->src->protocol; - persistDisk->src->nhosts = snap->src->nhosts; - persistDisk->src->hosts = persistHosts; - - persistSource = NULL; - persistHosts = NULL; + persistDiskSrc->backingStore = persistDisk->src; + persistDisk->src = persistDiskSrc; + persistDiskSrc = NULL;
I LOVE how much nicer this looks !
@@ -13015,21 +12980,15 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, static void qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr origdisk, virDomainDiskDefPtr disk, virDomainDiskDefPtr persistDisk, bool need_unlink)
Makes sense - now that we are dealing with a linked list, we are just peeling off the head of the list and don't need an origdisk for reference.
@@ -13037,35 +12996,18 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virStorageFileUnlink(disk->src) < 0) VIR_WARN("Unable to remove just-created %s", disk->src->path);
+ virStorageFileDeinit(disk->src); +
Why do you need to explicitly deinit? Can't you just let that happen automatically when freeing the virStorageSourcePtr down below?
/* Update vm in place to match changes. */ - VIR_FREE(disk->src->path); - disk->src->path = source; - source = NULL; - disk->src->format = origdisk->src->format; - disk->src->type = origdisk->src->type; - disk->src->protocol = origdisk->src->protocol; - virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); - disk->src->nhosts = origdisk->src->nhosts; - disk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, - origdisk->src->hosts); + tmp = disk->src->backingStore; + virStorageSourceFree(disk->src);
Oops. virStorageSourceFree() is recursive, which invalidates the memory pointed to by tmp. You are missing: disk->src->backingStore = NULL; in between assigning to tmp and freeing disk->src.
+ disk->src = tmp;
But this is indeed a slick reduction of the size of the backing chain :)
+ if (persistDisk) { - VIR_FREE(persistDisk->src->path); - persistDisk->src->path = persistSource; - persistSource = NULL; - persistDisk->src->format = origdisk->src->format; - persistDisk->src->type = origdisk->src->type; - persistDisk->src->protocol = origdisk->src->protocol; - virStorageNetHostDefFree(persistDisk->src->nhosts, - persistDisk->src->hosts); - persistDisk->src->nhosts = origdisk->src->nhosts; - persistDisk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, - origdisk->src->hosts); + tmp = persistDisk->src->backingStore;
Another missing assignment to NULL.
+ virStorageSourceFree(persistDisk->src); + persistDisk->src = tmp; } - - cleanup: - virStorageFileDeinit(disk->src); - VIR_FREE(source); - VIR_FREE(persistSource); }
/* The domain is expected to be locked and active. */ @@ -13168,7 +13110,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, }
qemuDomainSnapshotUndoSingleDiskActive(driver, vm, - snap->def->dom->disks[i], vm->def->disks[i], persistDisk, need_unlink);
Not quite ready for prime-time, but definitely closer this time around. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/19/14 22:47, Eric Blake wrote:
On 06/19/2014 07:46 AM, Peter Krempa wrote:
Until now we were changing information about the disk source via multiple steps of copying data. Now that we changed to a pointer to store the disk source we might use it to change the approach to track the data.
Additionally this will allow proper tracking of the backing chain. ---
Now the security data is copied, but I'm still not sure that's everything we need.
src/qemu/qemu_driver.c | 113 ++++++++++++------------------------------------- 1 file changed, 27 insertions(+), 86 deletions(-)
...
@@ -13037,35 +12996,18 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virStorageFileUnlink(disk->src) < 0) VIR_WARN("Unable to remove just-created %s", disk->src->path);
+ virStorageFileDeinit(disk->src); +
Why do you need to explicitly deinit? Can't you just let that happen automatically when freeing the virStorageSourcePtr down below?
The deinit calls into the storage driver and thus can't be integrated into the util code freeing the virStorageSource.
/* Update vm in place to match changes. */ - VIR_FREE(disk->src->path); - disk->src->path = source; - source = NULL; - disk->src->format = origdisk->src->format; - disk->src->type = origdisk->src->type; - disk->src->protocol = origdisk->src->protocol;
Peter
On 06/19/14 15:46, Peter Krempa wrote:
This is a work in progress snapshot of my current state to get rid of a few very ugly places where we deal with storage source information while labelling images.
The goal of this series will be to get rid of the ugly places and additionally fix problem when block-copying a networked disk which may break badly.
Peter Krempa (10): storage: encryption: Add deep copy function for storage encryption util: seclabel: Add deep copy function for device labels util: storagefile: Introduce helper to free storage source perms util: storagesource: Add helper to copy and free storage source seclabels util: storagefile: Add deep copy for struct virStorageSource qemu: cgroup: Add functions to set cgroup image stuff on individual imgs util: Don't require full disk definition when getting imagelabels security: Sanitize type of @migrated in virSecurityManagerRestoreAllLabel security: Rename virSecurityManagerSetImageLabel to *Disk* qemu: snapshot: Improve approach to deal with snapshot metadata
I've pushed 1-3 and 7-9 and will fix the rest in a next version. Peter
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa