Re: [libvirt] [PATCH v3 10/11] admin: Introduce virAdmConnectGetLibVersion
On Thu, Nov 19, 2015 at 10:52:35AM +0100, Erik Skultety wrote:
On 16/11/15 17:42, Martin Kletzander wrote:
>> const ADMIN_PROGRAM = 0x06900690;
>> const ADMIN_PROTOCOL_VERSION = 1;
>> @@ -71,5 +75,10 @@ enum admin_procedure {
>> /**
>> * @generate: none
>> */
>> - ADMIN_PROC_CONNECT_CLOSE = 2
>> + ADMIN_PROC_CONNECT_CLOSE = 2,
>> +
>> + /**
>> + * @generate: both
>> + */
>> + ADMIN_PROC_CONNECT_GET_LIB_VERSION = 3
>
> I was wondering why 'connect' is here, it does not necessarily relate
> to connection and makes the name long, we could start using 'daemon'
> instead as that is what we'll need to add anyway. Also 'lib' seems
> unnecessary here.
Well, this is a matter of consistency with libvirt library. You also
mentioned the general API naming convention that should be met according
to the arguments the API takes in one of your earlier reviews [1]. The
'connect' is there because virAdmConnectGetLibVersion indeed takes
virAdmConnectPtr as its 1st argument and the remote version name of the
API imho should not differ that much.
We want to stay consistent with libvirt, but not with all the wrong
choices that were made along the way. I wish I named the initial
pointer the user gets with virAdmConnect() something like
virAdmDaemonPtr, everything you call with that would start with
virAdmDaemon and would make tad more sense. But I didn't know there
will be a "daemon" back then. Having said that, we *can* change that,
but I already confused myself as to what is the best naming convention
we should go with. Wider discussion about that would be fine and I
think we would achieve a conclusion in less than 5 minutes if there's
more than the two of us and it's not done through mail.
So for now keep it as it is, and we'll add the discussion about naming
to the TODO list of things needed to be done before enabling the admin
api in the daemon.
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)