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.
Michal