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

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.