8:46 a.m.
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
8:46 a.m.
New subject: [libvirt] [PATCH 01/10] storage: encryption: Add deep copy function for storage encryption
---
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
1:42 p.m.
New subject: [libvirt] [PATCH 01/10] storage: encryption: Add deep copy function for storage encryption
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
8:46 a.m.
New subject: [libvirt] [PATCH 02/10] util: seclabel: Add deep copy function for device labels
---
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
1:43 p.m.
New subject: [libvirt] [PATCH 02/10] util: seclabel: Add deep copy function for device labels
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
8:46 a.m.
New subject: [libvirt] [PATCH 03/10] util: storagefile: Introduce helper to free storage source perms
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
1:44 p.m.
New subject: [libvirt] [PATCH 03/10] util: storagefile: Introduce helper to free storage source perms
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
1:57 p.m.
New subject: [libvirt] [PATCH 03/10] util: storagefile: Introduce helper to free storage source perms
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);
1:53 a.m.
New subject: [libvirt] [PATCH 03/10] util: storagefile: Introduce helper to free storage source perms
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
5:09 a.m.
New subject: [libvirt] [PATCH 03/10] util: storagefile: Introduce helper to free storage source perms
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
2:33 a.m.
New subject: [libvirt] [PATCH 03/10] util: storagefile: Introduce helper to free storage source perms
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
8:46 a.m.
New subject: [libvirt] [PATCH 04/10] util: storagesource: Add helper to copy and free storage source seclabels
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
1:50 p.m.
New subject: [libvirt] [PATCH 04/10] util: storagesource: Add helper to copy and free storage source seclabels
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
1:54 p.m.
New subject: [libvirt] [PATCH 04/10] util: storagesource: Add helper to copy and free storage source seclabels
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
8:46 a.m.
New subject: [libvirt] [PATCH 05/10] util: storagefile: Add deep copy for struct virStorageSource
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
2:03 p.m.
New subject: [libvirt] [PATCH 05/10] util: storagefile: Add deep copy for struct virStorageSource
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
8:46 a.m.
New subject: [libvirt] [PATCH 06/10] qemu: cgroup: Add functions to set cgroup image stuff on individual imgs
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
2:42 p.m.
New subject: [libvirt] [PATCH 06/10] qemu: cgroup: Add functions to set cgroup image stuff on individual imgs
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
8:46 a.m.
New subject: [libvirt] [PATCH 07/10] util: Don't require full disk definition when getting imagelabels
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
3:01 p.m.
New subject: [libvirt] [PATCH 07/10] util: Don't require full disk definition when getting imagelabels
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
2:52 a.m.
New subject: [libvirt] [PATCH 07/10] util: Don't require full disk definition when getting imagelabels
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
11:49 a.m.
New subject: [libvirt] [PATCH 07/10] util: Don't require full disk definition when getting imagelabels
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
8:46 a.m.
New subject: [libvirt] [PATCH 08/10] security: Sanitize type of @migrated in virSecurityManagerRestoreAllLabel
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
3:22 p.m.
New subject: [libvirt] [PATCH 08/10] security: Sanitize type of @migrated in virSecurityManagerRestoreAllLabel
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
8:46 a.m.
New subject: [libvirt] [PATCH 09/10] security: Rename virSecurityManagerSetImageLabel to *Disk*
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
3:30 p.m.
New subject: [libvirt] [PATCH 09/10] security: Rename virSecurityManagerSetImageLabel to *Disk*
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
8:46 a.m.
New subject: [libvirt] [PATCH RFCv2 10/10] qemu: snapshot: Improve approach to deal with snapshot metadata
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
3:47 p.m.
New subject: [libvirt] [PATCH RFCv2 10/10] qemu: snapshot: Improve approach to deal with snapshot metadata
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
7:22 a.m.
New subject: [libvirt] [PATCH RFCv2 10/10] qemu: snapshot: Improve approach to deal with snapshot metadata
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
3:11 a.m.
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
3805
days inactive
3809
days old
29 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa