You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/21 00:26:10 UTC
[GitHub] [incubator-nuttx-apps] gustavonihei opened a new pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
gustavonihei opened a new pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809
## Summary
This PR intends to revert commit ee1f4fdcdbbfbb02f4e9e9970b27e45a95a724e7.
After the mentioned commit an application no longer receives the HTTP header via the `sink_callback` when the amount of received bytes equals the size of the HTTP header (`ws->offset == ws->datend`).
This creates an issue for applications that requires information contained on the HTTP header (e.g., the value of `Content-Length`).
On the other hand, the same filtering from ee1f4fdcdbbfbb02f4e9e9970b27e45a95a724e7 could be performed by the application during the handling of the `sink_callback`, maintaining the same current behavior.
## Impact
Users of `webclient` application.
## Testing
Custom application.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-883852570
> i'm not sure if i understand what your app does.
> sink_callback is for the response body.
> as far as i know, right now there is no api for applications to investigate response headers.
> can you explain a bit?
Sure. I want to log the download progress. I intended to retrieve the file size by parsing the value of Content-Length from the header.
Currently it suceeds most of the time. But occasionally it meets that condition from the commit and fails to retrieve the file size, messing the download progress log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884142582
> > > i'm not sure if i understand what your app does.
> > > sink_callback is for the response body.
> > > as far as i know, right now there is no api for applications to investigate response headers.
> > > can you explain a bit?
> >
> >
> > Sure. I want to log the download progress. I intended to retrieve the file size by parsing the value of Content-Length from the header.
> > Currently it suceeds most of the time. But occasionally it meets that condition from the commit and fails to retrieve the file size, messing the download progress log.
>
> how does your app get the value of Content-Length?
> afaik, the current api doen't provide an access to response headers.
Actually it does via the `sink_callback`, see the API documentation:
https://github.com/apache/incubator-nuttx-apps/blob/master/include/netutils/webclient.h#L93-L95
What my app does is check whether `offset > 0`, which means the buffer contains the header data.
Then I parse the Content-Length field using standard library functions:
https://godbolt.org/z/TxYe3fzP6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884220783
> The API just needs to ensure that `header_line` is NULL-terminated, otherwise it may need to provide a `line_length` parameter.
I think we may invoke the callback from here: https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L586
Then the line string will be NULL-terminated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884220783
> The API just needs to ensure that `header_line` is NULL-terminated, otherwise it may need to provide a `line_length` parameter.
I think we my invoke the callback from here: https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L586
Then the line string will be NULL-terminated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-883835451
i'm not sure if i understand what your app does.
sink_callback is for the response body.
as far as i know, right now there is no api for applications to investigate response headers.
can you explain a bit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884186816
> > > > > what your app does won't work reliably even if this revert is merged.
> > > > > depending on how much data each recv() returns, the buffer before the offset might not contain the entire header. (in the worse case, sink_callback will never see offset > 0 at all.)
> > > > > it's better to introduce an explicit api to get headers.
> > > >
> > > >
> > > > Ok, it makes sense.
> > > > I'll create another callback specific for returning header data.
> > > > Thanks.
> > >
> > >
> > > can you share the rough api before implementing it?
> > > i have some use cases and want to give early feedbacks.
> >
> >
> > My initial idea was to make the webclient notify the HTTP header to the application line-by-line, by invoking the callback between those lines:
> > https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L561-L562
> > Then the application may parse the desired information.
>
> it sounds like a plan.
> how about the "truncated" case?
Haven't thought about it yet. Which restrictions should this API be aware of?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884142582
> > > i'm not sure if i understand what your app does.
> > > sink_callback is for the response body.
> > > as far as i know, right now there is no api for applications to investigate response headers.
> > > can you explain a bit?
> >
> >
> > Sure. I want to log the download progress. I intended to retrieve the file size by parsing the value of Content-Length from the header.
> > Currently it suceeds most of the time. But occasionally it meets that condition from the commit and fails to retrieve the file size, messing the download progress log.
>
> how does your app get the value of Content-Length?
> afaik, the current api doen't provide an access to response headers.
Actually it does via the `sink_callback`, see the API documentation:
https://github.com/apache/incubator-nuttx-apps/blob/master/include/netutils/webclient.h#L93-L95
What my app does is check whether `offset > 0`, which means the beginning of the buffer contains the header data.
Then I parse the Content-Length field using standard library functions:
https://godbolt.org/z/TxYe3fzP6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884198027
> i guess we should either of the following:
>
> * remove CONFIG_WEBCLIENT_MAXHTTPLINE restriction and support arbitrary length.
> * or, report some headers are incomplete.
>
> i think the latter is good enough. (at least my use cases)
> how about:
>
> ```
> int (*header_callback)(const char *header_line, bool truncated, void *arg);
> void *header_callback_arg;
> ```
>
> if the callback returns -errno, it makes the entire webclient_perform fail.
Looks good, this is similar to what I had in mind.
My use case is quite simple, so this proposal would also fit.
The API just needs to ensure that `header_line` is either NULL-terminated, otherwise it may need to provide a `line_length` parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884173610
> > what your app does won't work reliably even if this revert is merged.
> > depending on how much data each recv() returns, the buffer before the offset might not contain the entire header. (in the worse case, sink_callback will never see offset > 0 at all.)
> > it's better to introduce an explicit api to get headers.
>
> Ok, it makes sense.
> I'll create another callback specific for returning header data.
> Thanks.
can you share the rough api before implementing it?
i have some use cases and want to give early feedbacks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884150784
> > > > i'm not sure if i understand what your app does.
> > > > sink_callback is for the response body.
> > > > as far as i know, right now there is no api for applications to investigate response headers.
> > > > can you explain a bit?
> > >
> > >
> > > Sure. I want to log the download progress. I intended to retrieve the file size by parsing the value of Content-Length from the header.
> > > Currently it suceeds most of the time. But occasionally it meets that condition from the commit and fails to retrieve the file size, messing the download progress log.
> >
> >
> > how does your app get the value of Content-Length?
> > afaik, the current api doen't provide an access to response headers.
>
> Actually it does via the `sink_callback`, see the API documentation:
> https://github.com/apache/incubator-nuttx-apps/blob/master/include/netutils/webclient.h#L93-L95
>
> What my app does is check whether `offset > 0`, which means the beginning of the buffer contains the header data.
> Then I parse the Content-Length field using standard library functions:
> https://godbolt.org/z/TxYe3fzP6
ok. i understand what your app does. thank you for explanation.
honestly speaking, i think it's an abuse of the api.
btw, i have some use cases related to Content-Length too. i had a plan to introduce a separate callback for headers. but it has been blocked because https://github.com/apache/incubator-nuttx-apps/pull/695 has been blocked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884181913
> > > > what your app does won't work reliably even if this revert is merged.
> > > > depending on how much data each recv() returns, the buffer before the offset might not contain the entire header. (in the worse case, sink_callback will never see offset > 0 at all.)
> > > > it's better to introduce an explicit api to get headers.
> > >
> > >
> > > Ok, it makes sense.
> > > I'll create another callback specific for returning header data.
> > > Thanks.
> >
> >
> > can you share the rough api before implementing it?
> > i have some use cases and want to give early feedbacks.
>
> My initial idea was to make the webclient notify the HTTP header to the application line-by-line, by invoking the callback between those lines:
> https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L561-L562
> Then the application may parse the desired information.
it sounds like a plan.
how about the "truncated" case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884600636
> > The API just needs to ensure that `header_line` is NULL-terminated, otherwise it may need to provide a `line_length` parameter.
>
> I think we may invoke the callback from here: https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L599
> Then the line string will be NULL-terminated.
sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-883871081
> > i'm not sure if i understand what your app does.
> > sink_callback is for the response body.
> > as far as i know, right now there is no api for applications to investigate response headers.
> > can you explain a bit?
>
> Sure. I want to log the download progress. I intended to retrieve the file size by parsing the value of Content-Length from the header.
> Currently it suceeds most of the time. But occasionally it meets that condition from the commit and fails to retrieve the file size, messing the download progress log.
how does your app get the value of Content-Length?
afaik, the current api doen't provide an access to response headers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884162562
> what your app does won't work reliably even if this revert is merged.
> depending on how much data each recv() returns, the buffer before the offset might not contain the entire header. (in the worse case, sink_callback will never see offset > 0 at all.)
> it's better to introduce an explicit api to get headers.
Ok, it makes sense.
I'll create another callback specific for returning header data.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884220783
> The API just needs to ensure that `header_line` is NULL-terminated, otherwise it may need to provide a `line_length` parameter.
I think we may invoke the callback from here: https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L599
Then the line string will be NULL-terminated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884176837
> > > what your app does won't work reliably even if this revert is merged.
> > > depending on how much data each recv() returns, the buffer before the offset might not contain the entire header. (in the worse case, sink_callback will never see offset > 0 at all.)
> > > it's better to introduce an explicit api to get headers.
> >
> >
> > Ok, it makes sense.
> > I'll create another callback specific for returning header data.
> > Thanks.
>
> can you share the rough api before implementing it?
> i have some use cases and want to give early feedbacks.
My initial idea was to make the webclient notify the HTTP header to the application line-by-line, by invoking the callback between those lines:
https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L561-L562
Then the application may parse the desired information.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884192045
> > > > > > what your app does won't work reliably even if this revert is merged.
> > > > > > depending on how much data each recv() returns, the buffer before the offset might not contain the entire header. (in the worse case, sink_callback will never see offset > 0 at all.)
> > > > > > it's better to introduce an explicit api to get headers.
> > > > >
> > > > >
> > > > > Ok, it makes sense.
> > > > > I'll create another callback specific for returning header data.
> > > > > Thanks.
> > > >
> > > >
> > > > can you share the rough api before implementing it?
> > > > i have some use cases and want to give early feedbacks.
> > >
> > >
> > > My initial idea was to make the webclient notify the HTTP header to the application line-by-line, by invoking the callback between those lines:
> > > https://github.com/apache/incubator-nuttx-apps/blob/master/netutils/webclient/webclient.c#L561-L562
> > > Then the application may parse the desired information.
> >
> >
> > it sounds like a plan.
> > how about the "truncated" case?
>
> Haven't thought about it yet. Which restrictions should this API be aware of?
i guess we should either of the following:
* remove CONFIG_WEBCLIENT_MAXHTTPLINE restriction and support arbitrary length.
* or, report some headers are incomplete.
i think the latter is good enough. (at least my use cases)
how about:
```
int (*header_callback)(const char *header_line, bool truncated, void *arg);
void *header_callback_arg;
```
if the callback returns -errno, it makes the entire webclient_perform fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei closed pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei closed pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #809: Revert "webclient: Don't call the sink callback if no data is available"
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #809:
URL: https://github.com/apache/incubator-nuttx-apps/pull/809#issuecomment-884198027
> i guess we should either of the following:
>
> * remove CONFIG_WEBCLIENT_MAXHTTPLINE restriction and support arbitrary length.
> * or, report some headers are incomplete.
>
> i think the latter is good enough. (at least my use cases)
> how about:
>
> ```
> int (*header_callback)(const char *header_line, bool truncated, void *arg);
> void *header_callback_arg;
> ```
>
> if the callback returns -errno, it makes the entire webclient_perform fail.
Looks good, this is similar to what I had in mind.
My use case is quite simple, so this proposal would also fit.
The API just needs to ensure that `header_line` is NULL-terminated, otherwise it may need to provide a `line_length` parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org