10:26 a.m.
On 03/12/2014 05:34 PM, Eric Blake wrote:
Code-wise, I'm looking at splitting 'struct _virDomainDiskDef' into two
parts. The outermost part is _virDomainDiskDef, which tracks anything
tied to the guest view, or to the device as a whole (<target>, <alias>,
<address>); the inner part is a new _virDomainDiskSrcDef, which tracks
anything related to a host view (node name, <source>, <driver>,
<backingStore>), and where each backingStore is also a
_virDomainDiskSrcDef, as a recursive structure - we just special case
the output so that the first _virDomainDiskSrcDef feeds the XML of
<disk> element, while all other _virDomainDiskSrcDef feed the XML of a
<backingStore>.
This won't compile, but here's the split I'm currently envisioning.
Also, Peter reminded me on IRC that it is going to be nicer if the
host-side source resource can be reusable in the src/util/virstorage
framework, which means I need to move the inner struct out of
conf/domain_conf.h (conf/ can use util/, but util/ cannot use conf/):
diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h
index 37191a8..5a3cd77 100644
--- i/src/conf/domain_conf.h
+++ w/src/conf/domain_conf.h
@@ -688,108 +688,119 @@ enum virDomainDiskSourcePoolMode {
...
};
typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
-/* Stores the virtual disk configuration */
-struct _virDomainDiskDef {
- int type;
- int device;
- int bus;
+struct _virDomainDiskSourceDef {
+ int type; /* enum virDomainDiskType */
char *src;
- char *dst;
- int tray_status;
- int removable;
- int protocol;
+ int protocol; /* enum virDomainDiskProtocol */
size_t nhosts;
virDomainDiskHostDefPtr hosts;
virDomainDiskSourcePoolDefPtr srcpool;
struct {
char *username;
int secretType; /* enum virDomainDiskSecretType */
union {
unsigned char uuid[VIR_UUID_BUFLEN];
char *usage;
} secret;
} auth;
+ virStorageEncryptionPtr encryption;
char *driverName;
int format; /* enum virStorageFileFormat */
virStorageFileMetadataPtr backingChain;
+ size_t nseclabels;
+ virSecurityDeviceLabelDefPtr *seclabels;
+
+ bool noBacking;
+ virDomainDiskSourceDefPtr backing;
+};
+typedef struct _virDomainDiskSourceDef virDomainDiskSourceDef;
+typedef virDomainDiskSourceDef *virDomainDiskSourceDefPtr;
+
+/* Stores the virtual disk configuration */
+struct _virDomainDiskDef {
+ virDomainDiskSourceDef src;
+
+ int device; /* enum virDomainDiskDevice */
+ int bus; /* enum virDomainDiskBus */
+ char *dst;
+ int tray_status;
+ int removable;
+ virStorageFileMetadataPtr backingChain;
+
char *mirror;
int mirrorFormat; /* enum virStorageFileFormat */
bool mirroring;
struct {
unsigned int cylinders;
unsigned int heads;
unsigned int sectors;
int trans;
} geometry;
struct {
unsigned int logical_block_size;
unsigned int physical_block_size;
} blockio;
virDomainBlockIoTuneInfo blkdeviotune;
char *serial;
char *wwn;
char *vendor;
char *product;
int cachemode;
int error_policy; /* enum virDomainDiskErrorPolicy */
int rerror_policy; /* enum virDomainDiskErrorPolicy */
int iomode;
int ioeventfd;
int event_idx;
int copy_on_read;
int snapshot; /* enum virDomainSnapshotLocation, snapshot_conf.h */
int startupPolicy; /* enum virDomainStartupPolicy */
bool readonly;
bool shared;
bool transient;
virDomainDeviceInfo info;
- virStorageEncryptionPtr encryption;
bool rawio_specified;
int rawio; /* no = 0, yes = 1 */
int sgio; /* enum virDomainDeviceSGIO */
int discard; /* enum virDomainDiskDiscard */
-
- size_t nseclabels;
- virSecurityDeviceLabelDefPtr *seclabels;
};
If I did the split right, then everything that is per-device remains in
the outer struct, and everything that is per-file is in the inner
struct. noBacking is required to know whether 'backing==NULL' implies
<backingStore/> as an explicit end of chain, vs. omitting the subelement
from older versions or user input that is still expecting libvirt to
populate the backing chain into the xml. Another interesting
observation - we have obviously not done much with chains of encrypted
volumes, because just a little thought makes it obvious that <auth> and
storageEncryption must be per-file attributes (it is feasible to have a
chain of two separate encrypted qcow2 images[*], where the two images
need SEPARATE passwords) while the current design of only one <auth> per
device doesn't cope. Similarly, we can finally express the fact that
the security label on backing stores is readonly while the top-most file
is read-write; as well as designate when we have changed a backing store
to read-write in order to update metadata such as during a commit
operation (there are some FIXMEs in qemu_driver about knowing when to
revert read-write privileges of backing stores if a block commit extends
over a libvirtd restart).
[*] Of course, I must give the caveat that I'd highly recommend AGAINST
using qcow2 encryption - it is known to be a lousy implementation, when
compared to LUKS.
In making the proposed split, I noticed that we've abused the <driver>
element to contain a hodgepodge of things that are per-device (for
example, cache is a per-device setting, while format is a per-file
setting), so I'm now trying to figure out how to tweak the XML to
express the difference. I may end up keeping <driver> only at the top
level, and adding a new <format> subelement inside <backingStore>, then
for back-compat reasons duplicate <driver format='...'/> and <format>
at
the top level, or teaching the disk source formatter to merely append in
a string of device-level attributes when formatting the active disk of
the chain.
Peter, how does this split coincide with what you were looking at?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org