[libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available

[Michal Privoznik]

On 4/1/19 1:14 PM, Bjoern Walk wrote:
> Michal Privoznik <mprivozn at redhat.com> [2019-04-01, 10:49AM +0200]:
>> On 4/1/19 10:17 AM, Bjoern Walk wrote:
>>> I intent to have the new API function superseed the existing one, so I
>>> actually want to call virDomainGetStateParams() unconditionally and only
>>> fall back to virshDomainState() when the new API is not there (so that
>>> hopefully at some distant point in the future, we could deprecate the
>>> old stuff).
>>
>> I understand that but consider the following scenario: a shell script that
>> calls 'virsh domstate $dom' every so often. If we switch to new API then
>> this will still fetch expected info for the client but also it will produce
>> an error message logged at the server side (if it is old and doesn't support
>> the new API).
>>
>> That's the main reason I think we should use new API only if necessary, i.e.
>> if --info was requested. But if you still want to prefer the new API we
>> should do it the proper way. That is something among these lines:
>>
>> if (virDomainGetStateParams() < 0) {
>>    if (!last_error || last_error->code != VIR_ERR_NO_SUPPORT)
>>      goto error;
>>
>>    /* fallback */
>>    if (virshDomainState() < 0)
>>      goto error;
>> }
>>
>> Note that we have to check for the reason why vurDomainGetStateParams()
>> failed and fallback on old API if and only if the new API is not supported.
> 
> Which would still generate the server-side error message, right?

Yes, it would.

> If this
> is an issue, it's an issue and I will use your initial suggestion.
> Otherwise, something like the above seems cleaner. I will have to think
> about this.

Well, to be fair we use something like this. For instance when listing 
domains/networks/... But there is a clear advantage of using 'new' 
virConnectListAll*() over 'old' GetNumDomains() + ListDomains() + 
GetDefinedDomains() + ListDefinedDomains() - they are atomic. But what 
is the advantage of using the new API in case of plain 'virsh domstate'?

BTW: We don't really have notion of deprecating an API. We can merely 
say 'don't use this, use that because reasons' in the docs. But we still 
have to fix all APIs / maitain them.

This is the reason I am advocating for using new API only if neccessary.

> 
>>>> Also, printing out some error message would be nice:
>>>
>>> If the new API is not available, I wanted to just silently have the old
>>> behaviour. The domain state and reason can always be retrieve (by means
>>> of the old API) only additional information could be missing. I am not
>>> sure if we should error out if this is the case.
>>
>> I think we should. I mean, if I'd run 'virsh domstate --info' that means I
>> am expecting some extended info on domain state. If I don't get it, isn't
>> that a failure?
> 
> There can also be no additional information available, think different
> domain states or other hypervisors, for which also silently no further
> information is printed. Should we do something about this scenario too?
> 

No. That is perfectly okay. The API succeeded, but there is no 
additional info.

Michal