On 01/31/2018 02:39 PM, Peter Krempa wrote:
> On Wed, Jan 31, 2018 at 14:34:24 +0100, Michal Privoznik wrote:
>> On 01/31/2018 01:09 PM, Peter Krempa wrote:
>>> On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
>>>> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
>>>> actually used in virsh code. Remove the unused ones.
>>>
>>> I don't think this is well justified. All of the flags are implemented
>>> in virsh code in general.
>>>
>>> I see almost all of them implemented in the cmdList command.
>>
>> We don't have to care about cmdList since it does not use
>> virshDomainNameCompleter. What this patch addresses is use of
>> VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
>> commands accept domains only from a small subset of those. It's safe to
>> say that 99% of commands take active/inactive domains, and the rest 1%
>> is more specific. Anyway, lets try to print out all used flags for this
>> completer:
>
> All this information is not present in the commit message. You
> generalize that the "flags are (not) actually used in virsh code", which
> is simply false. The commit message does in no way mention that you are
> specifically talking about usage of the flags in the completion code,
> which leads to false assumptions.
Ah sorry, I though that "virsh*Completer: " prefix was clear enough. It
isn't. Okay, I'll update the commit message before pushing.
>
>>
>> libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
>> cut -d')' -f 1 | sort -u
>>
>> VIR_CONNECT_LIST_DOMAINS_ACTIVE
>> VIR_CONNECT_LIST_DOMAINS_INACTIVE
>> VIR_CONNECT_LIST_DOMAINS_OTHER
>> VIR_CONNECT_LIST_DOMAINS_PAUSED
>> VIR_CONNECT_LIST_DOMAINS_PERSISTENT
>> VIR_CONNECT_LIST_DOMAINS_RUNNING
>>
>>>
>>> You either need to rewrite the commit message or the patch below is
>>> flawed.
>>
>> I guess you just misunderstood this patch.
>
> You did not explain it well enough in the commit message.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
