| Message ID | 20171212144744.24224-2-christian.storm@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
Hi Christian, On 12/12/2017 15:47, Christian Storm wrote: > The functions for opening, closing, receiving, and sending > data over the channel should be exposed for testability as > described in the header comment. > > This reverts commit f531694 as it breaks suricatta-tests. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > include/channel_curl.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/channel_curl.h b/include/channel_curl.h > index 98240a9..94d3c55 100644 > --- a/include/channel_curl.h > +++ b/include/channel_curl.h > @@ -21,6 +21,7 @@ > #include <json-c/json.h> > #include <stdio.h> > #include "sslapi.h" > +#include <channel.h> > > /* hawkBit Channel Implementation Private Header File. > * > @@ -59,3 +60,10 @@ typedef struct { > struct swupdate_digest *dgst; > char sha1hash[SHA_DIGEST_LENGTH * 2 + 1]; > } channel_data_t; > + > +channel_op_res_t channel_close(channel_t *this); > +channel_op_res_t channel_open(channel_t *this, void *cfg); > +channel_op_res_t channel_put(channel_t *this, void *data); > +channel_op_res_t channel_get_file(channel_t *this, void *data, int file_handle); > +channel_op_res_t channel_get(channel_t *this, void *data); > +channel_op_res_t channel_curl_init(void); > This small change has the drawback that is conflicts when a new channel will be added. We have already an interface for a new channel, defined with the API above. A channel's user should call the initialization function (channel_new at the moment, we should also change this name) to get the right pointers. At the end, we need a sort of Factory Method, where the user of the channel will call the right method to instantiate. As far as I understand, you need this for the test framework in server_hawkbit_setup(). Waht about to call channel_new and use the returinng structure to get the function pointers ? Best regards, Stefano
Hi Stefano, > On 12/12/2017 15:47, Christian Storm wrote: > > The functions for opening, closing, receiving, and sending > > data over the channel should be exposed for testability as > > described in the header comment. > > > > This reverts commit f531694 as it breaks suricatta-tests. > > > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > --- > > include/channel_curl.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/include/channel_curl.h b/include/channel_curl.h > > index 98240a9..94d3c55 100644 > > --- a/include/channel_curl.h > > +++ b/include/channel_curl.h > > @@ -21,6 +21,7 @@ > > #include <json-c/json.h> > > #include <stdio.h> > > #include "sslapi.h" > > +#include <channel.h> > > > > /* hawkBit Channel Implementation Private Header File. > > * > > @@ -59,3 +60,10 @@ typedef struct { > > struct swupdate_digest *dgst; > > char sha1hash[SHA_DIGEST_LENGTH * 2 + 1]; > > } channel_data_t; > > + > > +channel_op_res_t channel_close(channel_t *this); > > +channel_op_res_t channel_open(channel_t *this, void *cfg); > > +channel_op_res_t channel_put(channel_t *this, void *data); > > +channel_op_res_t channel_get_file(channel_t *this, void *data, int file_handle); > > +channel_op_res_t channel_get(channel_t *this, void *data); > > +channel_op_res_t channel_curl_init(void); > > > > This small change has the drawback that is conflicts when a new channel > will be added. We have already an interface for a new channel, defined > with the API above. Yes, in include/channel.h, the struct channel {...} defines the API by means of function pointers and channel_new() is supposed to return a properly initialized struct for a particular channel transport method instance, here: curl. This is the general API each concrete channel implementation has to implement, regardless of the transport method being curl or whatever. include/channel_curl.h, however, is specific to the curl channel implementation, as the header comment says: /* Curl Channel Implementation Private Header File. * * This is a "private" header for testability, i.e., the declarations and * definitions herein should be used by code employing the curl channel * (e.g. server_hawkbit.c) and unit tests only. */ So, you're right in the sense that putting these in here means one has to also rename those on refactorings and the like. Agreed that we should avoid that. On the other hand, this is the place to put "data"/ declarations specific to the channel, here: the curl channel. However, those are not specific but general signatures, so agreed :) > A channel's user should call the initialization function (channel_new at > the moment, we should also change this name) to get the right pointers. > At the end, we need a sort of Factory Method, where the user of the > channel will call the right method to instantiate. OK, since that's initialization, channel_init would be a better name I guess. > As far as I understand, you need this for the test framework in > server_hawkbit_setup(). Waht about to call channel_new and use the > returinng structure to get the function pointers ? Yes, I need those in suricatta/test/test_server_hawkbit.c. The test framework relies on the linker's -Wl,--wrap= and hence messing with it dynamically at run-time is somewhat tedious and counters the framework's idea of how to do things. Nevertheless, I'll try to find a way around this and send a v2 shortly... Best regards, Christian
diff --git a/include/channel_curl.h b/include/channel_curl.h index 98240a9..94d3c55 100644 --- a/include/channel_curl.h +++ b/include/channel_curl.h @@ -21,6 +21,7 @@ #include <json-c/json.h> #include <stdio.h> #include "sslapi.h" +#include <channel.h> /* hawkBit Channel Implementation Private Header File. * @@ -59,3 +60,10 @@ typedef struct { struct swupdate_digest *dgst; char sha1hash[SHA_DIGEST_LENGTH * 2 + 1]; } channel_data_t; + +channel_op_res_t channel_close(channel_t *this); +channel_op_res_t channel_open(channel_t *this, void *cfg); +channel_op_res_t channel_put(channel_t *this, void *data); +channel_op_res_t channel_get_file(channel_t *this, void *data, int file_handle); +channel_op_res_t channel_get(channel_t *this, void *data); +channel_op_res_t channel_curl_init(void);
The functions for opening, closing, receiving, and sending data over the channel should be exposed for testability as described in the header comment. This reverts commit f531694 as it breaks suricatta-tests. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- include/channel_curl.h | 8 ++++++++ 1 file changed, 8 insertions(+)