On 01/30/2013 01:11 PM, Osier Yang wrote:

It's long enough to have a independant definition. --- docs/schemas/domaincommon.rng | 93 +++++++++++++++++++++-------------------- 1 files changed, 48 insertions(+), 45 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 049f232..87653ce 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1075,51 +1075,7 @@ <interleave> <optional> <element name="source"> - <attribute name="protocol"> - <choice> - <value>nbd</value> - <value>rbd</value> - <value>sheepdog</value> - <value>gluster</value> - </choice> - </attribute> - <optional> - <attribute name="name"/> - </optional> - <zeroOrMore> - <element name="host"> - <choice> - <group> - <optional> - <attribute name="transport"> - <choice> - <value>tcp</value> - <value>rdma</value> - </choice> - </attribute> - </optional> - <attribute name="name"> - <choice> - <ref name="dnsName"/> - <ref name="ipAddr"/> - </choice> - </attribute> - <attribute name="port"> - <ref name="unsignedInt"/> - </attribute> - </group> - <group> - <attribute name="transport"> - <value>unix</value> - </attribute> - <attribute name="socket"> - <ref name="absFilePath"/> - </attribute> - </group> - </choice> - </element> - </zeroOrMore> - <empty/> + <ref name='diskSourceNetwork'/> </element> </optional> <ref name="diskspec"/> @@ -1129,6 +1085,53 @@ </choice> </element> </define> + <define name="diskSourceNetwork"> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>rbd</value> + <value>sheepdog</value> + <value>gluster</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <zeroOrMore> + <element name="host"> + <choice> + <group> + <optional> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>rdma</value> + </choice> + </attribute> + </optional> + <attribute name="name"> + <choice> + <ref name="dnsName"/> + <ref name="ipAddr"/> + </choice> + </attribute> + <attribute name="port"> + <ref name="unsignedInt"/> + </attribute> + </group> + <group> + <attribute name="transport"> + <value>unix</value> + </attribute> + <attribute name="socket"> + <ref name="absFilePath"/> + </attribute> + </group> + </choice> + </element> + </zeroOrMore> + <empty/> + </define> <define name="diskTarget"> <data type="string"> <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param>

ACK

On 01/30/2013 01:11 PM, Osier Yang wrote:

With this patch, one can specify the disk source using libvirt storage like:

<disk type='volume' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source pool='default' volume='fc18.img'/> <target dev='vdb' bus='virtio'/> </disk>

"seclables" and "startupPolicy" are not supported for this new disk type ("volume"). They will be supported in later patches.

docs/formatdomain.html.in: * Add documents for new XMLs docs/schemas/domaincommon.rng: * Add rng for new XMLs; src/conf/domain_conf.h: * New struct for 'volume' type disk source (virDomainDiskSourcePoolDef) * Add VIR_DOMAIN_DISK_TYPE_VOLUME for enum virDomainDiskType src/conf/domain_conf.c: * New helper virDomainDiskSourcePoolDefParse to parse the 'volume' type disk source. * New helper virDomainDiskSourcePoolDefFree to free the source def if 'volume' type disk. * New helper virDomainDiskSourceDefFormat to format the disk source. tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml: tests/qemuxml2xmltest.c: * New test

--- The data struct for disk source should be tidied up. It's a mess now, but it will be later patches. --- docs/formatdomain.html.in | 17 +- docs/schemas/domaincommon.rng | 18 ++ src/conf/domain_conf.c | 259 +++++++++++++------- src/conf/domain_conf.h | 9 + .../qemuxml2argv-disk-source-pool.xml | 33 +++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 247 insertions(+), 90 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7ad8aea..ac5657a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1367,6 +1367,11 @@ <blockio logical_block_size='512' physical_block_size='4096'/> <target dev='hda' bus='ide'/> </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol0'/> + <target dev='hda' bus='ide'/> + </disk> </devices> ...</pre>

@@ -1374,7 +1379,7 @@ <dt><code>disk</code></dt> <dd>The <code>disk</code> element is the main container for describing disks. The <code>type</code> attribute is either "file", - "block", "dir", or "network" + "block", "dir", "network", or "volume". and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed to the guest OS. Possible values for this attribute are @@ -1444,9 +1449,15 @@ volume/image will be used. When the disk <code>type</code> is "network", the <code>source</code> may have zero or more <code>host</code> sub-elements used to specify the hosts - to connect. + to connect. If the disk <code>type</code> is "volume", the underlying + disk source is represented by attributes <code>pool</code> and + <code>volume</code>. Attribute <code>pool</code> specifies the + name of storage pool (managed by libvirt) where the disk source resides, + and attribute <code>volume</code> specifies the name of storage volume + (managed by libvirt) used as the disk source. <span class="since">Since 0.0.3; <code>type='dir'</code> since - 0.7.5; <code>type='network'</code> since 0.8.7</span><br/> + 0.7.5; <code>type='network'</code> since 0.8.7; + <code>type='volume'</code> since 1.0.3</span><br/> For a "file" disk type which represents a cdrom or floppy (the <code>device</code> attribute), it is possible to define policy what to do with the disk if the source file is not accessible. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 87653ce..88612ae 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1081,6 +1081,24 @@ <ref name="diskspec"/> </interleave> </group> + <group> + <attribute name="type"> + <value>volume</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="pool"> + <ref name="genericName"/> + </attribute> + <attribute name="volume"> + <ref name="volName"/> + </attribute> + </element> + </optional> + <ref name="diskspec"/> + </interleave> + </group> <ref name="diskspec"/> </choice> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8dbfb96..5e65406 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -182,7 +182,8 @@ VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST, "block", "file", "dir", - "network") + "network", + "volume")

VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST, "disk", @@ -985,6 +986,18 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def) VIR_FREE(def); }

+static void +virDomainDiskSourcePoolDefFree(virDomainDiskSourcePoolDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->pool); + VIR_FREE(def->volume); + + VIR_FREE(def); +} + void virDomainDiskDefFree(virDomainDiskDefPtr def) { unsigned int i; @@ -994,6 +1007,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)

VIR_FREE(def->serial); VIR_FREE(def->src); + virDomainDiskSourcePoolDefFree(def->srcpool); VIR_FREE(def->dst); VIR_FREE(def->driverName); virStorageFileFreeMetadata(def->backingChain); @@ -3573,6 +3587,46 @@ cleanup: goto cleanup; }

+static int +virDomainDiskSourcePoolDefParse(xmlNodePtr node, + virDomainDiskDefPtr def) +{ + char *pool = NULL; + char *volume = NULL; + int ret = -1; + + pool = virXMLPropString(node, "pool"); + volume = virXMLPropString(node, "volume"); + + /* CD-ROM and Floppy allows no source */ + if (!pool && !volume) + return 0; + + if ((!pool && volume) || (pool && !volume)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'pool' and 'volume' must be specified together " + "for 'pool' type source")); + goto cleanup; + }

Couldn't this be simplified to "if (!pool || !volume) {"?

+ + if (VIR_ALLOC(def->srcpool) < 0) { + virReportOOMError(); + goto cleanup; + } + + def->srcpool->pool = pool; + pool = NULL; + def->srcpool->volume = volume; + volume = NULL; + + ret = 0; + +cleanup: + VIR_FREE(pool); + VIR_FREE(volume); + return ret; +} + #define VENDOR_LEN 8 #define PRODUCT_LEN 16

@@ -3666,7 +3720,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { - if (!source && !hosts && + if (!source && !hosts && !def->srcpool && xmlStrEqual(cur->name, BAD_CAST "source")) {

sourceNode = cur; @@ -3760,6 +3814,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, child = child->next; } break; + case VIR_DOMAIN_DISK_TYPE_VOLUME: + if (virDomainDiskSourcePoolDefParse(cur, def) < 0) + goto error; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %s"), @@ -4050,7 +4108,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,

/* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present */ - if (source == NULL && hosts == NULL && + if (!source && !hosts && !def->srcpool && def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { virReportError(VIR_ERR_NO_SOURCE, @@ -4072,8 +4130,19 @@ virDomainDiskDefParseXML(virCapsPtr caps, }

if (target == NULL) { - virReportError(VIR_ERR_NO_TARGET, - source ? "%s" : NULL, source); + if (def->srcpool) { + char *tmp; + if (virAsprintf(&tmp, "pool = '%s', volume = '%s'", + def->srcpool->pool, def->srcpool->volume) < 0) { + virReportOOMError(); + goto error; + } + + virReportError(VIR_ERR_NO_TARGET, "%s", tmp); + VIR_FREE(tmp); + } else { + virReportError(VIR_ERR_NO_TARGET, source ? "%s" : NULL, source); + } goto error; }

@@ -12168,6 +12237,102 @@ static void virDomainDiskBlockIoDefFormat(virBufferPtr buf, } }

+static int +virDomainDiskSourceDefFormat(virBufferPtr buf, + virDomainDiskDefPtr def) +{ + int n; + const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy); + + if (def->src || def->nhosts > 0 || def->srcpool || + def->startupPolicy) { + switch (def->type) { + case VIR_DOMAIN_DISK_TYPE_FILE:

What happened to virBufferAddLit() here?

+ if (def->src) + virBufferEscapeString(buf, " <source file='%s'", def->src);

If (!def->src), then startupPolicy and nseclabels won't have "<source " - whether that's realistic I'm not sure (still learning). If def->src cannot be NULL like other case labels, then no need to check. It seems from other code I read that it must be set.

+ if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); + if (def->nseclabels) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 8); + for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); + virBufferAdjustIndent(buf, -8); + virBufferAddLit(buf, " </source>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + break; + case VIR_DOMAIN_DISK_TYPE_BLOCK: + virBufferEscapeString(buf, " <source dev='%s'", + def->src); + if (def->nseclabels) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 8); + for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); + virBufferAdjustIndent(buf, -8); + virBufferAddLit(buf, " </source>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + break; + case VIR_DOMAIN_DISK_TYPE_DIR: + virBufferEscapeString(buf, " <source dir='%s'/>\n", + def->src); + break; + case VIR_DOMAIN_DISK_TYPE_NETWORK: + virBufferAsprintf(buf, " <source protocol='%s'", + virDomainDiskProtocolTypeToString(def->protocol)); + if (def->src) { + virBufferEscapeString(buf, " name='%s'", def->src); + } + if (def->nhosts == 0) { + virBufferAddLit(buf, "/>\n"); + } else { + int i; + + virBufferAddLit(buf, ">\n"); + for (i = 0; i < def->nhosts; i++) { + virBufferAddLit(buf, " <host"); + if (def->hosts[i].name) { + virBufferEscapeString(buf, " name='%s'", def->hosts[i].name); + } + if (def->hosts[i].port) { + virBufferEscapeString(buf, " port='%s'", + def->hosts[i].port); + } + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + if (def->hosts[i].socket) { + virBufferEscapeString(buf, " socket='%s'", def->hosts[i].socket); + } + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " </source>\n"); + } + break; + case VIR_DOMAIN_DISK_TYPE_VOLUME: + /* Parsing guarantees the def->srcpool->volume cannot be NULL + * if def->srcpool->pool is not NULL. + */ + if (def->srcpool->pool) + virBufferAsprintf(buf, " <source pool='%s' volume='%s'/>\n", + def->srcpool->pool, def->srcpool->volume); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk type %s"), + virDomainDiskTypeToString(def->type)); + return -1; + } + } + + return 0; +}

static int virDomainDiskDefFormat(virBufferPtr buf, @@ -12184,10 +12349,8 @@ virDomainDiskDefFormat(virBufferPtr buf, const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd); const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx); const char *copy_on_read = virDomainVirtioEventIdxTypeToString(def->copy_on_read); - const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy); const char *sgio = virDomainDiskSGIOTypeToString(def->sgio);

- int n; char uuidstr[VIR_UUID_STRING_BUFLEN];

if (!type) { @@ -12283,86 +12446,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, " </auth>\n"); }

- if (def->src || def->nhosts > 0 || - def->startupPolicy) { - switch (def->type) { - case VIR_DOMAIN_DISK_TYPE_FILE: - virBufferAddLit(buf," <source"); - if (def->src) - virBufferEscapeString(buf, " file='%s'", def->src); - if (def->startupPolicy) - virBufferEscapeString(buf, " startupPolicy='%s'", - startupPolicy); - if (def->nseclabels) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 8); - for (n = 0; n < def->nseclabels; n++) - virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); - virBufferAdjustIndent(buf, -8); - virBufferAddLit(buf, " </source>\n"); - } else { - virBufferAddLit(buf, "/>\n"); - } - break; - case VIR_DOMAIN_DISK_TYPE_BLOCK: - virBufferEscapeString(buf, " <source dev='%s'", - def->src); - if (def->nseclabels) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 8); - for (n = 0; n < def->nseclabels; n++) - virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); - virBufferAdjustIndent(buf, -8); - virBufferAddLit(buf, " </source>\n"); - } else { - virBufferAddLit(buf, "/>\n"); - } - break; - case VIR_DOMAIN_DISK_TYPE_DIR: - virBufferEscapeString(buf, " <source dir='%s'/>\n", - def->src); - break; - case VIR_DOMAIN_DISK_TYPE_NETWORK: - virBufferAsprintf(buf, " <source protocol='%s'", - virDomainDiskProtocolTypeToString(def->protocol)); - if (def->src) { - virBufferEscapeString(buf, " name='%s'", def->src); - } - if (def->nhosts == 0) { - virBufferAddLit(buf, "/>\n"); - } else { - int i; - - virBufferAddLit(buf, ">\n"); - for (i = 0; i < def->nhosts; i++) { - virBufferAddLit(buf, " <host"); - if (def->hosts[i].name) { - virBufferEscapeString(buf, " name='%s'", def->hosts[i].name); - } - if (def->hosts[i].port) { - virBufferEscapeString(buf, " port='%s'", - def->hosts[i].port); - } - if (def->hosts[i].transport) { - virBufferAsprintf(buf, " transport='%s'", - virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); - } - if (def->hosts[i].socket) { - virBufferEscapeString(buf, " socket='%s'", def->hosts[i].socket); - } - virBufferAddLit(buf, "/>\n"); - } - virBufferAddLit(buf, " </source>\n"); - } - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk type %s"), - virDomainDiskTypeToString(def->type)); - return -1; - } - } - + if (virDomainDiskSourceDefFormat(buf, def) < 0) + return -1; virDomainDiskGeometryDefFormat(buf, def); virDomainDiskBlockIoDefFormat(buf, def);

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9a9e0b1..9919db3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -427,6 +427,7 @@ enum virDomainDiskType { VIR_DOMAIN_DISK_TYPE_FILE, VIR_DOMAIN_DISK_TYPE_DIR, VIR_DOMAIN_DISK_TYPE_NETWORK, + VIR_DOMAIN_DISK_TYPE_VOLUME,

VIR_DOMAIN_DISK_TYPE_LAST }; @@ -585,6 +586,13 @@ struct _virDomainBlockIoTuneInfo { }; typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;

