[libvirt] [RFC PATCH 0/4] Add new macro to check for existent server
Hello, I am trying to add a generic way to check for server, specially in drivers that needs the server to be declared. If there is better way to put the check, or a better way to check this setting, let me know. Thanks in advance! Marcos Paulo de Souza (4): driver.h: Add macro VIR_DRV_CONN_CHECK_SERVER esx_driver: Use VIR_DRV_CONN_CHECK_SERVER macro hyperv_driver: Use VIR_DRV_CONN_CHECK_SERVER macro phyp_driver: Use VIR_DRV_CONN_CHECK_SERVER macro src/driver.h | 9 +++++++++ src/esx/esx_driver.c | 7 +------ src/hyperv/hyperv_driver.c | 7 +------ src/phyp/phyp_driver.c | 6 +----- 4 files changed, 12 insertions(+), 17 deletions(-) -- 2.17.1
This new macro will check if the server was passed to connectOpen function. Some drivers as esx, hyperv and phyp need a server address to connect to. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/driver.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/driver.h b/src/driver.h index 0b1f7a2269..1f9d7fd26b 100644 --- a/src/driver.h +++ b/src/driver.h @@ -60,6 +60,15 @@ typedef enum { ((drv)->connectSupportsFeature ? \ (drv)->connectSupportsFeature((conn), (feature)) > 0 : 0) +/* Check if the connection of the driver has filled the server address */ +# define VIR_DRV_CONN_CHECK_SERVER \ + do { \ + if (!conn->uri->server) { \ + virReportError(VIR_ERR_INVALID_ARG, "%s", \ + _("URI is missing the server part")); \ + return VIR_DRV_OPEN_ERROR; \ + } \ + } while (0) # define __VIR_DRIVER_H_INCLUDES___ -- 2.17.1
On Sat, Jul 07, 2018 at 22:06:53 -0300, Marcos Paulo de Souza wrote:
This new macro will check if the server was passed to connectOpen function. Some drivers as esx, hyperv and phyp need a server address to connect to.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/driver.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/driver.h b/src/driver.h index 0b1f7a2269..1f9d7fd26b 100644 --- a/src/driver.h +++ b/src/driver.h @@ -60,6 +60,15 @@ typedef enum { ((drv)->connectSupportsFeature ? \ (drv)->connectSupportsFeature((conn), (feature)) > 0 : 0)
+/* Check if the connection of the driver has filled the server address */ +# define VIR_DRV_CONN_CHECK_SERVER \
'conn' should be passed in as an argument. Without that it is less obvious what is happening here. P.S: Yes I'm aware that we have e.g. virCheckFlags that checks the 'flags' variable in any context, but we don't have to introduce more of it.
On Mon, Jul 09, 2018 at 08:31:05AM +0200, Peter Krempa wrote:
On Sat, Jul 07, 2018 at 22:06:53 -0300, Marcos Paulo de Souza wrote:
This new macro will check if the server was passed to connectOpen function. Some drivers as esx, hyperv and phyp need a server address to connect to.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/driver.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/driver.h b/src/driver.h index 0b1f7a2269..1f9d7fd26b 100644 --- a/src/driver.h +++ b/src/driver.h @@ -60,6 +60,15 @@ typedef enum { ((drv)->connectSupportsFeature ? \ (drv)->connectSupportsFeature((conn), (feature)) > 0 : 0)
+/* Check if the connection of the driver has filled the server address */ +# define VIR_DRV_CONN_CHECK_SERVER \
'conn' should be passed in as an argument. Without that it is less obvious what is happening here.
P.S: Yes I'm aware that we have e.g. virCheckFlags that checks the 'flags' variable in any context, but we don't have to introduce more of it.
How about to follow Matthias idea, and add a new member to virConnectDriver? (https://www.redhat.com/archives/libvir-list/2018-July/msg00400.html) I am already preparing a new patchset following his idea. -- Thanks, Marcos
ESX needs a server to connect to, so use this macro in order to reuse code. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_driver.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 06e1238385..89ac7c430f 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -854,12 +854,7 @@ esxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, conn->uri->path, conn->uri->scheme); } - /* Require server part */ - if (!conn->uri->server) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("URI is missing the server part")); - return VIR_DRV_OPEN_ERROR; - } + VIR_DRV_CONN_CHECK_SERVER; /* Require auth */ if (!auth || !auth->cb) { -- 2.17.1
hyperv needs a server to connect to, so use this macro in order to reuse code. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/hyperv/hyperv_driver.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 95496bdf73..a419f0ccee 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -128,12 +128,7 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); - /* Require server part */ - if (conn->uri->server == NULL) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("URI is missing the server part")); - return VIR_DRV_OPEN_ERROR; - } + VIR_DRV_CONN_CHECK_SERVER; /* Require auth */ if (auth == NULL || auth->cb == NULL) { -- 2.17.1
phyp needs a server to connect to, so use this macro in order to reuse code. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/phyp/phyp_driver.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 67ce7903ba..beca1c6fc7 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1141,11 +1141,7 @@ phypConnectOpen(virConnectPtr conn, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); - if (conn->uri->server == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing server name in phyp:// URI")); - return VIR_DRV_OPEN_ERROR; - } + VIR_DRV_CONN_CHECK_SERVER; if (VIR_ALLOC(phyp_driver) < 0) goto failure; -- 2.17.1
2018-07-08 3:06 GMT+02:00 Marcos Paulo de Souza <marcos.souza.org@gmail.com>:
Hello,
I am trying to add a generic way to check for server, specially in drivers that needs the server to be declared. If there is better way to put the check, or a better way to check this setting, let me know.
Thanks in advance!
I'd realize this in a different way. The virConnectDriver struct has a localOnly flag that is used to indicate whether a connection driver permits a server in the URI. Then virConnectOpenInternal uses this flag during its driver auto-probing phase. I suggest expanding this concept and adding a remoteOnly flag to the virConnectDriver struct. Instead of checking the URI server part in the driver's own open function let virConnectOpenInternal do this check based on the remoteOnly flag. -- Matthias Bolte http://photron.blogspot.com
participants (3)
-
Marcos Paulo de Souza -
Matthias Bolte -
Peter Krempa