diff mbox series

[v4,4/5] telemetry: implement empty stubs for Windows

Message ID 20200804062947.6176-5-fady@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series compiling ethdev lib under windows |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Fady Bader Aug. 4, 2020, 6:29 a.m. UTC 
  Telemetry didn't compile under Windows.
Empty stubs implementation was added for Windows.

Signed-off-by: Fady Bader <fady@mellanox.com>
---
 lib/librte_telemetry/rte_telemetry.h    |  4 +++
 lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
 lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
 3 files changed, 79 insertions(+), 2 deletions(-)

Comments

Narcisa Ana Maria Vasile Aug. 4, 2020, 6:47 p.m. UTC | #1 
On Tue, Aug 04, 2020 at 09:29:46AM +0300, Fady Bader wrote:
> Telemetry didn't compile under Windows.
> Empty stubs implementation was added for Windows.
> 
> Signed-off-by: Fady Bader <fady@mellanox.com>
> ---
>  lib/librte_telemetry/rte_telemetry.h    |  4 +++
>  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
>  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
>  3 files changed, 79 insertions(+), 2 deletions(-)
> 
Acked-by: Narcisa Vasile <navasile@linux.microsoft.com>
Dmitry Kozlyuk Aug. 4, 2020, 11:39 p.m. UTC | #2 
On Tue,  4 Aug 2020 09:29:46 +0300, Fady Bader wrote:
> Telemetry didn't compile under Windows.
> Empty stubs implementation was added for Windows.
> 
> Signed-off-by: Fady Bader <fady@mellanox.com>
> ---
>  lib/librte_telemetry/rte_telemetry.h    |  4 +++
>  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
>  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
>  3 files changed, 79 insertions(+), 2 deletions(-)

You could #ifdef code in librte_ethdev that uses librte_telemetry and not
build librte_telemetry at all. This approach is already taken in
eal_common_options.c and it avoids spreading #ifdef throughout telemetry code.
Thomas Monjalon Aug. 5, 2020, 8:27 a.m. UTC | #3 
05/08/2020 01:39, Dmitry Kozlyuk:
> On Tue,  4 Aug 2020 09:29:46 +0300, Fady Bader wrote:
> > Telemetry didn't compile under Windows.
> > Empty stubs implementation was added for Windows.
> > 
> > Signed-off-by: Fady Bader <fady@mellanox.com>
> > ---
> >  lib/librte_telemetry/rte_telemetry.h    |  4 +++
> >  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
> >  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
> >  3 files changed, 79 insertions(+), 2 deletions(-)
> 
> You could #ifdef code in librte_ethdev that uses librte_telemetry and not
> build librte_telemetry at all. This approach is already taken in
> eal_common_options.c and it avoids spreading #ifdef throughout telemetry code.

The problem is that telemetry can be used anywhere, not only in ethdev.
I feel it is better to #ifdef telemetry than every telemetry calls.
Thomas Monjalon Aug. 5, 2020, 8:28 a.m. UTC | #4 
04/08/2020 08:29, Fady Bader:
> Telemetry didn't compile under Windows.
> Empty stubs implementation was added for Windows.
> 
> Signed-off-by: Fady Bader <fady@mellanox.com>
> ---
>  lib/librte_telemetry/rte_telemetry.h    |  4 +++
>  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
>  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
>  3 files changed, 79 insertions(+), 2 deletions(-)

The removed lines are blank lines which should remain.
Bruce Richardson Aug. 5, 2020, 8:40 a.m. UTC | #5 
On Wed, Aug 05, 2020 at 10:27:27AM +0200, Thomas Monjalon wrote:
> 05/08/2020 01:39, Dmitry Kozlyuk:
> > On Tue,  4 Aug 2020 09:29:46 +0300, Fady Bader wrote:
> > > Telemetry didn't compile under Windows.
> > > Empty stubs implementation was added for Windows.
> > > 
> > > Signed-off-by: Fady Bader <fady@mellanox.com>
> > > ---
> > >  lib/librte_telemetry/rte_telemetry.h    |  4 +++
> > >  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
> > >  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
> > >  3 files changed, 79 insertions(+), 2 deletions(-)
> > 
> > You could #ifdef code in librte_ethdev that uses librte_telemetry and not
> > build librte_telemetry at all. This approach is already taken in
> > eal_common_options.c and it avoids spreading #ifdef throughout telemetry code.
> 
> The problem is that telemetry can be used anywhere, not only in ethdev.
> I feel it is better to #ifdef telemetry than every telemetry calls.
> 
Given that the majority of telemetry has no external dependencies (jansson
is only used for the older compatibility part), and it uses sockets for
communication, is there a reason why it can't just be made to build and
work on windows? The unix domain socket could be converted to a standard
UDP socket on localhost, perhaps. Is there anything unix-specific beyond
that?

