[libvirt] [PATCH REPOST 1/2] admin: Introduce virAdmClientClose API

[Daniel P. Berrange]

On Thu, May 05, 2016 at 12:22:02PM +0200, Michal Privoznik wrote:
> On 05.05.2016 10:48, Erik Skultety wrote:
> >>> +
> >>> +/**
> >>> + * virAdmClientClose:
> >>> + * @client: a valid client object reference
> >>> + * @flags: extra flags; not used yet, so callers should always pass 0
> >>> + *
> >>> + * Close @client's connection to daemon forcefully.
> >>> + *
> >>> + * Returns 0 if the daemon's connection with @client was closed successfully
> >>> + * or -1 in case of an error.
> >>> + */
> >>> +int virAdmClientClose(virAdmClientPtr client,
> >>> +                      unsigned int flags)
> >>> +{
> >>> +    int ret = -1;
> >>> +
> >>> +    VIR_DEBUG("client=%p, flags=%x", client, flags);
> >>> +    virResetLastError();
> >>> +
> >>> +    virCheckAdmClientGoto(client, error);
> >>> +    virCheckFlagsGoto(0, error);
> >>
> >> Ewww. No. This should be checked on the server side. Not client.
> >>
> > 
> > Well, this approach was used with all the other APIs as well. In my
> > opinion, we don't necessarily need to contact the remote daemon to check
> > for the flags at this moment, since we explicitly document, that we
> > don't support any flags yet. Once we change that and start supporting
> > any flags, it will have to be removed of course, with all the remaining
> > occurrences in libvirt-admin.c and the doc string will need to be
> > modified anyway. Another thing is, if callers would like to use some
> > flags, they'll most probably use our enumerated types or constants for
> > that, for which they will have to update the admin library and once they
> > update, the check will be gone.
> 
> That's not the approach we have in the rest of our calls. The reasoning
> behind is, that if you connect to a newer daemon that already supports
> new flag, you can just pass numeric value (magic constant) to the API
> from your app and everything works. But if we leave this check in, API
> will fail.
> 
> Then, what if you introduce new flag (among with removing this check),
> and then another one (in a different commit). But somebody doing
> backports wants to backport just the latter. Well, they can't because
> the check would still be there.
> 
> Moreover, as we add new flags to our APIs, in general flags known to
> client and to server can be different, because you could have newer
> client connected to older server or vice versa.
> 
> Therefore I don't see much value in this client side check.

Agreed, we never want to check flags in the public API entry points.
It is actively harmful as you describe.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|