On 03/11/2017 12:22 PM, Laine Stump wrote:
> On 03/10/2017 04:10 PM, John Ferlan wrote:
>> Split out the code that munges through the storage pool adapter into
>> helpers - it's about to be moved into it's own source file.
>>
>> This is purely code motion at this point.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/conf/storage_conf.c | 455 ++++++++++++++++++++++++++----------------------
>> 1 file changed, 243 insertions(+), 212 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 8e3b175..1993d3a 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -463,6 +463,128 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
>> }
>>
>> static int
>> +virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr source,
>> + xmlXPathContextPtr ctxt)
>> +{
>> + int ret = -1;
>> + char *adapter_type = NULL;
>> + char *managed = NULL;
>> +
>> + if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) {
>> + if ((source->adapter.type =
>> + virStoragePoolSourceAdapterTypeFromString(adapter_type)) <= 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("Unknown pool adapter type '%s'"),
>> + adapter_type);
>> + goto cleanup;
>> + }
>> +
>> + if (source->adapter.type ==
>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> + source->adapter.data.fchost.parent =
>> + virXPathString("string(./adapter/@parent)", ctxt);
>> + managed = virXPathString("string(./adapter/@managed)", ctxt);
>> + if (managed) {
>> + source->adapter.data.fchost.managed =
>> + virTristateBoolTypeFromString(managed);
>> + if (source->adapter.data.fchost.managed < 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown fc_host managed setting '%s'"),
>> + managed);
>> + goto cleanup;
>> + }
>> + }
>> +
>> + source->adapter.data.fchost.parent_wwnn =
>> + virXPathString("string(./adapter/@parent_wwnn)", ctxt);
>> + source->adapter.data.fchost.parent_wwpn =
>> + virXPathString("string(./adapter/@parent_wwpn)", ctxt);
>> + source->adapter.data.fchost.parent_fabric_wwn =
>> + virXPathString("string(./adapter/@parent_fabric_wwn)", ctxt);
>> +
>> + source->adapter.data.fchost.wwpn =
>> + virXPathString("string(./adapter/@wwpn)", ctxt);
>> + source->adapter.data.fchost.wwnn =
>> + virXPathString("string(./adapter/@wwnn)", ctxt);
>> + } else if (source->adapter.type ==
>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> +
>> + source->adapter.data.scsi_host.name =
>> + virXPathString("string(./adapter/@name)", ctxt);
>> + if (virXPathNode("./adapter/parentaddr", ctxt)) {
>> + xmlNodePtr addrnode = virXPathNode("./adapter/parentaddr/address",
>> + ctxt);
>> + virPCIDeviceAddressPtr addr =
>> + &source->adapter.data.scsi_host.parentaddr;
>> +
>> + if (!addrnode) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Missing scsi_host PCI address element"));
>> + goto cleanup;
>> + }
>> + source->adapter.data.scsi_host.has_parent = true;
>> + if (virPCIDeviceAddressParseXML(addrnode, addr) < 0)
>> + goto cleanup;
>> + if ((virXPathInt("string(./adapter/parentaddr/@unique_id)",
>> + ctxt,
>> + &source->adapter.data.scsi_host.unique_id) < 0) ||
>> + (source->adapter.data.scsi_host.unique_id < 0)) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Missing or invalid scsi adapter "
>> + "'unique_id' value"));
>> + goto cleanup;
>> + }
>> + }
>> + }
>> + } else {
>> + char *wwnn = NULL;
>> + char *wwpn = NULL;
>> + char *parent = NULL;
>> +
>> + /* "type" was not specified in the XML, so we must verify that
>> + * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the
>> + * XML. If any are found, then we cannot just use "name" alone".
>> + */
>> + wwnn = virXPathString("string(./adapter/@wwnn)", ctxt);
>> + wwpn = virXPathString("string(./adapter/@wwpn)", ctxt);
>> + parent = virXPathString("string(./adapter/@parent)", ctxt);
>> +
>> + if (wwnn || wwpn || parent) {
>> + VIR_FREE(wwnn);
>> + VIR_FREE(wwpn);
>> + VIR_FREE(parent);
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Use of 'wwnn', 'wwpn', and 'parent' attributes "
>> + "requires use of the adapter 'type'"));
>> + goto cleanup;
>> + }
>> +
>> + if (virXPathNode("./adapter/parentaddr", ctxt)) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Use of 'parent' element requires use "
>> + "of the adapter 'type'"));
>> + goto cleanup;
>> + }
>> +
>> + /* To keep back-compat, 'type' is not required to specify
>> + * for scsi_host adapter.
>> + */
>> + if ((source->adapter.data.scsi_host.name =
>> + virXPathString("string(./adapter/@name)", ctxt)))
>> + source->adapter.type =
>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
>> + }
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> + VIR_FREE(adapter_type);
>> + VIR_FREE(managed);
>> + return ret;
>> +}
>> +
>> +
>> +static int
>> virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>> virStoragePoolSourcePtr source,
>> int pool_type,
>> @@ -476,8 +598,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>> virStorageAuthDefPtr authdef = NULL;
>> char *name = NULL;
>> char *port = NULL;
>> - char *adapter_type = NULL;
>> - char *managed = NULL;
>> int n;
>>
>> relnode = ctxt->node;
>> @@ -583,110 +703,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>> VIR_STRDUP(source->dir, "/") < 0)
>> goto cleanup;
>>
>> - if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) {
>> - if ((source->adapter.type =
>> - virStoragePoolSourceAdapterTypeFromString(adapter_type)) <= 0) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - _("Unknown pool adapter type '%s'"),
>> - adapter_type);
>> - goto cleanup;
>> - }
>> -
>> - if (source->adapter.type ==
>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> - source->adapter.data.fchost.parent =
>> - virXPathString("string(./adapter/@parent)", ctxt);
>> - managed = virXPathString("string(./adapter/@managed)", ctxt);
>> - if (managed) {
>> - source->adapter.data.fchost.managed =
>> - virTristateBoolTypeFromString(managed);
>> - if (source->adapter.data.fchost.managed < 0) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - _("unknown fc_host managed setting '%s'"),
>> - managed);
>> - goto cleanup;
>> - }
>> - }
>> -
>> - source->adapter.data.fchost.parent_wwnn =
>> - virXPathString("string(./adapter/@parent_wwnn)", ctxt);
>> - source->adapter.data.fchost.parent_wwpn =
>> - virXPathString("string(./adapter/@parent_wwpn)", ctxt);
>> - source->adapter.data.fchost.parent_fabric_wwn =
>> - virXPathString("string(./adapter/@parent_fabric_wwn)", ctxt);
>> -
>> - source->adapter.data.fchost.wwpn =
>> - virXPathString("string(./adapter/@wwpn)", ctxt);
>> - source->adapter.data.fchost.wwnn =
>> - virXPathString("string(./adapter/@wwnn)", ctxt);
>> - } else if (source->adapter.type ==
>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> -
>> - source->adapter.data.scsi_host.name =
>> - virXPathString("string(./adapter/@name)", ctxt);
>> - if (virXPathNode("./adapter/parentaddr", ctxt)) {
>> - xmlNodePtr addrnode = virXPathNode("./adapter/parentaddr/address",
>> - ctxt);
>> - virPCIDeviceAddressPtr addr =
>> - &source->adapter.data.scsi_host.parentaddr;
>> -
>> - if (!addrnode) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("Missing scsi_host PCI address element"));
>> - goto cleanup;
>> - }
>> - source->adapter.data.scsi_host.has_parent = true;
>> - if (virPCIDeviceAddressParseXML(addrnode, addr) < 0)
>> - goto cleanup;
>> - if ((virXPathInt("string(./adapter/parentaddr/@unique_id)",
>> - ctxt,
>> - &source->adapter.data.scsi_host.unique_id) < 0) ||
>> - (source->adapter.data.scsi_host.unique_id < 0)) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("Missing or invalid scsi adapter "
>> - "'unique_id' value"));
>> - goto cleanup;
>> - }
>> - }
>> - }
>> - } else {
>> - char *wwnn = NULL;
>> - char *wwpn = NULL;
>> - char *parent = NULL;
>> -
>> - /* "type" was not specified in the XML, so we must verify that
>> - * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the
>> - * XML. If any are found, then we cannot just use "name" alone".
>> - */
>> - wwnn = virXPathString("string(./adapter/@wwnn)", ctxt);
>> - wwpn = virXPathString("string(./adapter/@wwpn)", ctxt);
>> - parent = virXPathString("string(./adapter/@parent)", ctxt);
>> -
>> - if (wwnn || wwpn || parent) {
>> - VIR_FREE(wwnn);
>> - VIR_FREE(wwpn);
>> - VIR_FREE(parent);
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("Use of 'wwnn', 'wwpn', and 'parent' attributes "
>> - "requires use of the adapter 'type'"));
>> - goto cleanup;
>> - }
>> -
>> - if (virXPathNode("./adapter/parentaddr", ctxt)) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("Use of 'parent' element requires use "
>> - "of the adapter 'type'"));
>> - goto cleanup;
>> - }
>> -
>> - /* To keep back-compat, 'type' is not required to specify
>> - * for scsi_host adapter.
>> - */
>> - if ((source->adapter.data.scsi_host.name =
>> - virXPathString("string(./adapter/@name)", ctxt)))
>> - source->adapter.type =
>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
>> - }
>> + if (virStoragePoolDefParseSourceAdapter(source, ctxt) < 0)
>> + goto cleanup;
>>
>> if ((authnode = virXPathNode("./auth", ctxt))) {
>> if (!(authdef = virStorageAuthDefParse(node->doc, authnode)))
>> @@ -711,8 +729,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>>
>> VIR_FREE(port);
>> VIR_FREE(nodeset);
>> - VIR_FREE(adapter_type);
>> - VIR_FREE(managed);
>> virStorageAuthDefFree(authdef);
>> return ret;
>> }
>> @@ -831,6 +847,74 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>> return ret;
>> }
>>
>> +static int
>> +virStoragePoolSourceAdapterParseValidate(virStoragePoolDefPtr ret)
>
> (looking ahead, I see that at least some of this discussion is moot because you're moving/renaming the same stuff again in later patches, so take it for what its worth...)
All this work started long before that discussion started... The
original goal was to just be sure to remove "StoragePool" from the name
because this same structure was going to be reused inside a domain
controller. A vHBA "kind of" lives between a SCSI hostdev and the
controller used by that hostdev. Unlike a SCSI hostdev which provides
access to a single LUN via it's controller address, this thing would (or
could) provide access to all the LUN's controlled by the vHBA.
Of course in a way I find this strangely similar to (at least) the
(early) discussions regarding how vGPU/mdev's are being
presented/related. There is a single device with a "pool" (of sorts) of
children devices that have use for a guest. Whether all or some are
provided and how they need to be presented is something I was never
clear on, but then again I wasn't paying close attention.
Anyway, that's a different topic...
>
> Since function naming is a hot topic these days....
>
> The first new function you created operates on a virStoragePoolSourcePtr and is named:
>
> virStoragePoolDef Parse SourceAdapter
>
> This function operates on a virStoragePoolDefPtr and is named:
>
> virStoragePoolSourceAdapter Parse Validate
>
> So they are inconsistent about the subject of the function name vs. the argument that is passed.
>
> In the first case, it looks like all uses of the virStoragePoolSourcePtr "source" actually use "source->adapter", so you could change the function to take a virStoragePoolSourceAdapter as its arg (and then either leave it in virSubjectVerbObject format as it is, or change it to virStoragePoolSourceAdapterParse()).
>
> In the 2nd case, I think the "Parse" part is incorrect, since it's only Validating the data, not parsing it, and also, every use of the virStoragePoolDefPtr ("ret") is actually using "ret->source.adapter", so again the arg could be changed to match what's implied in the function name.
>
> But maybe you're just doing this to make it easier to verify that it's plain code motion (and that definitely *was* a help :-)).
Well yes, these are essentially set up for code motion so I didn't have
to rename the functions later. What I did was start with the patch2 from
v2 and work backwards piecing things together. As I went along
destructing the original body of work - I wanted to be sure I wasn't
altering logic in the final target - rather only having to merge in name
changes that would hopefully protect the innocent bystanders.
John
FWIW/IMO: These kind of functions and how we've (recently) historically
name things are where some of the discussions about the proper naming
start to break down. I see us spending too much time focusing on
naming... What I really wouldn't want to see is a spate of patches that
just start "fixing" names - that's busywork...
>
> ACK based on the assumption that the arguments and function names will be made more appropriate in later patches.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