/Bruce
Thomas Monjalon Aug. 5, 2020, 9:06 a.m. UTC | #6 
05/08/2020 10:40, Bruce Richardson:
> On Wed, Aug 05, 2020 at 10:27:27AM +0200, Thomas Monjalon wrote:
> > 05/08/2020 01:39, Dmitry Kozlyuk:
> > > On Tue,  4 Aug 2020 09:29:46 +0300, Fady Bader wrote:
> > > > Telemetry didn't compile under Windows.
> > > > Empty stubs implementation was added for Windows.
> > > > 
> > > > Signed-off-by: Fady Bader <fady@mellanox.com>
> > > > ---
> > > >  lib/librte_telemetry/rte_telemetry.h    |  4 +++
> > > >  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
> > > >  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
> > > >  3 files changed, 79 insertions(+), 2 deletions(-)
> > > 
> > > You could #ifdef code in librte_ethdev that uses librte_telemetry and not
> > > build librte_telemetry at all. This approach is already taken in
> > > eal_common_options.c and it avoids spreading #ifdef throughout telemetry code.
> > 
> > The problem is that telemetry can be used anywhere, not only in ethdev.
> > I feel it is better to #ifdef telemetry than every telemetry calls.
> > 
> Given that the majority of telemetry has no external dependencies (jansson
> is only used for the older compatibility part), and it uses sockets for
> communication, is there a reason why it can't just be made to build and
> work on windows? The unix domain socket could be converted to a standard
> UDP socket on localhost, perhaps. Is there anything unix-specific beyond
> that?

Yes of course telemetry should build on Windows.
But it seems nobody committed yet to work on it.
Feel free :)
Bruce Richardson Aug. 5, 2020, 10:02 a.m. UTC | #7 
On Wed, Aug 05, 2020 at 11:06:04AM +0200, Thomas Monjalon wrote:
> 05/08/2020 10:40, Bruce Richardson:
> > On Wed, Aug 05, 2020 at 10:27:27AM +0200, Thomas Monjalon wrote:
> > > 05/08/2020 01:39, Dmitry Kozlyuk:
> > > > On Tue,  4 Aug 2020 09:29:46 +0300, Fady Bader wrote:
> > > > > Telemetry didn't compile under Windows.
> > > > > Empty stubs implementation was added for Windows.
> > > > > 
> > > > > Signed-off-by: Fady Bader <fady@mellanox.com>
> > > > > ---
> > > > >  lib/librte_telemetry/rte_telemetry.h    |  4 +++
> > > > >  lib/librte_telemetry/telemetry.c        | 51 ++++++++++++++++++++++++++++++++-
> > > > >  lib/librte_telemetry/telemetry_legacy.c | 26 ++++++++++++++++-
> > > > >  3 files changed, 79 insertions(+), 2 deletions(-)
> > > > 
> > > > You could #ifdef code in librte_ethdev that uses librte_telemetry and not
> > > > build librte_telemetry at all. This approach is already taken in
> > > > eal_common_options.c and it avoids spreading #ifdef throughout telemetry code.
> > > 
> > > The problem is that telemetry can be used anywhere, not only in ethdev.
> > > I feel it is better to #ifdef telemetry than every telemetry calls.
> > > 
> > Given that the majority of telemetry has no external dependencies (jansson
> > is only used for the older compatibility part), and it uses sockets for
> > communication, is there a reason why it can't just be made to build and
> > work on windows? The unix domain socket could be converted to a standard
> > UDP socket on localhost, perhaps. Is there anything unix-specific beyond
> > that?
> 
> Yes of course telemetry should build on Windows.
> But it seems nobody committed yet to work on it.
> Feel free :)

Thanks, I might give it a go sometime if I have the chance. However, I was
mainly just checking that there was no known serious impediment to having
it work.
diff mbox series

Patch

diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
index d13010b8fb..c3c0e44599 100644
--- a/lib/librte_telemetry/rte_telemetry.h
+++ b/lib/librte_telemetry/rte_telemetry.h
@@ -5,6 +5,10 @@ 
 #include <stdint.h>
 #include <rte_compat.h>
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#include <sched.h>
+#endif
+
 #ifndef _RTE_TELEMETRY_H_
 #define _RTE_TELEMETRY_H_
 
diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c
index 0252282735..d794e75e5e 100644
--- a/lib/librte_telemetry/telemetry.c
+++ b/lib/librte_telemetry/telemetry.c
@@ -1,7 +1,7 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2020 Intel Corporation
  */
-
+#ifndef RTE_EXEC_ENV_WINDOWS
 #include <unistd.h>
 #include <pthread.h>
 #include <sys/socket.h>
@@ -20,6 +20,20 @@ 
 #include "telemetry_data.h"
 #include "rte_telemetry_legacy.h"
 
