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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list