[libvirt] [PATCH 0/7] Add support to list Storage Driver backend capabilities
Although I suppose this could have been an RFC - I just went with a v1. I would think at least the first 4 patches are non controversial. Beyond that it depends on what is "expected" as output for the capabilities output for the storage driver. John Ferlan (7): conf: Extract host XML formatting from virCapabilitiesFormatXML conf: Alter virCapabilitiesFormatHostXML to take virCapsHostPtr conf: Extract guest XML formatting from virCapabilitiesFormatXML conf: Alter virCapabilitiesFormatGuestXML to take virCapsGuestPtr conf: Introduce storage pool functions into capabilities storage: Process storage pool capabilities storage: Add storage backend pool/vol API's to capability output src/conf/capabilities.c | 449 ++++++++++++++++++++++------------ src/conf/capabilities.h | 19 ++ src/conf/virstorageobj.h | 4 + src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 65 +++++ src/storage/storage_backend.h | 3 + src/storage/storage_driver.c | 17 ++ 7 files changed, 400 insertions(+), 158 deletions(-) -- 2.20.1
Let's extract out the <host> code into it's own method/helper. NB: One minor change between the two is usage of "buf" instead of "&buf" in the new code since we pass the address of &buf to the helper. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 135 ++++++++++++++++++++++------------------ 1 file changed, 76 insertions(+), 59 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 8e9bba0dbe..33c0c37fbf 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1056,130 +1056,147 @@ virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf, return 0; } -/** - * virCapabilitiesFormatXML: - * @caps: capabilities to format - * - * Convert the capabilities object into an XML representation - * - * Returns the XML document as a string - */ -char * -virCapabilitiesFormatXML(virCapsPtr caps) + +static int +virCapabilitiesFormatHostXML(virCapsPtr caps, + virBufferPtr buf) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - size_t i, j, k; + size_t i, j; char host_uuid[VIR_UUID_STRING_BUFLEN]; - virBufferAddLit(&buf, "<capabilities>\n\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAddLit(&buf, "<host>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<host>\n"); + virBufferAdjustIndent(buf, 2); if (virUUIDIsValid(caps->host.host_uuid)) { virUUIDFormat(caps->host.host_uuid, host_uuid); - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", host_uuid); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", host_uuid); } - virBufferAddLit(&buf, "<cpu>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<cpu>\n"); + virBufferAdjustIndent(buf, 2); if (caps->host.arch) - virBufferAsprintf(&buf, "<arch>%s</arch>\n", + virBufferAsprintf(buf, "<arch>%s</arch>\n", virArchToString(caps->host.arch)); if (caps->host.nfeatures) { - virBufferAddLit(&buf, "<features>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); for (i = 0; i < caps->host.nfeatures; i++) { - virBufferAsprintf(&buf, "<%s/>\n", + virBufferAsprintf(buf, "<%s/>\n", caps->host.features[i]); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</features>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); } - virCPUDefFormatBuf(&buf, caps->host.cpu); + virCPUDefFormatBuf(buf, caps->host.cpu); for (i = 0; i < caps->host.nPagesSize; i++) { - virBufferAsprintf(&buf, "<pages unit='KiB' size='%u'/>\n", + virBufferAsprintf(buf, "<pages unit='KiB' size='%u'/>\n", caps->host.pagesSize[i]); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</cpu>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</cpu>\n"); /* The PM query was successful. */ if (caps->host.powerMgmt) { /* The host supports some PM features. */ unsigned int pm = caps->host.powerMgmt; - virBufferAddLit(&buf, "<power_management>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<power_management>\n"); + virBufferAdjustIndent(buf, 2); while (pm) { int bit = ffs(pm) - 1; - virBufferAsprintf(&buf, "<%s/>\n", + virBufferAsprintf(buf, "<%s/>\n", virCapsHostPMTargetTypeToString(bit)); pm &= ~(1U << bit); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</power_management>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</power_management>\n"); } else { /* The host does not support any PM feature. */ - virBufferAddLit(&buf, "<power_management/>\n"); + virBufferAddLit(buf, "<power_management/>\n"); } - virBufferAsprintf(&buf, "<iommu support='%s'/>\n", + virBufferAsprintf(buf, "<iommu support='%s'/>\n", caps->host.iommu ? "yes" : "no"); if (caps->host.offlineMigrate) { - virBufferAddLit(&buf, "<migration_features>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<migration_features>\n"); + virBufferAdjustIndent(buf, 2); if (caps->host.liveMigrate) - virBufferAddLit(&buf, "<live/>\n"); + virBufferAddLit(buf, "<live/>\n"); if (caps->host.nmigrateTrans) { - virBufferAddLit(&buf, "<uri_transports>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<uri_transports>\n"); + virBufferAdjustIndent(buf, 2); for (i = 0; i < caps->host.nmigrateTrans; i++) { - virBufferAsprintf(&buf, "<uri_transport>%s</uri_transport>\n", + virBufferAsprintf(buf, "<uri_transport>%s</uri_transport>\n", caps->host.migrateTrans[i]); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</uri_transports>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</uri_transports>\n"); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</migration_features>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</migration_features>\n"); } if (caps->host.netprefix) - virBufferAsprintf(&buf, "<netprefix>%s</netprefix>\n", + virBufferAsprintf(buf, "<netprefix>%s</netprefix>\n", caps->host.netprefix); if (caps->host.nnumaCell && - virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell, + virCapabilitiesFormatNUMATopology(buf, caps->host.nnumaCell, caps->host.numaCell) < 0) goto error; - if (virCapabilitiesFormatCaches(&buf, &caps->host.cache) < 0) + if (virCapabilitiesFormatCaches(buf, &caps->host.cache) < 0) goto error; - if (virCapabilitiesFormatMemoryBandwidth(&buf, &caps->host.memBW) < 0) + if (virCapabilitiesFormatMemoryBandwidth(buf, &caps->host.memBW) < 0) goto error; for (i = 0; i < caps->host.nsecModels; i++) { - virBufferAddLit(&buf, "<secmodel>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<model>%s</model>\n", + virBufferAddLit(buf, "<secmodel>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<model>%s</model>\n", caps->host.secModels[i].model); - virBufferAsprintf(&buf, "<doi>%s</doi>\n", + virBufferAsprintf(buf, "<doi>%s</doi>\n", caps->host.secModels[i].doi); for (j = 0; j < caps->host.secModels[i].nlabels; j++) { - virBufferAsprintf(&buf, "<baselabel type='%s'>%s</baselabel>\n", + virBufferAsprintf(buf, "<baselabel type='%s'>%s</baselabel>\n", caps->host.secModels[i].labels[j].type, caps->host.secModels[i].labels[j].label); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</secmodel>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</secmodel>\n"); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</host>\n\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</host>\n\n"); + + return 0; + + error: + return -1; +} + + +/** + * virCapabilitiesFormatXML: + * @caps: capabilities to format + * + * Convert the capabilities object into an XML representation + * + * Returns the XML document as a string + */ +char * +virCapabilitiesFormatXML(virCapsPtr caps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + size_t i, j, k; + virBufferAddLit(&buf, "<capabilities>\n\n"); + virBufferAdjustIndent(&buf, 2); + + if (virCapabilitiesFormatHostXML(caps, &buf) < 0) + goto error; for (i = 0; i < caps->nguests; i++) { virBufferAddLit(&buf, "<guest>\n"); -- 2.20.1
Rather than deref off of "caps->host.", let's pass "&caps->host" and make the helper use "host->" instead. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 66 ++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 33c0c37fbf..863cd06a8d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1058,7 +1058,7 @@ virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf, static int -virCapabilitiesFormatHostXML(virCapsPtr caps, +virCapabilitiesFormatHostXML(virCapsHostPtr host, virBufferPtr buf) { size_t i, j; @@ -1066,40 +1066,40 @@ virCapabilitiesFormatHostXML(virCapsPtr caps, virBufferAddLit(buf, "<host>\n"); virBufferAdjustIndent(buf, 2); - if (virUUIDIsValid(caps->host.host_uuid)) { - virUUIDFormat(caps->host.host_uuid, host_uuid); + if (virUUIDIsValid(host->host_uuid)) { + virUUIDFormat(host->host_uuid, host_uuid); virBufferAsprintf(buf, "<uuid>%s</uuid>\n", host_uuid); } virBufferAddLit(buf, "<cpu>\n"); virBufferAdjustIndent(buf, 2); - if (caps->host.arch) + if (host->arch) virBufferAsprintf(buf, "<arch>%s</arch>\n", - virArchToString(caps->host.arch)); - if (caps->host.nfeatures) { + virArchToString(host->arch)); + if (host->nfeatures) { virBufferAddLit(buf, "<features>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < caps->host.nfeatures; i++) { + for (i = 0; i < host->nfeatures; i++) { virBufferAsprintf(buf, "<%s/>\n", - caps->host.features[i]); + host->features[i]); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</features>\n"); } - virCPUDefFormatBuf(buf, caps->host.cpu); + virCPUDefFormatBuf(buf, host->cpu); - for (i = 0; i < caps->host.nPagesSize; i++) { + for (i = 0; i < host->nPagesSize; i++) { virBufferAsprintf(buf, "<pages unit='KiB' size='%u'/>\n", - caps->host.pagesSize[i]); + host->pagesSize[i]); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</cpu>\n"); /* The PM query was successful. */ - if (caps->host.powerMgmt) { + if (host->powerMgmt) { /* The host supports some PM features. */ - unsigned int pm = caps->host.powerMgmt; + unsigned int pm = host->powerMgmt; virBufferAddLit(buf, "<power_management>\n"); virBufferAdjustIndent(buf, 2); while (pm) { @@ -1116,19 +1116,19 @@ virCapabilitiesFormatHostXML(virCapsPtr caps, } virBufferAsprintf(buf, "<iommu support='%s'/>\n", - caps->host.iommu ? "yes" : "no"); + host->iommu ? "yes" : "no"); - if (caps->host.offlineMigrate) { + if (host->offlineMigrate) { virBufferAddLit(buf, "<migration_features>\n"); virBufferAdjustIndent(buf, 2); - if (caps->host.liveMigrate) + if (host->liveMigrate) virBufferAddLit(buf, "<live/>\n"); - if (caps->host.nmigrateTrans) { + if (host->nmigrateTrans) { virBufferAddLit(buf, "<uri_transports>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < caps->host.nmigrateTrans; i++) { + for (i = 0; i < host->nmigrateTrans; i++) { virBufferAsprintf(buf, "<uri_transport>%s</uri_transport>\n", - caps->host.migrateTrans[i]); + host->migrateTrans[i]); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</uri_transports>\n"); @@ -1137,32 +1137,32 @@ virCapabilitiesFormatHostXML(virCapsPtr caps, virBufferAddLit(buf, "</migration_features>\n"); } - if (caps->host.netprefix) + if (host->netprefix) virBufferAsprintf(buf, "<netprefix>%s</netprefix>\n", - caps->host.netprefix); + host->netprefix); - if (caps->host.nnumaCell && - virCapabilitiesFormatNUMATopology(buf, caps->host.nnumaCell, - caps->host.numaCell) < 0) + if (host->nnumaCell && + virCapabilitiesFormatNUMATopology(buf, host->nnumaCell, + host->numaCell) < 0) goto error; - if (virCapabilitiesFormatCaches(buf, &caps->host.cache) < 0) + if (virCapabilitiesFormatCaches(buf, &host->cache) < 0) goto error; - if (virCapabilitiesFormatMemoryBandwidth(buf, &caps->host.memBW) < 0) + if (virCapabilitiesFormatMemoryBandwidth(buf, &host->memBW) < 0) goto error; - for (i = 0; i < caps->host.nsecModels; i++) { + for (i = 0; i < host->nsecModels; i++) { virBufferAddLit(buf, "<secmodel>\n"); virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<model>%s</model>\n", - caps->host.secModels[i].model); + host->secModels[i].model); virBufferAsprintf(buf, "<doi>%s</doi>\n", - caps->host.secModels[i].doi); - for (j = 0; j < caps->host.secModels[i].nlabels; j++) { + host->secModels[i].doi); + for (j = 0; j < host->secModels[i].nlabels; j++) { virBufferAsprintf(buf, "<baselabel type='%s'>%s</baselabel>\n", - caps->host.secModels[i].labels[j].type, - caps->host.secModels[i].labels[j].label); + host->secModels[i].labels[j].type, + host->secModels[i].labels[j].label); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</secmodel>\n"); @@ -1195,7 +1195,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAddLit(&buf, "<capabilities>\n\n"); virBufferAdjustIndent(&buf, 2); - if (virCapabilitiesFormatHostXML(caps, &buf) < 0) + if (virCapabilitiesFormatHostXML(&caps->host, &buf) < 0) goto error; for (i = 0; i < caps->nguests; i++) { -- 2.20.1
Let's extract out the <guest> code into it's own method/helper. NB: One minor change between the two is usage of "buf" instead of "&buf" in the new code since we pass the address of &buf to the helper. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 112 ++++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 51 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 863cd06a8d..c320c0f72d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1178,91 +1178,77 @@ virCapabilitiesFormatHostXML(virCapsHostPtr host, } -/** - * virCapabilitiesFormatXML: - * @caps: capabilities to format - * - * Convert the capabilities object into an XML representation - * - * Returns the XML document as a string - */ -char * -virCapabilitiesFormatXML(virCapsPtr caps) +static void +virCapabilitiesFormatGuestXML(virCapsPtr caps, + virBufferPtr buf) { - virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i, j, k; - virBufferAddLit(&buf, "<capabilities>\n\n"); - virBufferAdjustIndent(&buf, 2); - - if (virCapabilitiesFormatHostXML(&caps->host, &buf) < 0) - goto error; - for (i = 0; i < caps->nguests; i++) { - virBufferAddLit(&buf, "<guest>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<os_type>%s</os_type>\n", + virBufferAddLit(buf, "<guest>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<os_type>%s</os_type>\n", virDomainOSTypeToString(caps->guests[i]->ostype)); if (caps->guests[i]->arch.id) - virBufferAsprintf(&buf, "<arch name='%s'>\n", + virBufferAsprintf(buf, "<arch name='%s'>\n", virArchToString(caps->guests[i]->arch.id)); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<wordsize>%d</wordsize>\n", + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<wordsize>%d</wordsize>\n", caps->guests[i]->arch.wordsize); if (caps->guests[i]->arch.defaultInfo.emulator) - virBufferAsprintf(&buf, "<emulator>%s</emulator>\n", + virBufferAsprintf(buf, "<emulator>%s</emulator>\n", caps->guests[i]->arch.defaultInfo.emulator); if (caps->guests[i]->arch.defaultInfo.loader) - virBufferAsprintf(&buf, "<loader>%s</loader>\n", + virBufferAsprintf(buf, "<loader>%s</loader>\n", caps->guests[i]->arch.defaultInfo.loader); for (j = 0; j < caps->guests[i]->arch.defaultInfo.nmachines; j++) { virCapsGuestMachinePtr machine = caps->guests[i]->arch.defaultInfo.machines[j]; - virBufferAddLit(&buf, "<machine"); + virBufferAddLit(buf, "<machine"); if (machine->canonical) - virBufferAsprintf(&buf, " canonical='%s'", machine->canonical); + virBufferAsprintf(buf, " canonical='%s'", machine->canonical); if (machine->maxCpus > 0) - virBufferAsprintf(&buf, " maxCpus='%d'", machine->maxCpus); - virBufferAsprintf(&buf, ">%s</machine>\n", machine->name); + virBufferAsprintf(buf, " maxCpus='%d'", machine->maxCpus); + virBufferAsprintf(buf, ">%s</machine>\n", machine->name); } for (j = 0; j < caps->guests[i]->arch.ndomains; j++) { - virBufferAsprintf(&buf, "<domain type='%s'", + virBufferAsprintf(buf, "<domain type='%s'", virDomainVirtTypeToString(caps->guests[i]->arch.domains[j]->type)); if (!caps->guests[i]->arch.domains[j]->info.emulator && !caps->guests[i]->arch.domains[j]->info.loader && !caps->guests[i]->arch.domains[j]->info.nmachines) { - virBufferAddLit(&buf, "/>\n"); + virBufferAddLit(buf, "/>\n"); continue; } - virBufferAddLit(&buf, ">\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); if (caps->guests[i]->arch.domains[j]->info.emulator) - virBufferAsprintf(&buf, "<emulator>%s</emulator>\n", + virBufferAsprintf(buf, "<emulator>%s</emulator>\n", caps->guests[i]->arch.domains[j]->info.emulator); if (caps->guests[i]->arch.domains[j]->info.loader) - virBufferAsprintf(&buf, "<loader>%s</loader>\n", + virBufferAsprintf(buf, "<loader>%s</loader>\n", caps->guests[i]->arch.domains[j]->info.loader); for (k = 0; k < caps->guests[i]->arch.domains[j]->info.nmachines; k++) { virCapsGuestMachinePtr machine = caps->guests[i]->arch.domains[j]->info.machines[k]; - virBufferAddLit(&buf, "<machine"); + virBufferAddLit(buf, "<machine"); if (machine->canonical) - virBufferAsprintf(&buf, " canonical='%s'", machine->canonical); + virBufferAsprintf(buf, " canonical='%s'", machine->canonical); if (machine->maxCpus > 0) - virBufferAsprintf(&buf, " maxCpus='%d'", machine->maxCpus); - virBufferAsprintf(&buf, ">%s</machine>\n", machine->name); + virBufferAsprintf(buf, " maxCpus='%d'", machine->maxCpus); + virBufferAsprintf(buf, ">%s</machine>\n", machine->name); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</domain>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</domain>\n"); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</arch>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</arch>\n"); if (caps->guests[i]->nfeatures) { - virBufferAddLit(&buf, "<features>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); for (j = 0; j < caps->guests[i]->nfeatures; j++) { if (STREQ(caps->guests[i]->features[j]->name, "pae") || @@ -1270,22 +1256,46 @@ virCapabilitiesFormatXML(virCapsPtr caps) STREQ(caps->guests[i]->features[j]->name, "ia64_be") || STREQ(caps->guests[i]->features[j]->name, "cpuselection") || STREQ(caps->guests[i]->features[j]->name, "deviceboot")) { - virBufferAsprintf(&buf, "<%s/>\n", + virBufferAsprintf(buf, "<%s/>\n", caps->guests[i]->features[j]->name); } else { - virBufferAsprintf(&buf, "<%s default='%s' toggle='%s'/>\n", + virBufferAsprintf(buf, "<%s default='%s' toggle='%s'/>\n", caps->guests[i]->features[j]->name, caps->guests[i]->features[j]->defaultOn ? "on" : "off", caps->guests[i]->features[j]->toggle ? "yes" : "no"); } } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</features>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</guest>\n\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</guest>\n\n"); } +} + + +/** + * virCapabilitiesFormatXML: + * @caps: capabilities to format + * + * Convert the capabilities object into an XML representation + * + * Returns the XML document as a string + */ +char * +virCapabilitiesFormatXML(virCapsPtr caps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "<capabilities>\n\n"); + virBufferAdjustIndent(&buf, 2); + + if (virCapabilitiesFormatHostXML(&caps->host, &buf) < 0) + goto error; + + virCapabilitiesFormatGuestXML(caps, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</capabilities>\n"); -- 2.20.1
Rather than deref off of "caps->guests", let's pass "caps->guests" and caps->nguests to have the helper use "guests[i]->" instead. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 71 +++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index c320c0f72d..1269a4c739 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1179,31 +1179,32 @@ virCapabilitiesFormatHostXML(virCapsHostPtr host, static void -virCapabilitiesFormatGuestXML(virCapsPtr caps, +virCapabilitiesFormatGuestXML(virCapsGuestPtr *guests, + size_t nguests, virBufferPtr buf) { size_t i, j, k; - for (i = 0; i < caps->nguests; i++) { + for (i = 0; i < nguests; i++) { virBufferAddLit(buf, "<guest>\n"); virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<os_type>%s</os_type>\n", - virDomainOSTypeToString(caps->guests[i]->ostype)); - if (caps->guests[i]->arch.id) + virDomainOSTypeToString(guests[i]->ostype)); + if (guests[i]->arch.id) virBufferAsprintf(buf, "<arch name='%s'>\n", - virArchToString(caps->guests[i]->arch.id)); + virArchToString(guests[i]->arch.id)); virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<wordsize>%d</wordsize>\n", - caps->guests[i]->arch.wordsize); - if (caps->guests[i]->arch.defaultInfo.emulator) + guests[i]->arch.wordsize); + if (guests[i]->arch.defaultInfo.emulator) virBufferAsprintf(buf, "<emulator>%s</emulator>\n", - caps->guests[i]->arch.defaultInfo.emulator); - if (caps->guests[i]->arch.defaultInfo.loader) + guests[i]->arch.defaultInfo.emulator); + if (guests[i]->arch.defaultInfo.loader) virBufferAsprintf(buf, "<loader>%s</loader>\n", - caps->guests[i]->arch.defaultInfo.loader); + guests[i]->arch.defaultInfo.loader); - for (j = 0; j < caps->guests[i]->arch.defaultInfo.nmachines; j++) { - virCapsGuestMachinePtr machine = caps->guests[i]->arch.defaultInfo.machines[j]; + for (j = 0; j < guests[i]->arch.defaultInfo.nmachines; j++) { + virCapsGuestMachinePtr machine = guests[i]->arch.defaultInfo.machines[j]; virBufferAddLit(buf, "<machine"); if (machine->canonical) virBufferAsprintf(buf, " canonical='%s'", machine->canonical); @@ -1212,26 +1213,26 @@ virCapabilitiesFormatGuestXML(virCapsPtr caps, virBufferAsprintf(buf, ">%s</machine>\n", machine->name); } - for (j = 0; j < caps->guests[i]->arch.ndomains; j++) { + for (j = 0; j < guests[i]->arch.ndomains; j++) { virBufferAsprintf(buf, "<domain type='%s'", - virDomainVirtTypeToString(caps->guests[i]->arch.domains[j]->type)); - if (!caps->guests[i]->arch.domains[j]->info.emulator && - !caps->guests[i]->arch.domains[j]->info.loader && - !caps->guests[i]->arch.domains[j]->info.nmachines) { + virDomainVirtTypeToString(guests[i]->arch.domains[j]->type)); + if (!guests[i]->arch.domains[j]->info.emulator && + !guests[i]->arch.domains[j]->info.loader && + !guests[i]->arch.domains[j]->info.nmachines) { virBufferAddLit(buf, "/>\n"); continue; } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - if (caps->guests[i]->arch.domains[j]->info.emulator) + if (guests[i]->arch.domains[j]->info.emulator) virBufferAsprintf(buf, "<emulator>%s</emulator>\n", - caps->guests[i]->arch.domains[j]->info.emulator); - if (caps->guests[i]->arch.domains[j]->info.loader) + guests[i]->arch.domains[j]->info.emulator); + if (guests[i]->arch.domains[j]->info.loader) virBufferAsprintf(buf, "<loader>%s</loader>\n", - caps->guests[i]->arch.domains[j]->info.loader); + guests[i]->arch.domains[j]->info.loader); - for (k = 0; k < caps->guests[i]->arch.domains[j]->info.nmachines; k++) { - virCapsGuestMachinePtr machine = caps->guests[i]->arch.domains[j]->info.machines[k]; + for (k = 0; k < guests[i]->arch.domains[j]->info.nmachines; k++) { + virCapsGuestMachinePtr machine = guests[i]->arch.domains[j]->info.machines[k]; virBufferAddLit(buf, "<machine"); if (machine->canonical) virBufferAsprintf(buf, " canonical='%s'", machine->canonical); @@ -1246,23 +1247,23 @@ virCapabilitiesFormatGuestXML(virCapsPtr caps, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</arch>\n"); - if (caps->guests[i]->nfeatures) { + if (guests[i]->nfeatures) { virBufferAddLit(buf, "<features>\n"); virBufferAdjustIndent(buf, 2); - for (j = 0; j < caps->guests[i]->nfeatures; j++) { - if (STREQ(caps->guests[i]->features[j]->name, "pae") || - STREQ(caps->guests[i]->features[j]->name, "nonpae") || - STREQ(caps->guests[i]->features[j]->name, "ia64_be") || - STREQ(caps->guests[i]->features[j]->name, "cpuselection") || - STREQ(caps->guests[i]->features[j]->name, "deviceboot")) { + for (j = 0; j < guests[i]->nfeatures; j++) { + if (STREQ(guests[i]->features[j]->name, "pae") || + STREQ(guests[i]->features[j]->name, "nonpae") || + STREQ(guests[i]->features[j]->name, "ia64_be") || + STREQ(guests[i]->features[j]->name, "cpuselection") || + STREQ(guests[i]->features[j]->name, "deviceboot")) { virBufferAsprintf(buf, "<%s/>\n", - caps->guests[i]->features[j]->name); + guests[i]->features[j]->name); } else { virBufferAsprintf(buf, "<%s default='%s' toggle='%s'/>\n", - caps->guests[i]->features[j]->name, - caps->guests[i]->features[j]->defaultOn ? "on" : "off", - caps->guests[i]->features[j]->toggle ? "yes" : "no"); + guests[i]->features[j]->name, + guests[i]->features[j]->defaultOn ? "on" : "off", + guests[i]->features[j]->toggle ? "yes" : "no"); } } @@ -1294,7 +1295,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) if (virCapabilitiesFormatHostXML(&caps->host, &buf) < 0) goto error; - virCapabilitiesFormatGuestXML(caps, &buf); + virCapabilitiesFormatGuestXML(caps->guests, caps->nguests, &buf); virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</capabilities>\n"); -- 2.20.1
https://bugzilla.redhat.com/show_bug.cgi?id=1581670 Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as: <pool> <type>dir</pool> <pool> <type>fs</pool> </pool> ... <pool> <type>iscsi-direct</pool> </pool> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/conf/capabilities.h | 15 ++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 76 insertions(+) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1269a4c739..c60743a38d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -28,6 +28,7 @@ #include "cpu_conf.h" #include "domain_conf.h" #include "physmem.h" +#include "storage_conf.h" #include "viralloc.h" #include "virarch.h" #include "virbuffer.h" @@ -180,6 +181,17 @@ virCapabilitiesFreeGuest(virCapsGuestPtr guest) VIR_FREE(guest); } + +static void +virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool) +{ + if (!pool) + return; + + VIR_FREE(pool); +} + + void virCapabilitiesFreeNUMAInfo(virCapsPtr caps) { @@ -221,6 +233,10 @@ virCapsDispose(void *object) virCapsPtr caps = object; size_t i; + for (i = 0; i < caps->npools; i++) + virCapabilitiesFreeStoragePool(caps->pools[i]); + VIR_FREE(caps->pools); + for (i = 0; i < caps->nguests; i++) virCapabilitiesFreeGuest(caps->guests[i]); VIR_FREE(caps->guests); @@ -792,6 +808,30 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, emulator, machinetype); } + +int +virCapabilitiesAddStoragePool(virCapsPtr caps, + int poolType) +{ + virCapsStoragePoolPtr pool; + + if (VIR_ALLOC(pool) < 0) + goto error; + + pool->type = poolType; + + if (VIR_RESIZE_N(caps->pools, caps->npools_max, caps->npools, 1) < 0) + goto error; + caps->pools[caps->npools++] = pool; + + return 0; + + error: + virCapabilitiesFreeStoragePool(pool); + return -1; +} + + static int virCapabilitiesFormatNUMATopology(virBufferPtr buf, size_t ncells, @@ -1276,6 +1316,24 @@ virCapabilitiesFormatGuestXML(virCapsGuestPtr *guests, } +static void +virCapabilitiesFormatStoragePoolXML(virCapsStoragePoolPtr *pools, + size_t npools, + virBufferPtr buf) +{ + size_t i; + + for (i = 0; i < npools; i++) { + virBufferAddLit(buf, "<pool>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<type>%s</pool>\n", + virStoragePoolTypeToString(pools[i]->type)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</pool>\n\n"); + } +} + + /** * virCapabilitiesFormatXML: * @caps: capabilities to format @@ -1297,6 +1355,8 @@ virCapabilitiesFormatXML(virCapsPtr caps) virCapabilitiesFormatGuestXML(caps->guests, caps->nguests, &buf); + virCapabilitiesFormatStoragePoolXML(caps->pools, caps->npools, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</capabilities>\n"); diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 31c2a07a9b..cca1a20949 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -211,6 +211,13 @@ struct _virCapsHost { bool iommu; }; +typedef struct _virCapsStoragePool virCapsStoragePool; +typedef virCapsStoragePool *virCapsStoragePoolPtr; +struct _virCapsStoragePool { + int type; +}; + + typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr, xmlXPathContextPtr, void **); typedef void (*virDomainDefNamespaceFree)(void *); @@ -235,6 +242,10 @@ struct _virCaps { size_t nguests; size_t nguests_max; virCapsGuestPtr *guests; + + size_t npools; + size_t npools_max; + virCapsStoragePoolPtr *pools; }; typedef struct _virCapsDomainData virCapsDomainData; @@ -318,6 +329,10 @@ virCapabilitiesAddGuestFeature(virCapsGuestPtr guest, bool defaultOn, bool toggle); +int +virCapabilitiesAddStoragePool(virCapsPtr caps, + int poolType); + int virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel, const char *type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..9aaa8830e4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -49,6 +49,7 @@ virCapabilitiesAddGuestFeature; virCapabilitiesAddHostFeature; virCapabilitiesAddHostMigrateTransport; virCapabilitiesAddHostNUMACell; +virCapabilitiesAddStoragePool; virCapabilitiesAllocMachines; virCapabilitiesClearHostNUMACellCPUTopology; virCapabilitiesDomainDataLookup; -- 2.20.1
On Tue, Jan 15, 2019 at 08:15:47PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as:
<pool> <type>dir</pool>
<pool> <type>fs</pool> </pool>
...
<pool> <type>iscsi-direct</pool> </pool>
This looks weird, if you look into output of domcapabilities we use different formatting to list type values, so how about this: <pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> </pool> The name of the enum could be 'backend' as well. Pavel
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/conf/capabilities.h | 15 ++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 76 insertions(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1269a4c739..c60743a38d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -28,6 +28,7 @@ #include "cpu_conf.h" #include "domain_conf.h" #include "physmem.h" +#include "storage_conf.h" #include "viralloc.h" #include "virarch.h" #include "virbuffer.h" @@ -180,6 +181,17 @@ virCapabilitiesFreeGuest(virCapsGuestPtr guest) VIR_FREE(guest); }
+ +static void +virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool) +{ + if (!pool) + return; + + VIR_FREE(pool); +} + + void virCapabilitiesFreeNUMAInfo(virCapsPtr caps) { @@ -221,6 +233,10 @@ virCapsDispose(void *object) virCapsPtr caps = object; size_t i;
+ for (i = 0; i < caps->npools; i++) + virCapabilitiesFreeStoragePool(caps->pools[i]); + VIR_FREE(caps->pools); + for (i = 0; i < caps->nguests; i++) virCapabilitiesFreeGuest(caps->guests[i]); VIR_FREE(caps->guests); @@ -792,6 +808,30 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, emulator, machinetype); }
+ +int +virCapabilitiesAddStoragePool(virCapsPtr caps, + int poolType) +{ + virCapsStoragePoolPtr pool; + + if (VIR_ALLOC(pool) < 0) + goto error; + + pool->type = poolType; + + if (VIR_RESIZE_N(caps->pools, caps->npools_max, caps->npools, 1) < 0) + goto error; + caps->pools[caps->npools++] = pool; + + return 0; + + error: + virCapabilitiesFreeStoragePool(pool); + return -1; +} + + static int virCapabilitiesFormatNUMATopology(virBufferPtr buf, size_t ncells, @@ -1276,6 +1316,24 @@ virCapabilitiesFormatGuestXML(virCapsGuestPtr *guests, }
+static void +virCapabilitiesFormatStoragePoolXML(virCapsStoragePoolPtr *pools, + size_t npools, + virBufferPtr buf) +{ + size_t i; + + for (i = 0; i < npools; i++) { + virBufferAddLit(buf, "<pool>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<type>%s</pool>\n", + virStoragePoolTypeToString(pools[i]->type)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</pool>\n\n"); + } +} + + /** * virCapabilitiesFormatXML: * @caps: capabilities to format @@ -1297,6 +1355,8 @@ virCapabilitiesFormatXML(virCapsPtr caps)
virCapabilitiesFormatGuestXML(caps->guests, caps->nguests, &buf);
+ virCapabilitiesFormatStoragePoolXML(caps->pools, caps->npools, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</capabilities>\n");
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 31c2a07a9b..cca1a20949 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -211,6 +211,13 @@ struct _virCapsHost { bool iommu; };
+typedef struct _virCapsStoragePool virCapsStoragePool; +typedef virCapsStoragePool *virCapsStoragePoolPtr; +struct _virCapsStoragePool { + int type; +}; + + typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr, xmlXPathContextPtr, void **); typedef void (*virDomainDefNamespaceFree)(void *); @@ -235,6 +242,10 @@ struct _virCaps { size_t nguests; size_t nguests_max; virCapsGuestPtr *guests; + + size_t npools; + size_t npools_max; + virCapsStoragePoolPtr *pools; };
typedef struct _virCapsDomainData virCapsDomainData; @@ -318,6 +329,10 @@ virCapabilitiesAddGuestFeature(virCapsGuestPtr guest, bool defaultOn, bool toggle);
+int +virCapabilitiesAddStoragePool(virCapsPtr caps, + int poolType); + int virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel, const char *type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..9aaa8830e4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -49,6 +49,7 @@ virCapabilitiesAddGuestFeature; virCapabilitiesAddHostFeature; virCapabilitiesAddHostMigrateTransport; virCapabilitiesAddHostNUMACell; +virCapabilitiesAddStoragePool; virCapabilitiesAllocMachines; virCapabilitiesClearHostNUMACellCPUTopology; virCapabilitiesDomainDataLookup; -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/30/19 3:31 AM, Pavel Hrdina wrote:
On Tue, Jan 15, 2019 at 08:15:47PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as:
<pool> <type>dir</pool>
<pool> <type>fs</pool> </pool>
...
<pool> <type>iscsi-direct</pool> </pool>
This looks weird, if you look into output of domcapabilities we use different formatting to list type values, so how about this:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> </pool>
The name of the enum could be 'backend' as well.
Pavel
This format is fine by me... Keeping enum is fine as well since it follows other examples Do you have any opinions on whether listing the API's supported each pool is a worthwhile effort in any form? Building on the above, the output could be API by API: <pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> <pool_api name='virConnectFindStoragePoolSources'> <value>fs</value> <value>gluster</value> ... </pool_api> ... N pool_api's <vol_api name='virStorageVolUpload'> <value>disk</value> <value>fs</value> ... </vol_api> ... N vol_api's </pool> Tks - John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/conf/capabilities.h | 15 ++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 76 insertions(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1269a4c739..c60743a38d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -28,6 +28,7 @@ #include "cpu_conf.h" #include "domain_conf.h" #include "physmem.h" +#include "storage_conf.h" #include "viralloc.h" #include "virarch.h" #include "virbuffer.h" @@ -180,6 +181,17 @@ virCapabilitiesFreeGuest(virCapsGuestPtr guest) VIR_FREE(guest); }
+ +static void +virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool) +{ + if (!pool) + return; + + VIR_FREE(pool); +} + + void virCapabilitiesFreeNUMAInfo(virCapsPtr caps) { @@ -221,6 +233,10 @@ virCapsDispose(void *object) virCapsPtr caps = object; size_t i;
+ for (i = 0; i < caps->npools; i++) + virCapabilitiesFreeStoragePool(caps->pools[i]); + VIR_FREE(caps->pools); + for (i = 0; i < caps->nguests; i++) virCapabilitiesFreeGuest(caps->guests[i]); VIR_FREE(caps->guests); @@ -792,6 +808,30 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, emulator, machinetype); }
+ +int +virCapabilitiesAddStoragePool(virCapsPtr caps, + int poolType) +{ + virCapsStoragePoolPtr pool; + + if (VIR_ALLOC(pool) < 0) + goto error; + + pool->type = poolType; + + if (VIR_RESIZE_N(caps->pools, caps->npools_max, caps->npools, 1) < 0) + goto error; + caps->pools[caps->npools++] = pool; + + return 0; + + error: + virCapabilitiesFreeStoragePool(pool); + return -1; +} + + static int virCapabilitiesFormatNUMATopology(virBufferPtr buf, size_t ncells, @@ -1276,6 +1316,24 @@ virCapabilitiesFormatGuestXML(virCapsGuestPtr *guests, }
+static void +virCapabilitiesFormatStoragePoolXML(virCapsStoragePoolPtr *pools, + size_t npools, + virBufferPtr buf) +{ + size_t i; + + for (i = 0; i < npools; i++) { + virBufferAddLit(buf, "<pool>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<type>%s</pool>\n", + virStoragePoolTypeToString(pools[i]->type)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</pool>\n\n"); + } +} + + /** * virCapabilitiesFormatXML: * @caps: capabilities to format @@ -1297,6 +1355,8 @@ virCapabilitiesFormatXML(virCapsPtr caps)
virCapabilitiesFormatGuestXML(caps->guests, caps->nguests, &buf);
+ virCapabilitiesFormatStoragePoolXML(caps->pools, caps->npools, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</capabilities>\n");
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 31c2a07a9b..cca1a20949 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -211,6 +211,13 @@ struct _virCapsHost { bool iommu; };
+typedef struct _virCapsStoragePool virCapsStoragePool; +typedef virCapsStoragePool *virCapsStoragePoolPtr; +struct _virCapsStoragePool { + int type; +}; + + typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr, xmlXPathContextPtr, void **); typedef void (*virDomainDefNamespaceFree)(void *); @@ -235,6 +242,10 @@ struct _virCaps { size_t nguests; size_t nguests_max; virCapsGuestPtr *guests; + + size_t npools; + size_t npools_max; + virCapsStoragePoolPtr *pools; };
typedef struct _virCapsDomainData virCapsDomainData; @@ -318,6 +329,10 @@ virCapabilitiesAddGuestFeature(virCapsGuestPtr guest, bool defaultOn, bool toggle);
+int +virCapabilitiesAddStoragePool(virCapsPtr caps, + int poolType); + int virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel, const char *type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..9aaa8830e4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -49,6 +49,7 @@ virCapabilitiesAddGuestFeature; virCapabilitiesAddHostFeature; virCapabilitiesAddHostMigrateTransport; virCapabilitiesAddHostNUMACell; +virCapabilitiesAddStoragePool; virCapabilitiesAllocMachines; virCapabilitiesClearHostNUMACellCPUTopology; virCapabilitiesDomainDataLookup; -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jan 30, 2019 at 10:03:43AM -0500, John Ferlan wrote:
On 1/30/19 3:31 AM, Pavel Hrdina wrote:
On Tue, Jan 15, 2019 at 08:15:47PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as:
<pool> <type>dir</pool>
<pool> <type>fs</pool> </pool>
...
<pool> <type>iscsi-direct</pool> </pool>
This looks weird, if you look into output of domcapabilities we use different formatting to list type values, so how about this:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> </pool>
The name of the enum could be 'backend' as well.
Pavel
This format is fine by me... Keeping enum is fine as well since it follows other examples
Do you have any opinions on whether listing the API's supported each pool is a worthwhile effort in any form? Building on the above, the output could be API by API:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> <pool_api name='virConnectFindStoragePoolSources'> <value>fs</value> <value>gluster</value> ... </pool_api> ... N pool_api's <vol_api name='virStorageVolUpload'> <value>disk</value> <value>fs</value> ... </vol_api> ... N vol_api's
I really don't think we should go down that route. Whether a specific API works with a specific feature is really something that is practically only determined at the time the API is invoked, as whether it works or not may depend on the full set of arguments you pass to the API. IOW, at most I would list which pool driver backends are present in the capabilities. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jan 30, 2019 at 03:55:05PM +0000, Daniel P. Berrangé wrote:
On Wed, Jan 30, 2019 at 10:03:43AM -0500, John Ferlan wrote:
On 1/30/19 3:31 AM, Pavel Hrdina wrote:
On Tue, Jan 15, 2019 at 08:15:47PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as:
<pool> <type>dir</pool>
<pool> <type>fs</pool> </pool>
...
<pool> <type>iscsi-direct</pool> </pool>
This looks weird, if you look into output of domcapabilities we use different formatting to list type values, so how about this:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> </pool>
The name of the enum could be 'backend' as well.
Pavel
This format is fine by me... Keeping enum is fine as well since it follows other examples
Do you have any opinions on whether listing the API's supported each pool is a worthwhile effort in any form? Building on the above, the output could be API by API:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> <pool_api name='virConnectFindStoragePoolSources'> <value>fs</value> <value>gluster</value> ... </pool_api> ... N pool_api's <vol_api name='virStorageVolUpload'> <value>disk</value> <value>fs</value> ... </vol_api> ... N vol_api's
I really don't think we should go down that route. Whether a specific API works with a specific feature is really something that is practically only determined at the time the API is invoked, as whether it works or not may depend on the full set of arguments you pass to the API.
IOW, at most I would list which pool driver backends are present in the capabilities.
I wonder if there's a case to be made for not putting this in the global capabilities at all, and instead creating a virStoragePoolGetCapabilities() API that works in the same way as virDomainGetCapabilities (ie just accepts a pool type as an API parameter. If it returns an error you can assume the pool type is not supported. The XML doc can then be used to report on various enums the storage XML format may or may not support, such as disk file formats for file backends. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jan 30, 2019 at 04:04:07PM +0000, Daniel P. Berrangé wrote:
On Wed, Jan 30, 2019 at 03:55:05PM +0000, Daniel P. Berrangé wrote:
On Wed, Jan 30, 2019 at 10:03:43AM -0500, John Ferlan wrote:
On 1/30/19 3:31 AM, Pavel Hrdina wrote:
On Tue, Jan 15, 2019 at 08:15:47PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as:
<pool> <type>dir</pool>
<pool> <type>fs</pool> </pool>
...
<pool> <type>iscsi-direct</pool> </pool>
This looks weird, if you look into output of domcapabilities we use different formatting to list type values, so how about this:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> </pool>
The name of the enum could be 'backend' as well.
Pavel
This format is fine by me... Keeping enum is fine as well since it follows other examples
Do you have any opinions on whether listing the API's supported each pool is a worthwhile effort in any form? Building on the above, the output could be API by API:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> <pool_api name='virConnectFindStoragePoolSources'> <value>fs</value> <value>gluster</value> ... </pool_api> ... N pool_api's <vol_api name='virStorageVolUpload'> <value>disk</value> <value>fs</value> ... </vol_api> ... N vol_api's
I really don't think we should go down that route. Whether a specific API works with a specific feature is really something that is practically only determined at the time the API is invoked, as whether it works or not may depend on the full set of arguments you pass to the API.
IOW, at most I would list which pool driver backends are present in the capabilities.
I wonder if there's a case to be made for not putting this in the global capabilities at all, and instead creating a
virStoragePoolGetCapabilities()
API that works in the same way as virDomainGetCapabilities (ie just accepts a pool type as an API parameter. If it returns an error you can assume the pool type is not supported. The XML doc can then be used to report on various enums the storage XML format may or may not support, such as disk file formats for file backends.
This might be better than putting storage capabilities into the host capabilities, however, I would not make the API in a way that you would have to call it for every single storage pool type. IMHO management application would rather call one single API which would return single XML with all storage capabilities including a list of supported storage pools and their features. Pavel
On Wed, Jan 30, 2019 at 03:55:05PM +0000, Daniel P. Berrangé wrote:
On Wed, Jan 30, 2019 at 10:03:43AM -0500, John Ferlan wrote:
On 1/30/19 3:31 AM, Pavel Hrdina wrote:
On Tue, Jan 15, 2019 at 08:15:47PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Introduce the bare bones functions to processing capability data for the storage driver. Currently just looking to store and format the storage pool types in output, such as:
<pool> <type>dir</pool>
<pool> <type>fs</pool> </pool>
...
<pool> <type>iscsi-direct</pool> </pool>
This looks weird, if you look into output of domcapabilities we use different formatting to list type values, so how about this:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> </pool>
The name of the enum could be 'backend' as well.
Pavel
This format is fine by me... Keeping enum is fine as well since it follows other examples
Do you have any opinions on whether listing the API's supported each pool is a worthwhile effort in any form? Building on the above, the output could be API by API:
<pool> <enum name='type'> <value>dir</value> <value>fs</value> ... </enum> <pool_api name='virConnectFindStoragePoolSources'> <value>fs</value> <value>gluster</value> ... </pool_api> ... N pool_api's <vol_api name='virStorageVolUpload'> <value>disk</value> <value>fs</value> ... </vol_api> ... N vol_api's
I really don't think we should go down that route. Whether a specific API works with a specific feature is really something that is practically only determined at the time the API is invoked, as whether it works or not may depend on the full set of arguments you pass to the API.
IOW, at most I would list which pool driver backends are present in the capabilities.
Agreed, if we start listing supported storage APIs someone will eventually ask us to list also APIs for hypervisor drivers. It's overkill and we should not do that. Pavel
https://bugzilla.redhat.com/show_bug.cgi?id=1581670 During storage driver backend initialization, let's save which backends are available in the storage pool capabilities. In order to format those, we need add a connectGetCapabilities processor to the storageHypervisorDriver. This allows a storage connection, such as "storage:///system" to find the API and format the results. NB: This is not available from other hypervisor drivers such as QEMU type connections. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.h | 4 ++++ src/storage/storage_backend.c | 16 ++++++++++++++++ src/storage/storage_backend.h | 3 +++ src/storage/storage_driver.c | 17 +++++++++++++++++ 4 files changed, 40 insertions(+) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 1106aa71bd..34adc26df3 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -24,6 +24,8 @@ # include "storage_conf.h" +# include "capabilities.h" + typedef struct _virStoragePoolObj virStoragePoolObj; typedef virStoragePoolObj *virStoragePoolObjPtr; @@ -45,6 +47,8 @@ struct _virStorageDriverState { /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr storageEventState; + + virCapsPtr caps; }; typedef bool diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a54c338cf0..0ccc616db4 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -187,3 +187,19 @@ virStorageBackendForType(int type) type, NULLSTR(virStoragePoolTypeToString(type))); return NULL; } + + +virCapsPtr +virStorageBackendGetCapabilities(void) +{ + virCapsPtr caps; + size_t i; + + if (!(caps = virCapabilitiesNew(virArchFromHost(), false, false))) + return NULL; + + for (i = 0; i < virStorageBackendsCount; i++) + virCapabilitiesAddStoragePool(caps, virStorageBackends[i]->type); + + return caps; +} diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 2b178494ae..c670c66287 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -126,4 +126,7 @@ int virStorageBackendDriversRegister(bool allmodules); int virStorageBackendRegister(virStorageBackendPtr backend); +virCapsPtr +virStorageBackendGetCapabilities(void); + #endif /* LIBVIRT_STORAGE_BACKEND_H */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4a13e90481..04beff6b07 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -300,6 +300,9 @@ storageStateInitialize(bool privileged, driver->storageEventState = virObjectEventStateNew(); + if (!(driver->caps = virStorageBackendGetCapabilities())) + goto error; + storageDriverUnlock(); ret = 0; @@ -368,6 +371,7 @@ storageStateCleanup(void) storageDriverLock(); + virObjectUnref(driver->caps); virObjectUnref(driver->storageEventState); /* free inactive pools */ @@ -577,6 +581,18 @@ storageConnectListStoragePools(virConnectPtr conn, names, maxnames); } + +static char * +storageConnectGetCapabilities(virConnectPtr conn) +{ + + if (virConnectGetCapabilitiesEnsureACL(conn) < 0) + return NULL; + + return virCapabilitiesFormatXML(driver->caps); +} + + static int storageConnectNumOfDefinedStoragePools(virConnectPtr conn) { @@ -2850,6 +2866,7 @@ static virHypervisorDriver storageHypervisorDriver = { .connectIsEncrypted = storageConnectIsEncrypted, /* 4.1.0 */ .connectIsSecure = storageConnectIsSecure, /* 4.1.0 */ .connectIsAlive = storageConnectIsAlive, /* 4.1.0 */ + .connectGetCapabilities = storageConnectGetCapabilities, /* 5.1.0 */ }; static virConnectDriver storageConnectDriver = { -- 2.20.1
On 1/16/19 2:15 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
During storage driver backend initialization, let's save which backends are available in the storage pool capabilities.
In order to format those, we need add a connectGetCapabilities processor to the storageHypervisorDriver. This allows a storage connection, such as "storage:///system" to find the API and format the results.
NB: This is not available from other hypervisor drivers such as QEMU type connections.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.h | 4 ++++ src/storage/storage_backend.c | 16 ++++++++++++++++ src/storage/storage_backend.h | 3 +++ src/storage/storage_driver.c | 17 +++++++++++++++++ 4 files changed, 40 insertions(+)
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 1106aa71bd..34adc26df3 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -24,6 +24,8 @@
# include "storage_conf.h"
+# include "capabilities.h" + typedef struct _virStoragePoolObj virStoragePoolObj; typedef virStoragePoolObj *virStoragePoolObjPtr;
@@ -45,6 +47,8 @@ struct _virStorageDriverState {
/* Immutable pointer, self-locking APIs */ virObjectEventStatePtr storageEventState; + + virCapsPtr caps;
Would be nice to put a comment here: /* Immutable pointer, read only after initialized */
};
typedef bool diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a54c338cf0..0ccc616db4 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -187,3 +187,19 @@ virStorageBackendForType(int type) type, NULLSTR(virStoragePoolTypeToString(type))); return NULL; } + + +virCapsPtr +virStorageBackendGetCapabilities(void) +{ + virCapsPtr caps; + size_t i; + + if (!(caps = virCapabilitiesNew(virArchFromHost(), false, false))) + return NULL; +
The fact that you need to pass virArchFromHost() and false, false means, that we need to abstract our virCaps even more. I mean, what has the host architecture has to do with storage capabilities? Moreover, this is how the beginning of storage caps look like: # virsh -c storage:///system capabilities <capabilities> <host> <cpu> <arch>x86_64</arch> </cpu> <power_management/> <iommu support='no'/> </host>
+ for (i = 0; i < virStorageBackendsCount; i++) + virCapabilitiesAddStoragePool(caps, virStorageBackends[i]->type); + + return caps; +} diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 2b178494ae..c670c66287 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -126,4 +126,7 @@ int virStorageBackendDriversRegister(bool allmodules);
int virStorageBackendRegister(virStorageBackendPtr backend);
+virCapsPtr +virStorageBackendGetCapabilities(void); + #endif /* LIBVIRT_STORAGE_BACKEND_H */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4a13e90481..04beff6b07 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -300,6 +300,9 @@ storageStateInitialize(bool privileged,
driver->storageEventState = virObjectEventStateNew();
+ if (!(driver->caps = virStorageBackendGetCapabilities())) + goto error; +
Initially, I suspected that we want to refresh the caps on every 'virsh capabilities' (just like we're doing in say qemu driver). But this is different because we wouldn't load a storage driver out of the blue (where as admin might install new qemu binaries once we're running). Makes sense. Michal
On 1/29/19 5:18 AM, Michal Privoznik wrote:
On 1/16/19 2:15 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
During storage driver backend initialization, let's save which backends are available in the storage pool capabilities.
In order to format those, we need add a connectGetCapabilities processor to the storageHypervisorDriver. This allows a storage connection, such as "storage:///system" to find the API and format the results.
NB: This is not available from other hypervisor drivers such as QEMU type connections.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.h | 4 ++++ src/storage/storage_backend.c | 16 ++++++++++++++++ src/storage/storage_backend.h | 3 +++ src/storage/storage_driver.c | 17 +++++++++++++++++ 4 files changed, 40 insertions(+)
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 1106aa71bd..34adc26df3 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -24,6 +24,8 @@ # include "storage_conf.h" +# include "capabilities.h" + typedef struct _virStoragePoolObj virStoragePoolObj; typedef virStoragePoolObj *virStoragePoolObjPtr; @@ -45,6 +47,8 @@ struct _virStorageDriverState { /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr storageEventState; + + virCapsPtr caps;
Would be nice to put a comment here: /* Immutable pointer, read only after initialized */
Sure.
}; typedef bool diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a54c338cf0..0ccc616db4 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -187,3 +187,19 @@ virStorageBackendForType(int type) type, NULLSTR(virStoragePoolTypeToString(type))); return NULL; } + + +virCapsPtr +virStorageBackendGetCapabilities(void) +{ + virCapsPtr caps; + size_t i; + + if (!(caps = virCapabilitiesNew(virArchFromHost(), false, false))) + return NULL; +
The fact that you need to pass virArchFromHost() and false, false means, that we need to abstract our virCaps even more. I mean, what has the host architecture has to do with storage capabilities? Moreover, this is how the beginning of storage caps look like:
# virsh -c storage:///system capabilities <capabilities>
<host> <cpu> <arch>x86_64</arch> </cpu> <power_management/> <iommu support='no'/> </host>
Yes, I found it odd; however, from your summary comment "This one is fine on its own, but will need a preceeding patch to abstract virCaps even more." How much abstraction do you really^^infinity want? Usage of ->caps.host is more prevalent than I think it's worth to try and abstract it away such that it would only be formatted if virCapsHostPtr != NULL. There's a lot of code that wants to poke around and perhaps know more about caps.host than should... Certainly more than I'd prefer altering. Or could it be "simple enough" as modifying virCapabilitiesFormatHostXML to return early: /* Without the presence of some data means we have nothing * minimally to format, so just return. */ if (!virUUIDIsValid(host->host_uuid) && !host->arch && !host->powerMgmt && !host->iommu) return 0; Initially I tried !host->cpu, but that tripped up all the tests/qemucaps2xmloutdata/caps_*.xml files. That also means the above call would pass VIR_ARCH_NONE.
+ for (i = 0; i < virStorageBackendsCount; i++) + virCapabilitiesAddStoragePool(caps, virStorageBackends[i]->type); + + return caps; +} diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 2b178494ae..c670c66287 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -126,4 +126,7 @@ int virStorageBackendDriversRegister(bool allmodules); int virStorageBackendRegister(virStorageBackendPtr backend); +virCapsPtr +virStorageBackendGetCapabilities(void); + #endif /* LIBVIRT_STORAGE_BACKEND_H */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4a13e90481..04beff6b07 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -300,6 +300,9 @@ storageStateInitialize(bool privileged, driver->storageEventState = virObjectEventStateNew(); + if (!(driver->caps = virStorageBackendGetCapabilities())) + goto error; +
Initially, I suspected that we want to refresh the caps on every 'virsh capabilities' (just like we're doing in say qemu driver). But this is different because we wouldn't load a storage driver out of the blue (where as admin might install new qemu binaries once we're running). Makes sense.
I can add a comment here has well... Tks - John
https://bugzilla.redhat.com/show_bug.cgi?id=1581670 Add the Storage Pool and Volume API's defined for each generated capability output, such as: <pool> <type>dir</pool> <poolapis> <buildPool/> <refreshPool/> <deletePool/> </poolapis> <volapis> <buildVol/> <buildVolFrom/> <createVol/> <refreshVol/> <deleteVol/> <resizeVol/> <uploadVol/> <downloadVol/> <wipeVol/> </volapis> </pool> ... <pool> <type>iscsi</pool> <poolapis> <findPoolSources/> <startPool/> <refreshPool/> <stopPool/> </poolapis> <volapis> <uploadVol/> <downloadVol/> <wipeVol/> </volapis> </pool> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/capabilities.c | 49 ++++++++++++++++++++++++++++++-- src/conf/capabilities.h | 6 +++- src/storage/storage_backend.c | 53 +++++++++++++++++++++++++++++++++-- 3 files changed, 103 insertions(+), 5 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index c60743a38d..f7e9873f64 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -188,6 +188,8 @@ virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool) if (!pool) return; + VIR_FREE(pool->poolapis); + VIR_FREE(pool->volapis); VIR_FREE(pool); } @@ -811,7 +813,9 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, int virCapabilitiesAddStoragePool(virCapsPtr caps, - int poolType) + int poolType, + const char *poolstr, + const char *volstr) { virCapsStoragePoolPtr pool; @@ -823,7 +827,10 @@ virCapabilitiesAddStoragePool(virCapsPtr caps, if (VIR_RESIZE_N(caps->pools, caps->npools_max, caps->npools, 1) < 0) goto error; caps->pools[caps->npools++] = pool; - + if (VIR_STRDUP(pool->poolapis, poolstr) < 0) + goto error; + if (VIR_STRDUP(pool->volapis, volstr) < 0) + goto error; return 0; error: @@ -1322,15 +1329,53 @@ virCapabilitiesFormatStoragePoolXML(virCapsStoragePoolPtr *pools, virBufferPtr buf) { size_t i; + char **poolapis = NULL; + size_t npoolapis = 0; + char **volapis = NULL; + size_t nvolapis = 0; for (i = 0; i < npools; i++) { + size_t j; + + if (!(poolapis = virStringSplitCount(pools[i]->poolapis, ",", + 0, &npoolapis)) || + !(volapis = virStringSplitCount(pools[i]->volapis, ",", + 0, &nvolapis))) + goto cleanup; + virBufferAddLit(buf, "<pool>\n"); virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<type>%s</pool>\n", virStoragePoolTypeToString(pools[i]->type)); + + virBufferAddLit(buf, "<poolapis>\n"); + virBufferAdjustIndent(buf, 2); + for (j = 0; j < npoolapis; j++) + virBufferAsprintf(buf, "<%s/>\n", poolapis[j]); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</poolapis>\n"); + + virBufferAddLit(buf, "<volapis>\n"); + virBufferAdjustIndent(buf, 2); + for (j = 0; j < nvolapis; j++) + virBufferAsprintf(buf, "<%s/>\n", volapis[j]); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</volapis>\n"); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</pool>\n\n"); + + virStringListFree(poolapis); + poolapis = NULL; + npoolapis = 0; + virStringListFree(volapis); + volapis = NULL; + nvolapis = 0; } + + cleanup: + virStringListFree(poolapis); + virStringListFree(volapis); } diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index cca1a20949..525f19f195 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -215,6 +215,8 @@ typedef struct _virCapsStoragePool virCapsStoragePool; typedef virCapsStoragePool *virCapsStoragePoolPtr; struct _virCapsStoragePool { int type; + char *poolapis; + char *volapis; }; @@ -331,7 +333,9 @@ virCapabilitiesAddGuestFeature(virCapsGuestPtr guest, int virCapabilitiesAddStoragePool(virCapsPtr caps, - int poolType); + int poolType, + const char *poolstr, + const char *volstr); int virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel, diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0ccc616db4..95873c151f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -193,13 +193,62 @@ virCapsPtr virStorageBackendGetCapabilities(void) { virCapsPtr caps; + virBuffer poolbuf = VIR_BUFFER_INITIALIZER; + virBuffer volbuf = VIR_BUFFER_INITIALIZER; size_t i; if (!(caps = virCapabilitiesNew(virArchFromHost(), false, false))) return NULL; - for (i = 0; i < virStorageBackendsCount; i++) - virCapabilitiesAddStoragePool(caps, virStorageBackends[i]->type); +#define BUF_POOL_BACKEND(field) \ + if (backend->field) \ + virBufferAsprintf(&poolbuf, "%s,", #field); + +#define BUF_VOL_BACKEND(field) \ + if (backend->field) \ + virBufferAsprintf(&volbuf, "%s,", #field); + + for (i = 0; i < virStorageBackendsCount; i++) { + char *poolstr = NULL; + char *volstr = NULL; + virStorageBackendPtr backend = virStorageBackends[i]; + + /* NB: checkPool is an internal only mechanism each pool has */ + BUF_POOL_BACKEND(findPoolSources); + BUF_POOL_BACKEND(startPool); + BUF_POOL_BACKEND(buildPool); + BUF_POOL_BACKEND(refreshPool); + BUF_POOL_BACKEND(stopPool); + BUF_POOL_BACKEND(deletePool); + virBufferTrim(&poolbuf, ",", -1); + + BUF_VOL_BACKEND(buildVol); + BUF_VOL_BACKEND(buildVolFrom); + BUF_VOL_BACKEND(createVol); + BUF_VOL_BACKEND(refreshVol); + BUF_VOL_BACKEND(deleteVol); + BUF_VOL_BACKEND(resizeVol); + BUF_VOL_BACKEND(uploadVol); + BUF_VOL_BACKEND(downloadVol); + BUF_VOL_BACKEND(wipeVol); + virBufferTrim(&volbuf, ",", -1); + + if (virBufferCheckError(&poolbuf) < 0 || + virBufferCheckError(&volbuf) < 0) + goto error; + + poolstr = virBufferContentAndReset(&poolbuf); + volstr = virBufferContentAndReset(&volbuf); + virCapabilitiesAddStoragePool(caps, backend->type, poolstr, volstr); + VIR_FREE(poolstr); + VIR_FREE(volstr); + } return caps; + + error: + virBufferFreeAndReset(&poolbuf); + virBufferFreeAndReset(&volbuf); + virObjectUnref(caps); + return NULL; } -- 2.20.1
On 1/16/19 2:15 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Add the Storage Pool and Volume API's defined for each generated capability output, such as:
<pool> <type>dir</pool> <poolapis> <buildPool/> <refreshPool/> <deletePool/> </poolapis> <volapis> <buildVol/> <buildVolFrom/> <createVol/> <refreshVol/> <deleteVol/> <resizeVol/> <uploadVol/> <downloadVol/> <wipeVol/> </volapis> </pool>
...
<pool> <type>iscsi</pool> <poolapis> <findPoolSources/> <startPool/> <refreshPool/> <stopPool/> </poolapis> <volapis> <uploadVol/> <downloadVol/> <wipeVol/> </volapis> </pool>
Frankly, I don't like this. Firstly, our users don't see 'uploadVol' they see 'virStorageVolUpload'. Secondly, I am not sure if we want to be introspectable this much. I mean, our users can just call the API and deal with VIR_ERR_NO_SUPPORT. This looks like an overkill to me. Michal
On 1/29/19 5:18 AM, Michal Privoznik wrote:
On 1/16/19 2:15 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Add the Storage Pool and Volume API's defined for each generated capability output, such as:
<pool> <type>dir</pool> <poolapis> <buildPool/> <refreshPool/> <deletePool/> </poolapis> <volapis> <buildVol/> <buildVolFrom/> <createVol/> <refreshVol/> <deleteVol/> <resizeVol/> <uploadVol/> <downloadVol/> <wipeVol/> </volapis> </pool>
...
<pool> <type>iscsi</pool> <poolapis> <findPoolSources/> <startPool/> <refreshPool/> <stopPool/> </poolapis> <volapis> <uploadVol/> <downloadVol/> <wipeVol/> </volapis> </pool>
Frankly, I don't like this. Firstly, our users don't see 'uploadVol' they see 'virStorageVolUpload'. Secondly, I am not sure if we want to be introspectable this much. I mean, our users can just call the API and deal with VIR_ERR_NO_SUPPORT. This looks like an overkill to me.
That's fine - hence the reason for separation. I guess I was thinking that developers may like to know what API's are supported by each pool - kind of a "features" list of sorts. Using the backend naming scheme is/was kind of a cheat. I'm not against being more creative - ideas are welcome ;-). I think most important is perhaps knowing what volume operations a pool supports or at least the upload/download, wipe, and resize. Without any kind of extra data, the output is kind of boring: <capabilities> <pool> <type>dir</pool> </pool> <pool> <type>fs</pool> </pool> <pool> <type>netfs</pool> </pool> .... <pool> <type>zfs</pool> </pool> </capabilities> John
ping? Tks, John On 1/15/19 8:15 PM, John Ferlan wrote:
Although I suppose this could have been an RFC - I just went with a v1. I would think at least the first 4 patches are non controversial. Beyond that it depends on what is "expected" as output for the capabilities output for the storage driver.
John Ferlan (7): conf: Extract host XML formatting from virCapabilitiesFormatXML conf: Alter virCapabilitiesFormatHostXML to take virCapsHostPtr conf: Extract guest XML formatting from virCapabilitiesFormatXML conf: Alter virCapabilitiesFormatGuestXML to take virCapsGuestPtr conf: Introduce storage pool functions into capabilities storage: Process storage pool capabilities storage: Add storage backend pool/vol API's to capability output
src/conf/capabilities.c | 449 ++++++++++++++++++++++------------ src/conf/capabilities.h | 19 ++ src/conf/virstorageobj.h | 4 + src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 65 +++++ src/storage/storage_backend.h | 3 + src/storage/storage_driver.c | 17 ++ 7 files changed, 400 insertions(+), 158 deletions(-)
On 1/16/19 2:15 AM, John Ferlan wrote:
Although I suppose this could have been an RFC - I just went with a v1. I would think at least the first 4 patches are non controversial. Beyond that it depends on what is "expected" as output for the capabilities output for the storage driver.
John Ferlan (7): conf: Extract host XML formatting from virCapabilitiesFormatXML conf: Alter virCapabilitiesFormatHostXML to take virCapsHostPtr conf: Extract guest XML formatting from virCapabilitiesFormatXML conf: Alter virCapabilitiesFormatGuestXML to take virCapsGuestPtr
ACK to these four ^^
conf: Introduce storage pool functions into capabilities
This one is fine on its own, but will need a preceeding patch to abstract virCaps even more.
storage: Process storage pool capabilities
This one will be okay if you fix those small issues I've raised.
storage: Add storage backend pool/vol API's to capability output
src/conf/capabilities.c | 449 ++++++++++++++++++++++------------ src/conf/capabilities.h | 19 ++ src/conf/virstorageobj.h | 4 + src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 65 +++++ src/storage/storage_backend.h | 3 + src/storage/storage_driver.c | 17 ++ 7 files changed, 400 insertions(+), 158 deletions(-)
Micha
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik -
Pavel Hrdina