+typedef struct _virDomainDiskSourcePoolDef virDomainDiskSourcePoolDef; +struct _virDomainDiskSourcePoolDef { + char *pool; /* pool name */ + char *volume; /* volume name */ +}; +typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; + /* Stores the virtual disk configuration */ struct _virDomainDiskDef { int type; @@ -596,6 +604,7 @@ struct _virDomainDiskDef { int protocol; size_t nhosts; virDomainDiskHostDefPtr hosts; + virDomainDiskSourcePoolDefPtr srcpool; struct { char *username; int secretType; /* enum virDomainDiskSecretType */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml new file mode 100644 index 0000000..876eebe --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='volume' device='cdrom'> + <source pool='blk-pool0' volume='blk-pool0-vol0'/> + <target dev='hda' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/idedisk.img'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='2'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='ide' index='1'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 160e958..7e174dd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -241,6 +241,7 @@ mymain(void) DO_TEST("disk-scsi-lun-passthrough-sgio");

DO_TEST("disk-scsi-disk-vpd"); + DO_TEST("disk-source-pool");

/* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto");

ACK - although my knowledge of the xml format/parse is still getting up to speed.

On Thu, Jan 31, 2013 at 02:11:27AM +0800, Osier Yang wrote:

With this patch, one can specify the disk source using libvirt storage like:

<disk type='volume' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source pool='default' volume='fc18.img'/> <target dev='vdb' bus='virtio'/> </disk>

"seclables" and "startupPolicy" are not supported for this new disk type ("volume"). They will be supported in later patches. @@ -12168,6 +12237,102 @@ static void virDomainDiskBlockIoDefFormat(virBufferPtr buf, } }

+static int +virDomainDiskSourceDefFormat(virBufferPtr buf, + virDomainDiskDefPtr def) +{ + int n; + const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy); + + if (def->src || def->nhosts > 0 || def->srcpool || + def->startupPolicy) { + switch (def->type) { + case VIR_DOMAIN_DISK_TYPE_FILE: + if (def->src) + virBufferEscapeString(buf, " <source file='%s'", def->src); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); + if (def->nseclabels) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 8); + for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); + virBufferAdjustIndent(buf, -8); + virBufferAddLit(buf, " </source>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + break; + case VIR_DOMAIN_DISK_TYPE_BLOCK: + virBufferEscapeString(buf, " <source dev='%s'", + def->src); + if (def->nseclabels) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 8); + for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); + virBufferAdjustIndent(buf, -8); + virBufferAddLit(buf, " </source>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + break; + case VIR_DOMAIN_DISK_TYPE_DIR: + virBufferEscapeString(buf, " <source dir='%s'/>\n", + def->src); + break; + case VIR_DOMAIN_DISK_TYPE_NETWORK: + virBufferAsprintf(buf, " <source protocol='%s'", + virDomainDiskProtocolTypeToString(def->protocol)); + if (def->src) { + virBufferEscapeString(buf, " name='%s'", def->src); + } + if (def->nhosts == 0) { + virBufferAddLit(buf, "/>\n"); + } else { + int i; + + virBufferAddLit(buf, ">\n"); + for (i = 0; i < def->nhosts; i++) { + virBufferAddLit(buf, " <host"); + if (def->hosts[i].name) { + virBufferEscapeString(buf, " name='%s'", def->hosts[i].name); + } + if (def->hosts[i].port) { + virBufferEscapeString(buf, " port='%s'", + def->hosts[i].port); + } + if (def->hosts[i].transport) { + virBufferAsprintf(buf, " transport='%s'", + virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport)); + } + if (def->hosts[i].socket) { + virBufferEscapeString(buf, " socket='%s'", def->hosts[i].socket); + } + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " </source>\n"); + } + break; + case VIR_DOMAIN_DISK_TYPE_VOLUME: + /* Parsing guarantees the def->srcpool->volume cannot be NULL + * if def->srcpool->pool is not NULL. + */ + if (def->srcpool->pool) + virBufferAsprintf(buf, " <source pool='%s' volume='%s'/>\n", + def->srcpool->pool, def->srcpool->volume); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk type %s"), + virDomainDiskTypeToString(def->type)); + return -1; + } + } + + return 0; +}

You've got a big refactoring here, mixed in with new features. it is really desirable to separate the refactoring into a patch on its own Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/30/2013 01:11 PM, Osier Yang wrote:

This creates src/conf/domain_storage.h and src/conf/storage_conf.c, which defines a driver contains internal APIs to glue the domain and storage. Currently there is only one API, translateDiskSourcePool, which is to translate the specified pool/volume into the the real underlying disk source. Later patch will implement API. --- src/Makefile.am | 3 ++- src/conf/domain_storage.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_storage.h | 37 +++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 4 ++++ 4 files changed, 87 insertions(+), 1 deletions(-) create mode 100644 src/conf/domain_storage.c create mode 100644 src/conf/domain_storage.h

diff --git a/src/Makefile.am b/src/Makefile.am index 070a089..d664c48 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -203,7 +203,8 @@ DOMAIN_CONF_SOURCES = \ conf/domain_conf.c conf/domain_conf.h \ conf/domain_audit.c conf/domain_audit.h \ conf/domain_nwfilter.c conf/domain_nwfilter.h \ - conf/snapshot_conf.c conf/snapshot_conf.h + conf/snapshot_conf.c conf/snapshot_conf.h \ + conf/domain_storage.c conf/domain_storage.h

DOMAIN_EVENT_SOURCES = \ conf/domain_event.c conf/domain_event.h diff --git a/src/conf/domain_storage.c b/src/conf/domain_storage.c new file mode 100644 index 0000000..299e9fd --- /dev/null +++ b/src/conf/domain_storage.c @@ -0,0 +1,44 @@ +/* + * domain_storage.c: Internal APIs to glue domain and storage + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Osier Yang <jyang@redhat.com> + */ + +#include <config.h> + +#include "internal.h" + +#include "datatypes.h" +#include "domain_conf.h" +#include "domain_storage.h" + +static virDomainStorageDriverPtr domainStorageDriver; + +void +virRegisterDomainStorageDriver(virDomainStorageDriverPtr driver) { + domainStorageDriver = driver; +}

This differs from what I read w/r/t what I read in HACKING which seems to use: void function(args) { code; }

+ +int +virDomainStorageTranslateDiskSourcePool(virConnectPtr conn, + virDomainDefPtr def) { + if (domainStorageDriver != NULL) + return domainStorageDriver->translateDiskSourcePool(conn, def); + return 0; +} diff --git a/src/conf/domain_storage.h b/src/conf/domain_storage.h new file mode 100644 index 0000000..143b476 --- /dev/null +++ b/src/conf/domain_storage.h @@ -0,0 +1,37 @@ +/* + * domain_storage.h: Internal APIs to glue domain and storage + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Osier Yang <jyang@redhat.com> + */ +#ifndef DOMAIN_STORAGE_H +# define DOMAIN_STORAGE_H + +typedef int (*virDomainStorageDiskSourcePoolTranslate)(virConnectPtr conn, + virDomainDefPtr def); +struct _virDomainStorageDriver { + virDomainStorageDiskSourcePoolTranslate translateDiskSourcePool; +}; +typedef struct _virDomainStorageDriver virDomainStorageDriver; +typedef virDomainStorageDriver *virDomainStorageDriverPtr; + +void virRegisterDomainStorageDriver(virDomainStorageDriverPtr driver); +int virDomainStorageTranslateDiskSourcePool(virConnectPtr conn, + virDomainDefPtr def); + +#endif /* DOMAIN_STORAGE_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c589236..a44f3ff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -617,6 +617,10 @@ virDomainConfNWFilterTeardown; virDomainConfVMNWFilterTeardown;

+# domain_storage.h +virDomainStorageTranslateDiskSourcePool; +virRegisterDomainStorageDriver; + # ebtables.h ebtablesAddForwardAllowIn; ebtablesAddForwardPolicyReject;

ACK

On 2013年02月04日 22:30, Daniel P. Berrange wrote:

On Thu, Jan 31, 2013 at 02:11:28AM +0800, Osier Yang wrote:

...

This creates src/conf/domain_storage.h and src/conf/storage_conf.c, which defines a driver contains internal APIs to glue the domain and storage. Currently there is only one API, translateDiskSourcePool, which is to translate the specified pool/volume into the the real underlying disk source. Later patch will implement API. --- src/Makefile.am | 3 ++- src/conf/domain_storage.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_storage.h | 37 +++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 4 ++++ 4 files changed, 87 insertions(+), 1 deletions(-) create mode 100644 src/conf/domain_storage.c create mode 100644 src/conf/domain_storage.h

diff --git a/src/Makefile.am b/src/Makefile.am index 070a089..d664c48 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -203,7 +203,8 @@ DOMAIN_CONF_SOURCES = \ conf/domain_conf.c conf/domain_conf.h \ conf/domain_audit.c conf/domain_audit.h \ conf/domain_nwfilter.c conf/domain_nwfilter.h \ - conf/snapshot_conf.c conf/snapshot_conf.h + conf/snapshot_conf.c conf/snapshot_conf.h \ + conf/domain_storage.c conf/domain_storage.h

DOMAIN_EVENT_SOURCES = \ conf/domain_event.c conf/domain_event.h diff --git a/src/conf/domain_storage.c b/src/conf/domain_storage.c new file mode 100644 index 0000000..299e9fd --- /dev/null +++ b/src/conf/domain_storage.c @@ -0,0 +1,44 @@ +/* + * domain_storage.c: Internal APIs to glue domain and storage + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + *<http://www.gnu.org/licenses/>. + * + * Author: Osier Yang<jyang@redhat.com> + */ + +#include<config.h> + +#include "internal.h" + +#include "datatypes.h" +#include "domain_conf.h" +#include "domain_storage.h" + +static virDomainStorageDriverPtr domainStorageDriver; + +void +virRegisterDomainStorageDriver(virDomainStorageDriverPtr driver) { + domainStorageDriver = driver; +} + +int +virDomainStorageTranslateDiskSourcePool(virConnectPtr conn, + virDomainDefPtr def) { + if (domainStorageDriver != NULL) + return domainStorageDriver->translateDiskSourcePool(conn, def); + return 0; +}

IMHO this abstraction is rather pointless. Just call virStoragePoolLookupbyUUID() at the time you need to generate the CLI args, just as we do with the existing <interface type='network'/>

I thought you agreed with using this method on RFC. :-) I'm wondering how much advantage we will get from invoking public API instead of doing the work directly inside the server side. If you look futher more on the later patches, especially 9/9, which shows we will not only need the pool/volume object, but also the pool/volume defs (to get either the underlying target path or the underlying volume desciption, this is different with the domain network, which only needs the network obj), which can't get by the public APIs. Of course, this is not the problem, as we can introduce some internal helpers to get the pool/volume defs. But I'm not sure how much benifit we will get from that way. Osier

On 01/30/2013 01:11 PM, Osier Yang wrote:

It iterates over all the domain disks, and translate the source of all the disks of 'volume' type from storage pool/volume to the real underlying source.

Disks of type 'file', 'block', and 'dir' are supported now. Network type is not supported yet, it will be another patch.

src/storage/storage_driver.c: * New helper storagePoolObjFindByDiskSource to find the specified pool by name. * Implement translateDiskSourcePool as storageTranslateDomainDiskSourcePool --- src/storage/storage_driver.c | 90 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e98c18c..9f60f2c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -47,6 +47,8 @@ #include "virlog.h" #include "virfile.h" #include "fdstream.h" +#include "domain_conf.h" +#include "domain_storage.h" #include "configmake.h"

#define VIR_FROM_THIS VIR_FROM_STORAGE @@ -2367,6 +2369,88 @@ storageListAllPools(virConnectPtr conn, return ret; }

+static virStoragePoolObjPtr +storagePoolObjFindByDiskSource(virConnectPtr conn, + const char *name) +{ + virStorageDriverStatePtr driver = conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, name); + storageDriverUnlock(driver); + + if (!pool) { + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching name '%s'"), + name); + goto error; + } + + if (!virStoragePoolObjIsActive(pool)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("The specified pool '%s' is not active"), + name); + goto error; + } + + return pool; +error: + if (pool) + virStoragePoolObjUnlock(pool); + return NULL; +} + +static int +storageTranslateDomainDiskSourcePool(virConnectPtr conn, + virDomainDefPtr def) +{ + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int i; + int ret = -1; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (disk->type != VIR_DOMAIN_DISK_TYPE_VOLUME) + continue; + + if (!(pool = storagePoolObjFindByDiskSource(conn, disk->srcpool->pool))) + goto cleanup; + + if (!(vol = virStorageVolDefFindByName(pool, disk->srcpool->volume))) { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol of specified pool with " + "matching name '%s'"), + disk->srcpool->volume); s/vol/volume

Since you have the pool name, wouldn't it be better to state that volume "%s" was not found in pool "%s"? Although I now see you just hoisted the message from elsewhere.

+ goto cleanup; + } + + switch (vol->type) { + case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_DIR: + disk->src = strdup(vol->target.path);

And if !disk->src here what assumptions get broken later?

+ break; + case VIR_STORAGE_VOL_NETWORK: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Using network volume as disk source is not supported")); + goto cleanup; + } + + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + ret = 0; + +cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} + static virStorageDriver storageDriver = { .name = "storage", .open = storageOpen, /* 0.4.0 */ @@ -2423,8 +2507,14 @@ static virStateDriver stateDriver = { .reload = storageDriverReload, };

+static virDomainStorageDriver domainStorageDriver = { + .translateDiskSourcePool = storageTranslateDomainDiskSourcePool, +}; + + int storageRegister(void) { virRegisterStorageDriver(&storageDriver); virRegisterStateDriver(&stateDriver); + virRegisterDomainStorageDriver(&domainStorageDriver); return 0; }

It iterates over all the domain disks, and translate the source of all the disks of 'volume' type from storage pool/volume to the real underlying source. Disks of type 'file', 'block', and 'dir' are supported now. Network type is not supported yet, it will be another patch. src/storage/storage_driver.c: * New helper storagePoolObjFindByDiskSource to find the specified pool by name. * Implement translateDiskSourcePool as storageTranslateDomainDiskSourcePool --- src/storage/storage_driver.c | 106 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 106 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e98c18c..a2c3a1a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -47,6 +47,8 @@ #include "virlog.h" #include "virfile.h" #include "fdstream.h" +#include "domain_conf.h" +#include "domain_storage.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -2367,6 +2369,104 @@ storageListAllPools(virConnectPtr conn, return ret; } +static virStoragePoolObjPtr +storagePoolObjFindByDiskSource(virConnectPtr conn, + const char *name) +{ + virStorageDriverStatePtr driver = conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, name); + storageDriverUnlock(driver); + + if (!pool) { + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching name '%s'"), + name); + goto error; + } + + if (!virStoragePoolObjIsActive(pool)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("The specified pool '%s' is not active"), + name); + goto error; + } + + return pool; +error: + if (pool) + virStoragePoolObjUnlock(pool); + return NULL; +} + +static int +storageTranslateDomainDiskSourcePool(virConnectPtr conn, + virDomainDefPtr def) +{ + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int i; + int ret = -1; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (disk->type != VIR_DOMAIN_DISK_TYPE_VOLUME) + continue; + + if (!(pool = storagePoolObjFindByDiskSource(conn, disk->srcpool->pool))) + goto cleanup; + + if (!(vol = virStorageVolDefFindByName(pool, disk->srcpool->volume))) { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage volume of pool '%s' with " + "matching name '%s'"), + disk->srcpool->pool, + disk->srcpool->volume); + goto cleanup; + } + + switch (vol->type) { + case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_DIR: + if (!vol->target.path && + disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("target path of the specified volume '%s' " + "(pool = '%s') is empty"), + disk->srcpool->volume + disk->srcpool->pool); + goto cleanup; + } + + if (vol->target.path && + !(disk->src = strdup(vol->target.path))) { + virReportOOMError(); + goto cleanup; + } + break; + case VIR_STORAGE_VOL_NETWORK: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Using network volume as disk source is not supported")); + goto cleanup; + } + + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + ret = 0; + +cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} + static virStorageDriver storageDriver = { .name = "storage", .open = storageOpen, /* 0.4.0 */ @@ -2423,8 +2523,14 @@ static virStateDriver stateDriver = { .reload = storageDriverReload, }; +static virDomainStorageDriver domainStorageDriver = { + .translateDiskSourcePool = storageTranslateDomainDiskSourcePool, +}; + + int storageRegister(void) { virRegisterStorageDriver(&storageDriver); virRegisterStateDriver(&stateDriver); + virRegisterDomainStorageDriver(&domainStorageDriver); return 0; } -- 1.7.7.6

On 2013年02月04日 22:03, John Ferlan wrote:

On 02/01/2013 12:24 PM, Osier Yang wrote:

...

It iterates over all the domain disks, and translate the source of all the disks of 'volume' type from storage pool/volume to the real underlying source.

Disks of type 'file', 'block', and 'dir' are supported now. Network type is not supported yet, it will be another patch.

src/storage/storage_driver.c: * New helper storagePoolObjFindByDiskSource to find the specified pool by name. * Implement translateDiskSourcePool as storageTranslateDomainDiskSourcePool --- src/storage/storage_driver.c | 106 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 106 insertions(+), 0 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e98c18c..a2c3a1a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -47,6 +47,8 @@ #include "virlog.h" #include "virfile.h" #include "fdstream.h" +#include "domain_conf.h" +#include "domain_storage.h" #include "configmake.h"

#define VIR_FROM_THIS VIR_FROM_STORAGE @@ -2367,6 +2369,104 @@ storageListAllPools(virConnectPtr conn, return ret; }

+static virStoragePoolObjPtr +storagePoolObjFindByDiskSource(virConnectPtr conn, + const char *name) +{ + virStorageDriverStatePtr driver = conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, name); + storageDriverUnlock(driver); + + if (!pool) { + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching name '%s'"), + name); + goto error; + } + + if (!virStoragePoolObjIsActive(pool)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("The specified pool '%s' is not active"), + name); + goto error; + } + + return pool; +error: + if (pool) + virStoragePoolObjUnlock(pool); + return NULL; +} + +static int +storageTranslateDomainDiskSourcePool(virConnectPtr conn, + virDomainDefPtr def) +{ + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int i; + int ret = -1; + + for (i = 0; i< def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (disk->type != VIR_DOMAIN_DISK_TYPE_VOLUME) + continue; + + if (!(pool = storagePoolObjFindByDiskSource(conn, disk->srcpool->pool))) + goto cleanup; + + if (!(vol = virStorageVolDefFindByName(pool, disk->srcpool->volume))) { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage volume of pool '%s' with " + "matching name '%s'"), + disk->srcpool->pool, + disk->srcpool->volume); + goto cleanup; + } + + switch (vol->type) { + case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_DIR: + if (!vol->target.path&& + disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM&& + disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("target path of the specified volume '%s' " + "(pool = '%s') is empty"), + disk->srcpool->volume + disk->srcpool->pool); + goto cleanup; + } + + if (vol->target.path&& + !(disk->src = strdup(vol->target.path))) { + virReportOOMError(); + goto cleanup; + }

