Michal Privoznik <mprivozn@redhat.com> [2019-04-01, 02:08PM +0200]:
On 4/1/19 1:14 PM, Bjoern Walk wrote:
Michal Privoznik <mprivozn@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.
Ok, I am convinced now :) Thanks for your advice, I will send v3 after the release.