Re: [libvirt] [PATCH 1/5] conf: Keep 'readonly' property when resetting disk source
On 04/07/2017 11:50 AM, Peter Krempa wrote:
The property is necessary also for the disk using the source (e.g.
cdrom)
which needs to be kept readonly.
---
src/conf/domain_conf.c | 3 +++
1 file changed, 3 insertions(+)
Wouldn't restoring readonly only be necessary "if (def->device ==
VIR_DOMAIN_DISK_DEVICE_CDROM)" since that's the primary reason it was
set by libvirt? Of course it's only ever set at XML parse time, so
would inserting a source path necessarily reset it for non CDROM's.
Also know it's possible to check git history and all, could the commit
message be adjusted to provide a few more shreds of information... Such
as adding in "commit id '462c4b66' was a bit too aggressive and missed
restoring the readonly flag for the storage source that is set by
ParseXML for CDROM's"
Of course this all makes me wonder if something else was too
aggressively cleared/reset, but at least this much is perfectly
understandable.
ACK for what's here w/ a bit more details for the commit message...
John
As an aside, the API could have been called DiskSetEmptySource or
DiskSetSourceEmpty... DiskEmptySource almost looks like a boolean
function. Oh well different issue ;-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 80baa090a..d660c06e0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1723,9 +1723,12 @@ void
virDomainDiskEmptySource(virDomainDiskDefPtr def)
{
virStorageSourcePtr src = def->src;
+ bool readonly = src->readonly;
virStorageSourceClear(src);
src->type = VIR_STORAGE_TYPE_FILE;
+ /* readonly property is necessary for CDROMs and thus can't be cleared */
+ src->readonly = readonly;
}