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

[Bjoern Walk]

Michal Privoznik <mprivozn at redhat.com> [2019-04-01, 02:08PM +0200]:
> 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.

Ok, I am convinced now :)

Thanks for your advice, I will send v3 after the release.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 902 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190401/e9b64fe2/attachment-0001.sig>