[libvirt] [PATCH 0/2] Some trivial admin fixes
Flags checking should be done solely on the daemon side and therefore such checks need to be removed from public APIs. Erik Skultety (2): admin: Remove flags checking from public API entry points admin: Remove flags checking from virAdmConnectOpen public API src/admin/admin_remote.c | 2 +- src/libvirt-admin.c | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) -- 2.4.11
Due to compatibility reasons these should be checked on the server side. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt-admin.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 4ad816b..d7a66ff 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -715,7 +715,6 @@ virAdmConnectListServers(virAdmConnectPtr conn, VIR_DEBUG("conn=%p, servers=%p, flags=%x", conn, servers, flags); virResetLastError(); - virCheckFlagsGoto(0, error); if (servers) *servers = NULL; @@ -756,7 +755,6 @@ virAdmConnectLookupServer(virAdmConnectPtr conn, virCheckAdmConnectGoto(conn, cleanup); virCheckNonNullArgGoto(name, cleanup); - virCheckFlagsGoto(0, cleanup); ret = remoteAdminConnectLookupServer(conn, name, flags); cleanup: @@ -876,7 +874,6 @@ virAdmServerListClients(virAdmServerPtr srv, VIR_DEBUG("srv=%p, clients=%p, flags=%x", srv, clients, flags); virResetLastError(); - virCheckFlagsGoto(0, error); if (clients) *clients = NULL; @@ -916,7 +913,6 @@ virAdmServerLookupClient(virAdmServerPtr srv, virResetLastError(); virCheckAdmServerGoto(srv, error); - virCheckFlagsGoto(0, error); if (!(ret = remoteAdminServerLookupClient(srv, id, flags))) goto error; @@ -966,7 +962,6 @@ virAdmClientGetInfo(virAdmClientPtr client, virResetLastError(); virCheckAdmClientReturn(client, -1); virCheckNonNullArgGoto(params, error); - virCheckFlagsGoto(0, error); if ((ret = remoteAdminClientGetInfo(client, params, nparams, flags)) < 0) goto error; -- 2.4.11
On 05.05.2016 13:49, Erik Skultety wrote:
Due to compatibility reasons these should be checked on the server side.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt-admin.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 4ad816b..d7a66ff 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -715,7 +715,6 @@ virAdmConnectListServers(virAdmConnectPtr conn, VIR_DEBUG("conn=%p, servers=%p, flags=%x", conn, servers, flags);
virResetLastError(); - virCheckFlagsGoto(0, error);
if (servers) *servers = NULL; @@ -756,7 +755,6 @@ virAdmConnectLookupServer(virAdmConnectPtr conn,
virCheckAdmConnectGoto(conn, cleanup); virCheckNonNullArgGoto(name, cleanup); - virCheckFlagsGoto(0, cleanup);
ret = remoteAdminConnectLookupServer(conn, name, flags); cleanup: @@ -876,7 +874,6 @@ virAdmServerListClients(virAdmServerPtr srv, VIR_DEBUG("srv=%p, clients=%p, flags=%x", srv, clients, flags);
virResetLastError(); - virCheckFlagsGoto(0, error);
if (clients) *clients = NULL; @@ -916,7 +913,6 @@ virAdmServerLookupClient(virAdmServerPtr srv, virResetLastError();
virCheckAdmServerGoto(srv, error); - virCheckFlagsGoto(0, error);
if (!(ret = remoteAdminServerLookupClient(srv, id, flags))) goto error; @@ -966,7 +962,6 @@ virAdmClientGetInfo(virAdmClientPtr client, virResetLastError(); virCheckAdmClientReturn(client, -1); virCheckNonNullArgGoto(params, error); - virCheckFlagsGoto(0, error);
if ((ret = remoteAdminClientGetInfo(client, params, nparams, flags)) < 0) goto error;
ACK Michal
Unlike the previous commit, we do actually support one client-side only flag VIR_CONNECT_NO_ALIASES, so besides removing the check for flags this flag has to be masked out before sending a message to the daemon, otherwise it would trigger an error when checking flags on the daemon side. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/admin/admin_remote.c | 2 +- src/libvirt-admin.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 40fcddb..180dd3b 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -162,7 +162,7 @@ remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags) virObjectLock(priv); - args.flags = flags; + args.flags = flags ^ VIR_CONNECT_NO_ALIASES; if (virNetClientRegisterAsyncIO(priv->client) < 0) { VIR_DEBUG("Failed to add event watch, disabling events and support for" diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index d7a66ff..91f40c8 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -185,7 +185,8 @@ virAdmGetDefaultURI(virConfPtr conf) /** * virAdmConnectOpen: * @name: uri of the daemon to connect to, NULL for default - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: extra flags; currently the only supported client-side flag is + * VIR_CONNECT_NO_ALIASES * * Opens connection to admin interface of the daemon. * @@ -204,7 +205,6 @@ virAdmConnectOpen(const char *name, unsigned int flags) VIR_DEBUG("flags=%x", flags); virResetLastError(); - virCheckFlagsGoto(VIR_CONNECT_NO_ALIASES, error); if (!(conn = virAdmConnectNew())) goto error; -- 2.4.11
On Thu, May 05, 2016 at 13:49:24 +0200, Erik Skultety wrote:
Unlike the previous commit, we do actually support one client-side only flag VIR_CONNECT_NO_ALIASES, so besides removing the check for flags this flag has to be masked out before sending a message to the daemon, otherwise it would trigger an error when checking flags on the daemon side.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/admin/admin_remote.c | 2 +- src/libvirt-admin.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 40fcddb..180dd3b 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -162,7 +162,7 @@ remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags)
virObjectLock(priv);
- args.flags = flags; + args.flags = flags ^ VIR_CONNECT_NO_ALIASES;
This won't mask out the flag, this will invert it.
On 05/05/16 14:13, Peter Krempa wrote:
On Thu, May 05, 2016 at 13:49:24 +0200, Erik Skultety wrote:
Unlike the previous commit, we do actually support one client-side only flag VIR_CONNECT_NO_ALIASES, so besides removing the check for flags this flag has to be masked out before sending a message to the daemon, otherwise it would trigger an error when checking flags on the daemon side.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/admin/admin_remote.c | 2 +- src/libvirt-admin.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 40fcddb..180dd3b 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -162,7 +162,7 @@ remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags)
virObjectLock(priv);
- args.flags = flags; + args.flags = flags ^ VIR_CONNECT_NO_ALIASES;
This won't mask out the flag, this will invert it.
Sigh...attempt n.2, see my fix in the attached patch. Erik
On 05.05.2016 13:49, Erik Skultety wrote:
Unlike the previous commit, we do actually support one client-side only flag VIR_CONNECT_NO_ALIASES, so besides removing the check for flags this flag has to be masked out before sending a message to the daemon, otherwise it would trigger an error when checking flags on the daemon side.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/admin/admin_remote.c | 2 +- src/libvirt-admin.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 40fcddb..180dd3b 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -162,7 +162,7 @@ remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags)
virObjectLock(priv);
- args.flags = flags; + args.flags = flags ^ VIR_CONNECT_NO_ALIASES;
As Peter already pointed out, this should be flags & ~VIR_CONNECT_NO_ALIASES;
if (virNetClientRegisterAsyncIO(priv->client) < 0) { VIR_DEBUG("Failed to add event watch, disabling events and support for" diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index d7a66ff..91f40c8 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -185,7 +185,8 @@ virAdmGetDefaultURI(virConfPtr conf) /** * virAdmConnectOpen: * @name: uri of the daemon to connect to, NULL for default - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: extra flags; currently the only supported client-side flag is + * VIR_CONNECT_NO_ALIASES *
I'd rather have here the regular description ("bitwise or of virConnectFlags ...") and document that the only supported flag so far is _NO_ALIASES. Thing is, I'd rather avoid enumerating all supported flags here as the list might be extensive over time.
* Opens connection to admin interface of the daemon. * @@ -204,7 +205,6 @@ virAdmConnectOpen(const char *name, unsigned int flags)
VIR_DEBUG("flags=%x", flags); virResetLastError(); - virCheckFlagsGoto(VIR_CONNECT_NO_ALIASES, error);
if (!(conn = virAdmConnectNew())) goto error;
ACK with those nits fixed. Michal
On 10/05/16 15:17, Michal Privoznik wrote:
On 05.05.2016 13:49, Erik Skultety wrote:
Unlike the previous commit, we do actually support one client-side only flag VIR_CONNECT_NO_ALIASES, so besides removing the check for flags this flag has to be masked out before sending a message to the daemon, otherwise it would trigger an error when checking flags on the daemon side.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/admin/admin_remote.c | 2 +- src/libvirt-admin.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 40fcddb..180dd3b 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -162,7 +162,7 @@ remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags)
virObjectLock(priv);
- args.flags = flags; + args.flags = flags ^ VIR_CONNECT_NO_ALIASES;
As Peter already pointed out, this should be flags & ~VIR_CONNECT_NO_ALIASES;
if (virNetClientRegisterAsyncIO(priv->client) < 0) { VIR_DEBUG("Failed to add event watch, disabling events and support for" diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index d7a66ff..91f40c8 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -185,7 +185,8 @@ virAdmGetDefaultURI(virConfPtr conf) /** * virAdmConnectOpen: * @name: uri of the daemon to connect to, NULL for default - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: extra flags; currently the only supported client-side flag is + * VIR_CONNECT_NO_ALIASES *
I'd rather have here the regular description ("bitwise or of virConnectFlags ...") and document that the only supported flag so far is _NO_ALIASES. Thing is, I'd rather avoid enumerating all supported flags here as the list might be extensive over time.
* Opens connection to admin interface of the daemon. * @@ -204,7 +205,6 @@ virAdmConnectOpen(const char *name, unsigned int flags)
VIR_DEBUG("flags=%x", flags); virResetLastError(); - virCheckFlagsGoto(VIR_CONNECT_NO_ALIASES, error);
if (!(conn = virAdmConnectNew())) goto error;
ACK with those nits fixed.
Michal
I fixed the bitwise operation, adjusted the comment as you pointed out and pushed, thanks. Erik
participants (3)
-
Erik Skultety -
Michal Privoznik -
Peter Krempa