[libvirt] [PATCH v3 06/18] conf: Rename API's in storage_adapter_conf

[Laine Stump]

On 03/14/2017 07:05 PM, John Ferlan wrote:
>
> On 03/11/2017 09:41 PM, Laine Stump wrote:
>> Ah, but wait! I ACKed this too soon! *This* is the patch that's renaming the functions. It should be changing the arguments in some cases too (and at least one of the names seems wrong).
>>
>>
>> On 03/10/2017 04:10 PM, John Ferlan wrote:
>>> Rename the API's to remove the storage pool source pieces
>> Yeah, if it's consistent in all cases, I can agree with that...
>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/conf/storage_adapter_conf.c | 14 +++++++-------
>>>  src/conf/storage_adapter_conf.h | 14 +++++++-------
>>>  src/conf/storage_conf.c         |  8 ++++----
>>>  src/libvirt_private.syms        |  8 ++++----
>>>  4 files changed, 22 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
>>> index 3a16bcc..4f5b665 100644
>>> --- a/src/conf/storage_adapter_conf.c
>>> +++ b/src/conf/storage_adapter_conf.c
>>> @@ -38,7 +38,7 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapter,
>>>  
>>>  
>>>  void
>>> -virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter)
>>> +virStorageAdapterClear(virStoragePoolSourceAdapterPtr adapter)
>>>  {
>>>      if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>>>          VIR_FREE(adapter->data.fchost.wwnn);
>>> @@ -55,9 +55,9 @@ virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter)
>>>  
>>>  
>>>  int
>>> -virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr source,
>>> -                                    xmlNodePtr node,
>>> -                                    xmlXPathContextPtr ctxt)
>>> +virStorageAdapterParseXML(virStoragePoolSourcePtr source,
>>> +                          xmlNodePtr node,
>>> +                          xmlXPathContextPtr ctxt)
>> This function should take a virStoragePoolSourceAdapterPtr rather than a virStoragePoolSourcePtr.
>>
>>>  {
>>>      int ret = -1;
>>>      xmlNodePtr relnode = ctxt->node;
>>> @@ -177,7 +177,7 @@ virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr source,
>>>  
>>>  
>>>  int
>>> -virStoragePoolSourceAdapterParseValidate(virStoragePoolDefPtr ret)
>>> +virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
>>
>> This function should take a virStoragePoolSourceAdapterPtr rather than virStoragePoolDefPtr, and the name should just be "virStorageAdapterValidate(), since the parsing is already finished, and this function just validates.
>>
> I'd prefer to use virStorageAdapterValidateParse() - as that what it's
> doing validating that the parse was correct.  So is this is a case where
> a verb can turn into an adverb?  (it's a grammar question!)

Yeah, that name makes sense once you explain it. Maybe. Is it really validating that the parse was done correctly? Or is it just validating that the data in the object meets various criteria? Seems like it's the latter. Would you really want to validate the object any differently if it had just been parsed from XML vs. if the object was generated in some other manner? (e.g. some chunk of C code that created the object and filled in attributes programmatically)

>
> I don't think there's any "order" I could choose other than a really
> painful use a long name now only to change to a shorter name approach in
> later patches.

Yeah, I didn't look ahead when I started reviewing the series, so I didn't realize what the end product was going to be. *A lot* of my comments are moot in light of that.


>  I'd like to reduce the amount of churn though just for
> the sake of "name sanity" between the steps that no one will care about
> except when they have to backport something... In which case, gitk is
> your friend because it really makes the what changed when lookup simple.

I've stared straight at it for multiple years now, but just last week thought to try out for the first time the "old Version" and "new Version" buttons in gitk - combine those with a very large number for context lines, and it is truly very useful.

Another nice way to look at diffs is with "git difftool -d -t meld revA..revB". I haven't found a simple way to move from one commit to the next, but once it's displayed it is a very nice format.

(while on the topic of tools, what I would *really* like is a tool like gitk where you could select a chunk and drag it from one commit to another, or drag entire commits around to change the order on a branch, or copy a commit or group of commits from one branch to another (essentially something that would do an entire "git rebase -i master" or "git cherry-pick" at the drag of a mouse). Any suggestions?