1:49 p.m.
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
1:49 p.m.
New subject: [libvirt] [PATCH 1/2] admin: Remove flags checking from public API entry points
Due to compatibility reasons these should be checked on the server side.
Signed-off-by: Erik Skultety <eskultet(a)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
3:17 p.m.
New subject: [libvirt] [PATCH 1/2] admin: Remove flags checking from public API entry points
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(a)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
1:49 p.m.
New subject: [libvirt] [PATCH 2/2] admin: Remove flags checking from virAdmConnectOpen public API
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(a)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
2:13 p.m.
New subject: [libvirt] [PATCH 2/2] admin: Remove flags checking from virAdmConnectOpen public API
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(a)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.
2:39 p.m.
New subject: [libvirt] [PATCH 2/2] admin: Remove flags checking from virAdmConnectOpen public API
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(a)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
3:17 p.m.
New subject: [libvirt] [PATCH 2/2] admin: Remove flags checking from virAdmConnectOpen public API
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(a)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
4:03 p.m.
New subject: [libvirt] [PATCH 2/2] admin: Remove flags checking from virAdmConnectOpen public API
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(a)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
3055
days inactive
3060
days old
7 comments
3 participants
participants (3)
-
Erik Skultety -
Michal Privoznik -
Peter Krempa