+#else /* RTE_EXEC_ENV_WINDOWS */
+
+/* includes for Windows */
+#include <rte_common.h>
+#include <rte_log.h>
+
+#include "rte_telemetry.h"
+#include "telemetry_json.h"
+#include "telemetry_data.h"
+#include "rte_telemetry_legacy.h"
+
+#endif
+
+#ifndef RTE_EXEC_ENV_WINDOWS
 #define MAX_CMD_LEN 56
 #define MAX_HELP_LEN 64
 #define MAX_OUTPUT_LEN (1024 * 16)
@@ -42,17 +56,24 @@  struct socket {
 };
 static struct socket v2_socket; /* socket for v2 telemetry */
 static struct socket v1_socket; /* socket for v1 telemetry */
+#endif
+
 static char telemetry_log_error[1024]; /* Will contain error on init failure */
+
+#ifndef RTE_EXEC_ENV_WINDOWS
 /* list of command callbacks, with one command registered by default */
 static struct cmd_callback callbacks[TELEMETRY_MAX_CALLBACKS];
 static int num_callbacks; /* How many commands are registered */
 /* Used when accessing or modifying list of command callbacks */
 static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER;
 static uint16_t v2_clients;
+#endif
 
 int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 {
+#ifndef RTE_EXEC_ENV_WINDOWS
+
 	int i = 0;
 
 	if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
@@ -76,8 +97,19 @@  rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 	rte_spinlock_unlock(&callback_sl);
 
 	return 0;
+
+#else /* RTE_EXEC_ENV_WINDOWS */
+
+	RTE_SET_USED(cmd);
+	RTE_SET_USED(fn);
+	RTE_SET_USED(help);
+
+	return 0;
+
+#endif
 }
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 static int
 list_commands(const char *cmd __rte_unused, const char *params __rte_unused,
 		struct rte_tel_data *d)
@@ -411,11 +443,14 @@  telemetry_v2_init(const char *runtime_dir, rte_cpuset_t *cpuset)
 
 	return 0;
 }
+#endif
 
 int32_t
 rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
 		const char **err_str)
 {
+#ifndef RTE_EXEC_ENV_WINDOWS
+
 	if (telemetry_v2_init(runtime_dir, cpuset) != 0) {
 		*err_str = telemetry_log_error;
 		return -1;
@@ -424,4 +459,18 @@  rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
 		*err_str = telemetry_log_error;
 	}
 	return 0;
+
+#else /* RTE_EXEC_ENV_WINDOWS */
+
+	RTE_SET_USED(runtime_dir);
+	RTE_SET_USED(cpuset);
+	RTE_SET_USED(err_str);
+
+	snprintf(telemetry_log_error, sizeof(telemetry_log_error),
+		"Telemetry is not supported on Windows.");
+
+	return 0;
+
+#endif
 }
+
diff --git a/lib/librte_telemetry/telemetry_legacy.c b/lib/librte_telemetry/telemetry_legacy.c
index a341fe4ebd..ed2c1cc428 100644
--- a/lib/librte_telemetry/telemetry_legacy.c
+++ b/lib/librte_telemetry/telemetry_legacy.c
@@ -1,7 +1,7 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2020 Intel Corporation
  */
-
+#ifndef RTE_EXEC_ENV_WINDOWS
 #include <unistd.h>
 #include <sys/socket.h>
 #include <sys/un.h>
@@ -15,6 +15,15 @@ 
 
 #include "rte_telemetry_legacy.h"
 
+#else /* RTE_EXEC_ENV_WINDOWS */
+
+#include <rte_common.h>
+
+#include "rte_telemetry_legacy.h"
+
+#endif
+#ifndef RTE_EXEC_ENV_WINDOWS
+
 #define MAX_LEN 128
 #define BUF_SIZE 1024
 #define CLIENTS_UNREG_ACTION "\"action\":2"
@@ -48,12 +57,15 @@  struct json_command callbacks[TELEMETRY_LEGACY_MAX_CALLBACKS] = {
 };
 int num_legacy_callbacks = 1;
 static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER;
+#endif
 
 int
 rte_telemetry_legacy_register(const char *cmd,
 		enum rte_telemetry_legacy_data_req data_req,
 		telemetry_legacy_cb fn)
 {
+#ifndef RTE_EXEC_ENV_WINDOWS
+
 	if (fn == NULL)
 		return -EINVAL;
 	if (num_legacy_callbacks >= (int) RTE_DIM(callbacks))
@@ -71,8 +83,19 @@  rte_telemetry_legacy_register(const char *cmd,
 	rte_spinlock_unlock(&callback_sl);
 
 	return 0;
+
+#else /* RTE_EXEC_ENV_WINDOWS */
+
+	RTE_SET_USED(cmd);
+	RTE_SET_USED(data_req);
+	RTE_SET_USED(fn);
+
+	return 0;
+
+#endif
 }
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 static int
 register_client(const char *cmd __rte_unused, const char *params,
 		char *buffer __rte_unused, int buf_len __rte_unused)
@@ -239,3 +262,4 @@  legacy_client_handler(void *sock_id)
 	close(s);
 	return NULL;
 }
+#endif