[Libstoragemgmt-devel] [PATCH v2 0/9] Various small cleanups to the C-based code/Makefiles

[Christophe Fergeau]

Hey,

Here is the 2nd version of this series. The only changes are in patch 6/9 to
drop the fallback implementation of json_object_object_get_ex as discussed on
the mailing list.
I've also changed 9/9 as I missed some functions which I should have marked as
static.
The rest of the paches are unchanged.

Christophe

[Christophe Fergeau]

This causes a warning on fedora 21 as this has been deprecated.
This commit switches to the recommended json_object_object_get_ex
replacement.
This function has been available since at least json-c 0.10 which is
the minimum version we require, so we don't need a fallback
version of that function.
---
daemon/lsm_rest.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/daemon/lsm_rest.c b/daemon/lsm_rest.c
index 1f38235..95d1a7c 100644
--- a/daemon/lsm_rest.c
+++ b/daemon/lsm_rest.c
@@ -309,9 +309,13 @@ const char *rpc(int socket_fd, const char *method, ParaList_t *para_list,
return NULL;
}
json_object *recv_json = json_tokener_parse(recv_json_string);
- const char *result_str = json_object_to_json_string_ext(
- json_object_object_get(recv_json, "result"),
- JSON_C_TO_STRING_PRETTY);
+ json_object *result_json;
+ if (!json_object_object_get_ex(recv_json, "result", &result_json)){
+ printf("No 'result' node in received JSON data");
+ return NULL;
+ }
+ const char *result_str = json_object_to_json_string_ext(result_json,
+ JSON_C_TO_STRING_PRETTY);
return result_str;
}

[Christophe Fergeau]

The compilation instructions are no longer needed as the rest daemon
build is integrated in the main autotools-based build system.
---
daemon/lsm_rest.c | 5 -----
1 file changed, 5 deletions(-)

diff --git a/daemon/lsm_rest.c b/daemon/lsm_rest.c
index 95d1a7c..16fdbe6 100644
--- a/daemon/lsm_rest.c
+++ b/daemon/lsm_rest.c
@@ -41,11 +41,6 @@
TODO: MHD_get_connection_values() with MHD_GET_ARGUMENT_KIND to
get all query argument
TODO: Check malloc() return code
-
- gcc lsm_rest.c -o lsm_restd \
- `pkg-config --cflags --libs libmicrohttpd xml2` \
- `xml2-config --libs` `xml2-config --cflags` \
- `pkd-config --cflags --libs json`
*/

void para_list_init(ParaList_t *para_list)

[Christophe Fergeau]

DEPS is not the name of an automake variable (closest name would be
DEPENDENCIES, see
https://www.gnu.org/software/automake/manual/html_node/Variable-Index.html#Variable-Index
). Moreover nothing uses DEPS in the Makefile.am or in other build
system files so it can be removed.
---
c_binding/Makefile.am | 2 --
1 file changed, 2 deletions(-)

diff --git a/c_binding/Makefile.am b/c_binding/Makefile.am
index 54a2d58..e4b8e1b 100644
--- a/c_binding/Makefile.am
+++ b/c_binding/Makefile.am
@@ -13,5 +13,3 @@ libstoragemgmt_la_SOURCES= \
lsm_mgmt.cpp lsm_datatypes.hpp lsm_datatypes.cpp lsm_convert.hpp \
lsm_convert.cpp lsm_ipc.hpp lsm_ipc.cpp lsm_plugin_ipc.hpp \
lsm_plugin_ipc.cpp util/qparams.c util/qparams.h
-
-DEPS = $(top_builddir)/c_binding/libstoragemgmt.la

[Christophe Fergeau]

The daemon does not need libstoragemgmt, so we don't need to reference
libstoragemgmt.la or its CFLAGS in daemon/Makefile.am
---
daemon/Makefile.am | 5 -----
1 file changed, 5 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index de45612..2855a49 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -1,14 +1,9 @@
AM_CPPFLAGS = \
- -I$(top_srcdir)/c_binding/include \
- -I$(top_builddir)/c_binding/include \
- -***@srcdir@/c_binding/include \
$(LIBXML_CFLAGS) $(LIBGLIB_CFLAGS) \
$(JSON_CFLAGS) $(LIBMICROHTTPD_CFLAGS)

bin_PROGRAMS = lsmd

-DEPS = $(top_builddir)/c_binding/libstoragemgmt.la
-
EXTRA_DIST=lsm_rest.c lsm_rest.h

lsmd_LDFLAGS=-Wl,-z,relro,-z,now -pie

[Christophe Fergeau]

The MHD_PLATFORM_H #define needs to be set before including
microhttpd.h, so it had no effect where it was placed.
This #define is useful when building on systems where the standard
headers defining uint64_t, size_t, ... are not available.
As we are building on reasonably standard platforms, we can rely let
microhttpd.h use the standard headers.
---
daemon/lsm_rest.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/daemon/lsm_rest.c b/daemon/lsm_rest.c
index 16fdbe6..126798d 100644
--- a/daemon/lsm_rest.c
+++ b/daemon/lsm_rest.c
@@ -35,9 +35,7 @@