Just checking - if !vol->target.path, then disk->src will be empty, is that expected at this point? You've added the first check since the initial pass at this and I want to make sure you've covered your cases. Without comments in the code I'm not sure what is expected.

CD-ROM and Floppy disk allow the disk source be empty. So as long as the code flows to here. It means the disk is either a CD-ROM or a Floppy, as we checked it before. But I agreed with you that adding a comment here will be better. Will add it when pushing.

...

+ break; + case VIR_STORAGE_VOL_NETWORK: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Using network volume as disk source is not supported")); + goto cleanup; + } + + virStoragePoolObjUnlock(pool); + pool = NULL; + } + + ret = 0; + +cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} + static virStorageDriver storageDriver = { .name = "storage", .open = storageOpen, /* 0.4.0 */ @@ -2423,8 +2523,14 @@ static virStateDriver stateDriver = { .reload = storageDriverReload, };

+static virDomainStorageDriver domainStorageDriver = { + .translateDiskSourcePool = storageTranslateDomainDiskSourcePool, +}; + + int storageRegister(void) { virRegisterStorageDriver(&storageDriver); virRegisterStateDriver(&stateDriver); + virRegisterDomainStorageDriver(&domainStorageDriver); return 0; }

ACK (as long as you feel you've covered your cases for the disk storage pool)

John

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

The only two network pools we supported are "rbd" and "sheepdog", and attributes "socket", "transport" are not supported in storage pool conf yet, this uses the default setting (TCP for 'transport', and "socket" is not set) temporarily. Future patches will extend the storage pool conf to support 'transport' and 'socket'. --- src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 71 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a44f3ff..4674bf9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1153,6 +1153,7 @@ virStoragePoolSourceFree; virStoragePoolSourceListFormat; virStoragePoolSourceListNewSource; virStoragePoolTypeFromString; +virStoragePoolTypeToString; virStorageVolDefFindByKey; virStorageVolDefFindByName; virStorageVolDefFindByPath; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 57995a1..2ff1c86 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2406,12 +2406,15 @@ storageTranslateDomainDiskSourcePool(virConnectPtr conn, virDomainDefPtr def) { virStoragePoolObjPtr pool = NULL; - virStorageVolDefPtr vol = NULL; int i; int ret = -1; + virDomainDiskHostDefPtr hosts = NULL; + int nhosts; for (i = 0; i < def->ndisks; i++) { + virStorageVolDefPtr vol = NULL; virDomainDiskDefPtr disk = def->disks[i]; + int n; if (disk->type != VIR_DOMAIN_DISK_TYPE_VOLUME) continue; @@ -2434,6 +2437,15 @@ storageTranslateDomainDiskSourcePool(virConnectPtr conn, goto cleanup; } + if (disk->nseclabels && + vol->type != VIR_STORAGE_VOL_FILE && + vol->type != VIR_STORAGE_VOL_BLOCK) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'seclabels' is only valid for 'file' or " + "'block' type volume")); + goto cleanup; + } + switch (vol->type) { case VIR_STORAGE_VOL_FILE: case VIR_STORAGE_VOL_BLOCK: @@ -2456,20 +2468,67 @@ storageTranslateDomainDiskSourcePool(virConnectPtr conn, } break; case VIR_STORAGE_VOL_NETWORK: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Using network volume as disk source is not supported")); - goto cleanup; - } + if (pool->def->type != VIR_STORAGE_POOL_RBD && + pool->def->type != VIR_STORAGE_POOL_SHEEPDOG) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Using '%s' pool as disk source is " + "not supported"), + virStoragePoolTypeToString(pool->def->type)); + goto cleanup; + } + + if (VIR_ALLOC_N(hosts, pool->def->source.nhost) < 0) { + virReportOOMError(); + goto cleanup; + } + nhosts = pool->def->source.nhost; + + for (n = 0; n < pool->def->source.nhost; n++) { + if (!(hosts[n].name = + strdup(pool->def->source.hosts[n].name))) { + virReportOOMError(); + goto cleanup; + } + + if (virAsprintf(&hosts[n].port, "%d", + pool->def->source.hosts[n].port) < 0) { + virReportOOMError(); + goto cleanup; + } + + + /* XXX: Use the default, as storage pool have not + * supported 'transport' and 'socket' yet. + */ + hosts[n].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + hosts[n].socket = NULL; + } + + disk->hosts = hosts; + disk->nhosts = pool->def->source.nhost; + + if (!(disk->src = strdup(vol->name))) { + virReportOOMError(); + goto cleanup; + } + hosts = NULL; + nhosts = 0; + break; + } virStoragePoolObjUnlock(pool); pool = NULL; } ret = 0; - cleanup: if (pool) virStoragePoolObjUnlock(pool); + while (nhosts > 0) { + virDomainDiskHostDefFree(&hosts[nhosts - 1]); + nhosts--; + } + VIR_FREE(hosts); return ret; } -- 1.7.7.6