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

On 4/1/19 1:14 PM, Bjoern Walk wrote: > Michal Privoznik <mprivozn(a)redhat.com&gt; [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.