#include "lsm_rest.h"

-#define MHD_PLATFORM_H
-/* prevent 'microhttpd.h' to include other platform headers
-
+/*
TODO: MHD_get_connection_values() with MHD_GET_ARGUMENT_KIND to
get all query argument
TODO: Check malloc() return code

[Christophe Fergeau]

Currently, CPPFLAGS are set globally for both lsmd and lsm_restd through
the use of AM_CPPFLAGS. However, lsmd does not need any such flags, they
are only needed by lsm_restd. This commit moves the global AM_CPPFLAGS
to lsm_restd_CFLAGS. It also removes LIBGLIB_CFLAGS as glib is not used
here.
---
daemon/Makefile.am | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 2855a49..521055f 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -1,7 +1,3 @@
-AM_CPPFLAGS = \
- $(LIBXML_CFLAGS) $(LIBGLIB_CFLAGS) \
- $(JSON_CFLAGS) $(LIBMICROHTTPD_CFLAGS)
-
bin_PROGRAMS = lsmd

EXTRA_DIST=lsm_rest.c lsm_rest.h
@@ -14,6 +10,6 @@ lsmd_SOURCES = lsm_daemon.c
if WITH_REST_API
bin_PROGRAMS += lsm_restd
lsm_restd_LDFLAGS=$(LIBMICROHTTPD_LIBS) $(JSON_LIBS) $(LIBXML_LIBS)
-lsm_restd_CFLAGS=-fPIE -DPIE
+lsm_restd_CFLAGS=-fPIE -DPIE $(LIBMICROHTTPD_CFLAGS) $(JSON_CFLAGS) $(LIBXML_CFLAGS)
lsm_restd_SOURCES= lsm_rest.c
endif

[Christophe Fergeau]

"fils system" -> "file system"
---
c_binding/include/libstoragemgmt/libstoragemgmt.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt.h b/c_binding/include/libstoragemgmt/libstoragemgmt.h
index 85b8e4e..b421424 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt.h
@@ -595,7 +595,7 @@ extern "C" {
uint32_t *fs_count, lsm_flag flags);

/**
- * Creates a new fils system from the specified pool
+ * Creates a new file system from the specified pool
* @param[in] conn Valid connection
* @param[in] pool Valid pool
* @param[in] name File system name

[Christophe Fergeau]

c_binding/Makefile.am references some files from the util/ subdirectory.
This causes warning with newer automakes unless 'subdir-objects' is
used:

c_binding/Makefile.am:12: warning: source file 'util/qparams.c' is in a subdirectory,
c_binding/Makefile.am:12: but option 'subdir-objects' is disabled
automake: warning: possible forward-incompatibility.
automake: At least a source file is in a subdirectory, but the 'subdir-objects'
automake: automake option hasn't been enabled. For now, the corresponding output
automake: object file(s) will be placed in the top-level directory. However,
automake: this behaviour will change in future Automake versions: they will
automake: unconditionally cause object files to be placed in the same subdirectory
automake: of the corresponding sources.
automake: You are advised to start using 'subdir-objects' option throughout your
automake: project, to avoid future incompatibilities.
---
configure.ac | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index c4692e0..a96cedb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -8,7 +8,7 @@ AC_CONFIG_AUX_DIR([build-aux])
AC_CONFIG_HEADERS([config.h])
AC_CONFIG_MACRO_DIR([m4])
dnl Make automake keep quiet about wildcards & other GNUmake-isms
-AM_INIT_AUTOMAKE([-Wno-portability])
+AM_INIT_AUTOMAKE([-Wno-portability subdir-objects])
AM_MAINTAINER_MODE([enable])
# Enable silent build when available (Automake 1.11)
m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])])

[Christophe Fergeau]

---
daemon/lsm_rest.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/daemon/lsm_rest.c b/daemon/lsm_rest.c
index 126798d..7f26e9e 100644
--- a/daemon/lsm_rest.c
+++ b/daemon/lsm_rest.c
@@ -130,7 +130,7 @@ json_object *para_list_to_json(ParaList_t *para_list)
return jobj;
}

-int connect_socket(const char *uri_str, const char *plugin_dir,
+static int connect_socket(const char *uri_str, const char *plugin_dir,
int *error_no)
{
int socket_fd = -1;
@@ -178,7 +178,7 @@ int connect_socket(const char *uri_str, const char *plugin_dir,
}


-int send_msg(int socket_fd, const char *msg, int *error_no)
+static int send_msg(int socket_fd, const char *msg, int *error_no)
{
int rc = -1;
size_t len = strlen(msg);
@@ -204,7 +204,7 @@ int send_msg(int socket_fd, const char *msg, int *error_no)
return rc;
}

-const char *_recv_msg(int socket_fd, size_t count, int *error_no)
+static const char *_recv_msg(int socket_fd, size_t count, int *error_no)
{
char buff[LSM_SOCK_BUFF_LEN];
size_t amount_read = 0;
@@ -238,7 +238,7 @@ const char *_recv_msg(int socket_fd, size_t count, int *error_no)
}
}

-const char *recv_msg(int socket_fd, int *error_no)
+static const char *recv_msg(int socket_fd, int *error_no)
{
*error_no = 0;
const char *msg_len_str = _recv_msg(socket_fd, LSM_HEADER_LEN,
@@ -269,7 +269,7 @@ const char *recv_msg(int socket_fd, int *error_no)
return msg;
}

-const char *rpc(int socket_fd, const char *method, ParaList_t *para_list,
+static const char *rpc(int socket_fd, const char *method, ParaList_t *para_list,
int *error_no)
{
*error_no = 0;
@@ -312,7 +312,7 @@ const char *rpc(int socket_fd, const char *method, ParaList_t *para_list,
return result_str;
}

-int plugin_startup(int socket_fd, const char *uri, const char *pass, int tmo)
+static int plugin_startup(int socket_fd, const char *uri, const char *pass, int tmo)
{
printf("Starting the plugin\n");
int error_no = 0;
@@ -329,7 +329,7 @@ int plugin_startup(int socket_fd, const char *uri, const char *pass, int tmo)
return error_no;
}

-int plugin_shutdown(int socket_fd)
+static int plugin_shutdown(int socket_fd)
{
printf("Shutting down the plugin\n");
int error_no = 0;
@@ -341,7 +341,7 @@ int plugin_shutdown(int socket_fd)
return error_no;
}

-const char *v01_query(int socket_fd, const char* method, ParaList_t *para_list,
+static const char *v01_query(int socket_fd, const char* method, ParaList_t *para_list,
int *error_no)
{
*error_no = 0;
@@ -354,7 +354,7 @@ const char *v01_query(int socket_fd, const char* method, ParaList_t *para_list,
return rpc(socket_fd, method, para_list, error_no);
}

-const char *lsm_api_0_1(struct MHD_Connection *connection,
+static const char *lsm_api_0_1(struct MHD_Connection *connection,
const char *uri, const char * pass,
const char *url, const char *method,
const char *upload_data)
@@ -409,7 +409,7 @@ const char *lsm_api_0_1(struct MHD_Connection *connection,
return json_msg;
}

-int answer_to_connection(void *cls, struct MHD_Connection *connection,
+static int answer_to_connection(void *cls, struct MHD_Connection *connection,
const char *url,
const char *method, const char *version,
const char *upload_data,

[Gris Ge]

Post by Christophe Fergeau
Hey,
Here is the 2nd version of this series. The only changes are in patch 6/9 to
drop the fallback implementation of json_object_object_get_ex as discussed on
the mailing list.
I've also changed 9/9 as I missed some functions which I should have marked as
static.
The rest of the paches are unchanged.
Christophe

Hi Christophe,

Looks good to me.
Compile pass in OBS for RHEL6/7, Fedora 19/20.

Tested via commands:

lsm_restd
curl http://127.0.0.1:8888/v0.1/volumes?uri=sim:
curl http://127.0.0.1:8888/v0.1/pools?uri=sim:

One tiny thing, it could be better if you could wrap the line with
78 text width in patch 6/9.

Thank you very much for these patches.
Best regard.

[Christophe Fergeau]

This causes a warning on fedora 21 as this has been deprecated.
This commit switches to the recommended json_object_object_get_ex
replacement.
This function has been available since at least json-c 0.10 which is
the minimum version we require, so we don't need a fallback
version of that function.
---
I've split the "JSON_C_TO_STRING_PRETTY" line to end at column 77 instead of
column 85 previously, is this the one you wanted shorter?

Christophe


daemon/lsm_rest.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/daemon/lsm_rest.c b/daemon/lsm_rest.c
index 1f38235..95cc075 100644
--- a/daemon/lsm_rest.c
+++ b/daemon/lsm_rest.c
@@ -309,9 +309,14 @@ const char *rpc(int socket_fd, const char *method, ParaList_t *para_list,
return NULL;
}
json_object *recv_json = json_tokener_parse(recv_json_string);
- const char *result_str = json_object_to_json_string_ext(
- json_object_object_get(recv_json, "result"),
- JSON_C_TO_STRING_PRETTY);
+ json_object *result_json;
+ if (!json_object_object_get_ex(recv_json, "result", &result_json)){
+ printf("No 'result' node in received JSON data");
+ return NULL;
+ }
+ const char *result_str;
+ result_str = json_object_to_json_string_ext(result_json,
+ JSON_C_TO_STRING_PRETTY);
return result_str;
}

[Tony Asleson]

Patch series committed!

Thanks,
Tony

Post by Christophe Fergeau
Hey,
Here is the 2nd version of this series. The only changes are in patch 6/9 to
drop the fallback implementation of json_object_object_get_ex as discussed on
the mailing list.
I've also changed 9/9 as I missed some functions which I should have marked as
static.
The rest of the paches are unchanged.
Christophe
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://p.sf.net/sfu/Zoho
_______________________________________________
Libstoragemgmt-devel mailing list
https://lists.sourceforge.net/lists/listinfo/libstoragemgmt-devel

------------------------------------------------------------------------------