| Message ID | 20201020112824.24101-1-christian.storm@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | channel_curl: Make curl's progress reporting configurable | expand |
Hi Christian, On 20.10.20 13:28, Christian Storm wrote: > Make curl's progress reporting configurable so that, e.g., a progress > notification thread running concurrently to an artifact download doesn't > report its progress but only the artifact download progress. > It is not clear to me if this should be done here or at the receiver side. The downloader calls a notify() to send an info message, and one way is to make distinguishable the two different notification (install progress vs download progress). > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > corelib/channel_curl.c | 3 ++- > include/channel_curl.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > index 8d119b4..e24e907 100644 > --- a/corelib/channel_curl.c > +++ b/corelib/channel_curl.c > @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da > goto cleanup; > } > #endif > - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { > + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, > + (long)channel_data->noprogress) != CURLE_OK) { > result = CHANNEL_EINIT; > goto cleanup; > } > diff --git a/include/channel_curl.h b/include/channel_curl.h > index c3418a5..7b51b34 100644 > --- a/include/channel_curl.h > +++ b/include/channel_curl.h > @@ -66,6 +66,7 @@ typedef struct { > bool nocheckanswer; > long http_response_code; > bool nofollow; > + bool noprogress; > int (*checkdwl)(void); > struct swupdate_digest *dgst; > char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1]; > Anyway, patch adds a variable, it is sets to zero (as before), this patch does nothing. Something missing ? Best regards, Stefano
Hi Stefano, > On 20.10.20 13:28, Christian Storm wrote: > > Make curl's progress reporting configurable so that, e.g., a progress > > notification thread running concurrently to an artifact download doesn't > > report its progress but only the artifact download progress. > > > > It is not clear to me if this should be done here or at the receiver > side. The downloader calls a notify() to send an info message, and one > way is to make distinguishable the two different notification (install > progress vs download progress). Currently it's PROGRESS and that reports download progress (as JSON) via the progress interface, which is fine. The receiver, however, can't tell whether the download progress comes from the actual artifact download or from the reply "download" when POST/PATCH reporting the artifact download progress to the backend server. This wouldn't be solved with two notification types as both (actual artifact download and backend server progress notification reply "download") are emitted via (download) PROGRESS messages. A solution would be to tell the progress receiver from which channel it got the notification. But then the progress client has to know the channels in order to filter out unwanted messages... I'd rather opt for not burdening the progress clients with this. Instead, when creating the channel, say in suricatta, I can configure the download progress reporting channel not to emit its reply "download" progress. This is what this patch is for. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > --- > > corelib/channel_curl.c | 3 ++- > > include/channel_curl.h | 1 + > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > > index 8d119b4..e24e907 100644 > > --- a/corelib/channel_curl.c > > +++ b/corelib/channel_curl.c > > @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da > > goto cleanup; > > } > > #endif > > - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { > > + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, > > + (long)channel_data->noprogress) != CURLE_OK) { > > result = CHANNEL_EINIT; > > goto cleanup; > > } > > diff --git a/include/channel_curl.h b/include/channel_curl.h > > index c3418a5..7b51b34 100644 > > --- a/include/channel_curl.h > > +++ b/include/channel_curl.h > > @@ -66,6 +66,7 @@ typedef struct { > > bool nocheckanswer; > > long http_response_code; > > bool nofollow; > > + bool noprogress; > > int (*checkdwl)(void); > > struct swupdate_digest *dgst; > > char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1]; > > > > Anyway, patch adds a variable, it is sets to zero (as before), this > patch does nothing. Not until you enable the flag, that is :) That's intended to keep compatibility with previous behavior if not explicitly told otherwise. > Something missing ? Nope, I don't think so. Kind regards, Christian
Hi Christian, On 21.10.20 11:12, Christian Storm wrote: > Hi Stefano, > >> On 20.10.20 13:28, Christian Storm wrote: >>> Make curl's progress reporting configurable so that, e.g., a progress >>> notification thread running concurrently to an artifact download doesn't >>> report its progress but only the artifact download progress. >>> >> >> It is not clear to me if this should be done here or at the receiver >> side. The downloader calls a notify() to send an info message, and one >> way is to make distinguishable the two different notification (install >> progress vs download progress). > > Currently it's PROGRESS and that reports download progress (as JSON) via > the progress interface, which is fine. The receiver, however, can't tell > whether the download progress comes from the actual artifact download or > from the reply "download" when POST/PATCH reporting the artifact > download progress to the backend server. Right, I see the problem. But the progress interface has already a field for this: 25 struct progress_msg { 26 unsigned int magic; /* Magic Number */ 27 RECOVERY_STATUS status; /* Update St... 28 unsigned int dwl_percent; /* % downloaded data */ This dwl_percent is then foreseen, but unused, and I asked myself if we should then use it. Nevertheless, download percent and installation percent are running at the same time if streaming (installed-directly) is set. The listener can ignore dwl_percent or not. We have quite two mechanism (a field + a JSON message) to notify the same information. > This wouldn't be solved with two notification types as both (actual > artifact download and backend server progress notification reply > "download") are emitted via (download) PROGRESS messages. Ok, I see we need to enable it just in channel_get_file() > A solution would be to tell the progress receiver from which channel it > got the notification. But then the progress client has to know the > channels But the progress client receives messages where dwl_percent is set, and it can evaluate it together with a progress installation. > in order to filter out unwanted messages... I'd rather opt for > not burdening the progress clients with this. Instead, when creating the > channel, say in suricatta, I can configure the download progress > reporting channel not to emit its reply "download" progress. > This is what this patch is for. > > >>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>> --- >>> corelib/channel_curl.c | 3 ++- >>> include/channel_curl.h | 1 + >>> 2 files changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c >>> index 8d119b4..e24e907 100644 >>> --- a/corelib/channel_curl.c >>> +++ b/corelib/channel_curl.c >>> @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da >>> goto cleanup; >>> } >>> #endif >>> - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { >>> + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, >>> + (long)channel_data->noprogress) != CURLE_OK) { >>> result = CHANNEL_EINIT; >>> goto cleanup; >>> } >>> diff --git a/include/channel_curl.h b/include/channel_curl.h >>> index c3418a5..7b51b34 100644 >>> --- a/include/channel_curl.h >>> +++ b/include/channel_curl.h >>> @@ -66,6 +66,7 @@ typedef struct { >>> bool nocheckanswer; >>> long http_response_code; >>> bool nofollow; >>> + bool noprogress; I do not understand who sets noprogress = 1 to disable the notification as you want. >>> int (*checkdwl)(void); >>> struct swupdate_digest *dgst; >>> char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1]; >>> >> >> Anyway, patch adds a variable, it is sets to zero (as before), this >> patch does nothing. > > Not until you enable the flag, that is :) Right, but there is no code in SWUpdate setting the flag, so the patch looks useless. Where is set this flag ? > That's intended to keep compatibility with previous behavior if not > explicitly told otherwise. > >> Something missing ? > > Nope, I don't think so. > Best regards, Stefano
Hi Stefano, > >>> Make curl's progress reporting configurable so that, e.g., a progress > >>> notification thread running concurrently to an artifact download doesn't > >>> report its progress but only the artifact download progress. > >>> > >> > >> It is not clear to me if this should be done here or at the receiver > >> side. The downloader calls a notify() to send an info message, and one > >> way is to make distinguishable the two different notification (install > >> progress vs download progress). > > > > Currently it's PROGRESS and that reports download progress (as JSON) via > > the progress interface, which is fine. The receiver, however, can't tell > > whether the download progress comes from the actual artifact download or > > from the reply "download" when POST/PATCH reporting the artifact > > download progress to the backend server. > > Right, I see the problem. > > But the progress interface has already a field for this: > > 25 struct progress_msg { > 26 unsigned int magic; /* Magic Number */ > 27 RECOVERY_STATUS status; /* Update St... > 28 unsigned int dwl_percent; /* % downloaded data */ > > This dwl_percent is then foreseen, but unused, and I asked myself if we > should then use it. Nevertheless, download percent and installation > percent are running at the same time if streaming (installed-directly) > is set. The listener can ignore dwl_percent or not. We have quite two > mechanism (a field + a JSON message) to notify the same information. Yes, true, dwl_percent is defined, documented, and unused :) This is an example, as a Lua Table, of what is emitted currently (without installed-directly): nsteps: 1.0 source: 2.0 dwl_percent: 0.0 hnd_name: info: {'0': {"percent": 100, "msg":"Received 5238784B of 5238784B"}} status: 8.0 cur_percent: 0.0 cur_image: cur_step: 0.0 So, the .info has to become the JSON's "msg" content and the .dwl_percent has to become JSON's "percent" then, right? I would favor this over the JSON encoding in .info: Then, a progress client has the information readily available without parsing JSON first. Question is about backwards-compatibility and what we break by this change if we set out to do it? > > This wouldn't be solved with two notification types as both (actual > > artifact download and backend server progress notification reply > > "download") are emitted via (download) PROGRESS messages. > > Ok, I see we need to enable it just in channel_get_file() This would also be an option, yes. Having it in the current place is IMO more generic though as it applies to all curl channels (not just the artifact downloading one). > > A solution would be to tell the progress receiver from which channel it > > got the notification. But then the progress client has to know the > > channels > > But the progress client receives messages where dwl_percent is set, and > it can evaluate it together with a progress installation. Yes, if dwl_percent is properly set and the context is unambiguous, i.e., it's set and artifact download or download+installation is in progress. But why limit a channel's download progress reporting to this case only and not let the channel creator (e.g., suricatta) decide? > > in order to filter out unwanted messages... I'd rather opt for > > not burdening the progress clients with this. Instead, when creating the > > channel, say in suricatta, I can configure the download progress > > reporting channel not to emit its reply "download" progress. > > This is what this patch is for. > > > > > >>> Signed-off-by: Christian Storm <christian.storm@siemens.com> > >>> --- > >>> corelib/channel_curl.c | 3 ++- > >>> include/channel_curl.h | 1 + > >>> 2 files changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > >>> index 8d119b4..e24e907 100644 > >>> --- a/corelib/channel_curl.c > >>> +++ b/corelib/channel_curl.c > >>> @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da > >>> goto cleanup; > >>> } > >>> #endif > >>> - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { > >>> + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, > >>> + (long)channel_data->noprogress) != CURLE_OK) { > >>> result = CHANNEL_EINIT; > >>> goto cleanup; > >>> } > >>> diff --git a/include/channel_curl.h b/include/channel_curl.h > >>> index c3418a5..7b51b34 100644 > >>> --- a/include/channel_curl.h > >>> +++ b/include/channel_curl.h > >>> @@ -66,6 +66,7 @@ typedef struct { > >>> bool nocheckanswer; > >>> long http_response_code; > >>> bool nofollow; > >>> + bool noprogress; > > I do not understand who sets noprogress = 1 to disable the notification > as you want. Yes, see below answer. > >>> int (*checkdwl)(void); > >>> struct swupdate_digest *dgst; > >>> char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1]; > >>> > >> > >> Anyway, patch adds a variable, it is sets to zero (as before), this > >> patch does nothing. > > > > Not until you enable the flag, that is :) > > Right, but there is no code in SWUpdate setting the flag, so the patch > looks useless. Where is set this flag ? Ah, I see. Yes, you're right. It's currently not used as I didn't want to change existing behavior. However, canonical candidates are for sure suricatta/server_hawkbit.c's process_notification_thread() and suricatta/server_general.c's server_progress_thread() so that their channel operations don't emit their progress via the progress interface. And, of course, new suricatta modules may make use of this... Kind regards, Christian
Hi Christian, On 21.10.20 21:00, Christian Storm wrote: > Hi Stefano, > >>>>> Make curl's progress reporting configurable so that, e.g., a progress >>>>> notification thread running concurrently to an artifact download doesn't >>>>> report its progress but only the artifact download progress. >>>>> >>>> >>>> It is not clear to me if this should be done here or at the receiver >>>> side. The downloader calls a notify() to send an info message, and one >>>> way is to make distinguishable the two different notification (install >>>> progress vs download progress). >>> >>> Currently it's PROGRESS and that reports download progress (as JSON) via >>> the progress interface, which is fine. The receiver, however, can't tell >>> whether the download progress comes from the actual artifact download or >>> from the reply "download" when POST/PATCH reporting the artifact >>> download progress to the backend server. >> >> Right, I see the problem. >> >> But the progress interface has already a field for this: >> >> 25 struct progress_msg { >> 26 unsigned int magic; /* Magic Number */ >> 27 RECOVERY_STATUS status; /* Update St... >> 28 unsigned int dwl_percent; /* % downloaded data */ >> >> This dwl_percent is then foreseen, but unused, and I asked myself if we >> should then use it. Nevertheless, download percent and installation >> percent are running at the same time if streaming (installed-directly) >> is set. The listener can ignore dwl_percent or not. We have quite two >> mechanism (a field + a JSON message) to notify the same information. > > Yes, true, dwl_percent is defined, documented, and unused :) > Right. > This is an example, as a Lua Table, of what is emitted currently > (without installed-directly): > > nsteps: 1.0 > source: 2.0 > dwl_percent: 0.0 > hnd_name: > info: {'0': {"percent": 100, "msg":"Received 5238784B of 5238784B"}} > status: 8.0 > cur_percent: 0.0 > cur_image: > cur_step: 0.0 > > So, the .info has to become the JSON's "msg" content and the .dwl_percent > has to become JSON's "percent" then, right? This is the only place in code where the download percentage is set. It could be that further places will needed and I will prefer to add a clear interface like a swupdate_update_download() instead of encoding values here. The function swupdate_update_download() will then prepare a suitable message (and set dwl_percent as well) to be emitted for the progress' listeners. I can prepare a patch in this direction if we agree. > I would favor this over the JSON encoding in .info: Then, a progress > client has the information readily available without parsing JSON first. Right - I ten to drop the info message. Is it worth to pass the received data together with percentage ? > > Question is about backwards-compatibility and what we break by this > change if we set out to do it? Like the IPC api change, but I will like to have clean up this point. > > >>> This wouldn't be solved with two notification types as both (actual >>> artifact download and backend server progress notification reply >>> "download") are emitted via (download) PROGRESS messages. >> >> Ok, I see we need to enable it just in channel_get_file() > > This would also be an option, yes. Having it in the current place is IMO > more generic though as it applies to all curl channels (not just the > artifact downloading one). Ok > > >>> A solution would be to tell the progress receiver from which channel it >>> got the notification. But then the progress client has to know the >>> channels >> >> But the progress client receives messages where dwl_percent is set, and >> it can evaluate it together with a progress installation. > > Yes, if dwl_percent is properly set and the context is unambiguous, > i.e., it's set and artifact download or download+installation is in > progress. Right. > > But why limit a channel's download progress reporting to this case only > and not let the channel creator (e.g., suricatta) decide? Also possible. > > >>> in order to filter out unwanted messages... I'd rather opt for >>> not burdening the progress clients with this. Instead, when creating the >>> channel, say in suricatta, I can configure the download progress >>> reporting channel not to emit its reply "download" progress. >>> This is what this patch is for. >>> >>> >>>>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>>>> --- >>>>> corelib/channel_curl.c | 3 ++- >>>>> include/channel_curl.h | 1 + >>>>> 2 files changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c >>>>> index 8d119b4..e24e907 100644 >>>>> --- a/corelib/channel_curl.c >>>>> +++ b/corelib/channel_curl.c >>>>> @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da >>>>> goto cleanup; >>>>> } >>>>> #endif >>>>> - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { >>>>> + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, >>>>> + (long)channel_data->noprogress) != CURLE_OK) { >>>>> result = CHANNEL_EINIT; >>>>> goto cleanup; >>>>> } >>>>> diff --git a/include/channel_curl.h b/include/channel_curl.h >>>>> index c3418a5..7b51b34 100644 >>>>> --- a/include/channel_curl.h >>>>> +++ b/include/channel_curl.h >>>>> @@ -66,6 +66,7 @@ typedef struct { >>>>> bool nocheckanswer; >>>>> long http_response_code; >>>>> bool nofollow; >>>>> + bool noprogress; >> >> I do not understand who sets noprogress = 1 to disable the notification >> as you want. > > Yes, see below answer. > > >>>>> int (*checkdwl)(void); >>>>> struct swupdate_digest *dgst; >>>>> char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1]; >>>>> >>>> >>>> Anyway, patch adds a variable, it is sets to zero (as before), this >>>> patch does nothing. >>> >>> Not until you enable the flag, that is :) >> >> Right, but there is no code in SWUpdate setting the flag, so the patch >> looks useless. Where is set this flag ? > > Ah, I see. Yes, you're right. It's currently not used as I didn't want > to change existing behavior. ...and then the patch is useless, at least at the moment. > However, canonical candidates are for sure > suricatta/server_hawkbit.c's process_notification_thread() and > suricatta/server_general.c's server_progress_thread() Ok - should be hardcoded then ? Or should be a configurable value ? > so that their channel operations don't emit their progress via the > progress interface. > And, of course, new suricatta modules may make use of this... > Best regards, Stefano
Hi Stefano, > >>>>> Make curl's progress reporting configurable so that, e.g., a progress > >>>>> notification thread running concurrently to an artifact download doesn't > >>>>> report its progress but only the artifact download progress. > >>>>> > >>>> > >>>> It is not clear to me if this should be done here or at the receiver > >>>> side. The downloader calls a notify() to send an info message, and one > >>>> way is to make distinguishable the two different notification (install > >>>> progress vs download progress). > >>> > >>> Currently it's PROGRESS and that reports download progress (as JSON) via > >>> the progress interface, which is fine. The receiver, however, can't tell > >>> whether the download progress comes from the actual artifact download or > >>> from the reply "download" when POST/PATCH reporting the artifact > >>> download progress to the backend server. > >> > >> Right, I see the problem. > >> > >> But the progress interface has already a field for this: > >> > >> 25 struct progress_msg { > >> 26 unsigned int magic; /* Magic Number */ > >> 27 RECOVERY_STATUS status; /* Update St... > >> 28 unsigned int dwl_percent; /* % downloaded data */ > >> > >> This dwl_percent is then foreseen, but unused, and I asked myself if we > >> should then use it. Nevertheless, download percent and installation > >> percent are running at the same time if streaming (installed-directly) > >> is set. The listener can ignore dwl_percent or not. We have quite two > >> mechanism (a field + a JSON message) to notify the same information. > > > > Yes, true, dwl_percent is defined, documented, and unused :) > > > > Right. > > > This is an example, as a Lua Table, of what is emitted currently > > (without installed-directly): > > > > nsteps: 1.0 > > source: 2.0 > > dwl_percent: 0.0 > > hnd_name: > > info: {'0': {"percent": 100, "msg":"Received 5238784B of 5238784B"}} > > status: 8.0 > > cur_percent: 0.0 > > cur_image: > > cur_step: 0.0 > > > > So, the .info has to become the JSON's "msg" content and the .dwl_percent > > has to become JSON's "percent" then, right? > > This is the only place in code where the download percentage is set. It > could be that further places will needed and I will prefer to add a > clear interface like a swupdate_update_download() instead of encoding > values here. > > The function swupdate_update_download() will then prepare a suitable > message (and set dwl_percent as well) to be emitted for the progress' > listeners. I can prepare a patch in this direction if we agree. Yes, makes sense. We could name it {swupdate_,}emit_download_progress although that's kind of a long name :) swupdate_update_download() is not a too telling name I think. > > I would favor this over the JSON encoding in .info: Then, a progress > > client has the information readily available without parsing JSON first. > > Right - I ten to drop the info message. Is it worth to pass the received > data together with percentage ? Well, if you keep the total (file) size, you can calculate with this and the percentage (almost) the bytes transferred. Hence, it's redundant information. So, I would suggest to keep (1) the percentage and (2) the total (file) size, e.g., for time estimates (assuming the current download speed remains) giving you a nice progress bar with an ETA. > > Question is about backwards-compatibility and what we break by this > > change if we set out to do it? > > Like the IPC api change, but I will like to have clean up this point. Understood, I do fully agree here. > >>> This wouldn't be solved with two notification types as both (actual > >>> artifact download and backend server progress notification reply > >>> "download") are emitted via (download) PROGRESS messages. > >> > >> Ok, I see we need to enable it just in channel_get_file() > > > > This would also be an option, yes. Having it in the current place is IMO > > more generic though as it applies to all curl channels (not just the > > artifact downloading one). > > Ok > > > > > > >>> A solution would be to tell the progress receiver from which channel it > >>> got the notification. But then the progress client has to know the > >>> channels > >> > >> But the progress client receives messages where dwl_percent is set, and > >> it can evaluate it together with a progress installation. > > > > Yes, if dwl_percent is properly set and the context is unambiguous, > > i.e., it's set and artifact download or download+installation is in > > progress. > > Right. > > > > > But why limit a channel's download progress reporting to this case only > > and not let the channel creator (e.g., suricatta) decide? > > Also possible. I would opt for this and let the channel creator decide whether that information is useful or not for being emitted as progress message. At least the channel creator should be able to override progress reporting for a particular channel to not emit progress messages. (See below for an example.) > >>> in order to filter out unwanted messages... I'd rather opt for > >>> not burdening the progress clients with this. Instead, when creating the > >>> channel, say in suricatta, I can configure the download progress > >>> reporting channel not to emit its reply "download" progress. > >>> This is what this patch is for. > >>> > >>> > >>>>> Signed-off-by: Christian Storm <christian.storm@siemens.com> > >>>>> --- > >>>>> corelib/channel_curl.c | 3 ++- > >>>>> include/channel_curl.h | 1 + > >>>>> 2 files changed, 3 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > >>>>> index 8d119b4..e24e907 100644 > >>>>> --- a/corelib/channel_curl.c > >>>>> +++ b/corelib/channel_curl.c > >>>>> @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da > >>>>> goto cleanup; > >>>>> } > >>>>> #endif > >>>>> - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { > >>>>> + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, > >>>>> + (long)channel_data->noprogress) != CURLE_OK) { > >>>>> result = CHANNEL_EINIT; > >>>>> goto cleanup; > >>>>> } > >>>>> diff --git a/include/channel_curl.h b/include/channel_curl.h > >>>>> index c3418a5..7b51b34 100644 > >>>>> --- a/include/channel_curl.h > >>>>> +++ b/include/channel_curl.h > >>>>> @@ -66,6 +66,7 @@ typedef struct { > >>>>> bool nocheckanswer; > >>>>> long http_response_code; > >>>>> bool nofollow; > >>>>> + bool noprogress; > >> > >> I do not understand who sets noprogress = 1 to disable the notification > >> as you want. > > > > Yes, see below answer. > > > > > >>>>> int (*checkdwl)(void); > >>>>> struct swupdate_digest *dgst; > >>>>> char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1]; > >>>>> > >>>> > >>>> Anyway, patch adds a variable, it is sets to zero (as before), this > >>>> patch does nothing. > >>> > >>> Not until you enable the flag, that is :) > >> > >> Right, but there is no code in SWUpdate setting the flag, so the patch > >> looks useless. Where is set this flag ? > > > > Ah, I see. Yes, you're right. It's currently not used as I didn't want > > to change existing behavior. > > ...and then the patch is useless, at least at the moment. That's correct. > > However, canonical candidates are for sure > > suricatta/server_hawkbit.c's process_notification_thread() and > > suricatta/server_general.c's server_progress_thread() > > Ok - should be hardcoded then ? Or should be a configurable value ? Hm, as suricatta module writer I do create the channels for a purpose and should be able to decide on whether the channel should emit progress reporting that is sensible or not. So, yes, hard-coded, depending on the channel's purpose: In my Lua suricatta module I do set this for the download progress reporting backchannel to the server so to not clutter the progress messages with its progress which is unneeded information. For the actual channel downloading the artifact (.swu) I do naturally not set this flag (default) as I'm actually interested in its progress to report it to the backend server. In both cases, it's hard-coded as I can't image someone having interest in the progress reporting channel's progress reporting :) > > so that their channel operations don't emit their progress via the > > progress interface. > > And, of course, new suricatta modules may make use of this... Kind regards, Christian
diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c index 8d119b4..e24e907 100644 --- a/corelib/channel_curl.c +++ b/corelib/channel_curl.c @@ -552,7 +552,8 @@ channel_op_res_t channel_set_options(channel_t *this, channel_data_t *channel_da goto cleanup; } #endif - if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, 0L) != CURLE_OK) { + if (curl_easy_setopt(channel_curl->handle, CURLOPT_NOPROGRESS, + (long)channel_data->noprogress) != CURLE_OK) { result = CHANNEL_EINIT; goto cleanup; } diff --git a/include/channel_curl.h b/include/channel_curl.h index c3418a5..7b51b34 100644 --- a/include/channel_curl.h +++ b/include/channel_curl.h @@ -66,6 +66,7 @@ typedef struct { bool nocheckanswer; long http_response_code; bool nofollow; + bool noprogress; int (*checkdwl)(void); struct swupdate_digest *dgst; char sha1hash[SWUPDATE_SHA_DIGEST_LENGTH * 2 + 1];
Make curl's progress reporting configurable so that, e.g., a progress notification thread running concurrently to an artifact download doesn't report its progress but only the artifact download progress. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- corelib/channel_curl.c | 3 ++- include/channel_curl.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)