Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tool_writeout: add new writeout variable, num_headers #5947

Closed
wants to merge 1 commit into from

Conversation

@anio
Copy link
Contributor

@anio anio commented Sep 9, 2020

It's a draft PR.
This variable gives the number of headers. Currently it just work with disabled redirect because I didn't find out how to reset the header counter while redirect.
And also this variable is not present in JSON-formatted writeout because the JSON handler just works with curl_easy_getinfo.

@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented Sep 9, 2020

@anio
Copy link
Contributor Author

@anio anio commented Sep 9, 2020

I'm using curl to do some sort of mass tests.
image

This variable may be helpful for example when I'm looking for some caching issues.
Or when I'm looking for routes on a website those are use a different function/server (e.g. proxy_pass)
image
image

And also some times it may be helpful to detect soft 404 pages.

p.s. In the next step I will add a new writeout variable to read header value. e.g. %{header[xyz-cache]}.

Copy link
Member

@bagder bagder left a comment

  1. There's no docs for the variable
  2. How's it supposed to count amount of headers when one or more redirect are involved?
  3. Test 970 needs update too right?
case VAR_HEADER_COUNT:
{
fprintf(stream, "%ld", per->header_count);
}

This comment has been minimized.

@bagder

bagder Sep 9, 2020
Member

You don't need these braces.

if(hdrcbdata->config->writeout) {
char *value = NULL;

value = memchr(ptr, ':', cb);

This comment has been minimized.

@emilengler

emilengler Sep 10, 2020
Contributor

Why do this later. You can also init at variable creation

if(outs->filename)
fprintf(stream, "%s", outs->filename);
if(per->outs.filename)
fprintf(stream, "%s", per->outs.filename);

This comment has been minimized.

@emilengler

emilengler Sep 10, 2020
Contributor

You can also use fputs. I don't think it's necessary to have the printf overhead here

This comment has been minimized.

@anio

anio Sep 10, 2020
Author Contributor

You are right, but this is a thing of the past and not changes I made, but I will change it.

This comment has been minimized.

@emilengler

emilengler Sep 10, 2020
Contributor

Sorry but I don't know what you mean

This comment has been minimized.

@anio

anio Sep 11, 2020
Author Contributor

I mean I didn't wrote fprintf(stream, "%s", .

This comment has been minimized.

@emilengler

emilengler Sep 11, 2020
Contributor

Still something you can change :-)

@anio
Copy link
Contributor Author

@anio anio commented Sep 10, 2020

1. There's no docs for the variable

I will write soon.

2. How's it supposed to count amount of headers when one or more redirect are involved?

I think same as http_code we should just count the last response header count. I don't know how to do that.

3. Test 970 needs update too right?

Yes, after updating the JSON response to include header_count. I should update tool_writeout_json. As I mentioned before, it currently just works with curl_easy_getinfo.

@bagder
Copy link
Member

@bagder bagder commented Sep 10, 2020

I don't know how to do that.

You get an "empty" header last, don't you? That means the end of the headers for this response.

@anio
Copy link
Contributor Author

@anio anio commented Sep 10, 2020

You get an "empty" header last, don't you? That means the end of the headers for this response.

Right, but how it can help when I don't know about previous responses? Do I need to count redirects?

@bagder
Copy link
Member

@bagder bagder commented Sep 10, 2020

Shouldn't you just clear the counter when you get a new set of headers after a previous empty header? That way you'd always count only the last set.

@anio anio force-pushed the anio:new-writeout-var-header_count branch from c2d3858 to a88904e Sep 11, 2020
@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Sep 11, 2020

Congratulations 🎉. DeepCode analyzed your code in 0.318 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@anio anio requested a review from bagder Sep 12, 2020
Copy link
Member

@bagder bagder left a comment

Looks fine! How about adding a test with a redirect as well just to make sure it does the right thing then as well?

docs/cmdline-opts/write-out.d Outdated Show resolved Hide resolved
@@ -39,6 +39,9 @@ option. (Added in 7.26.0)
The initial path curl ended up in when logging on to the remote FTP
server. (Added in 7.15.4)
.TP
.B header_count
The count of retrieved headers.

This comment has been minimized.

@bagder

bagder Sep 12, 2020
Member

I suggest:: "The number of response headers in the most recent request (restarted at each redirect)"

Should this mention that it doesn't include the first status line?

This comment has been minimized.

@anio

anio Sep 13, 2020
Author Contributor

Should this mention that it doesn't include the first status line?

I think it's not necessary when we are talking about headers.

This comment has been minimized.

@bagder

bagder Sep 13, 2020
Member

I've just learned that some users think of that as a (special) header too.

This comment has been minimized.

@anio

anio Sep 13, 2020
Author Contributor

Oops!
By mentioning this, we also have to mention: the status line which includes the status code and reason message IS NOT a header.

So, do you prefer to mention?

This comment has been minimized.

@jzakrzewski

jzakrzewski Sep 14, 2020
Contributor

I'd prefer to mention it because, as @bagder said, some treat the status line as header line (why???).
If we can be clear on the subject, that we should be.

This comment has been minimized.

@anio

anio Sep 14, 2020
Author Contributor

OK. So, @bagder, what is your opinion?

This comment has been minimized.

@bagder

bagder Sep 14, 2020
Member

Being clear is good, which is why I brought this up to begin with.

This comment has been minimized.

@anio

anio Sep 14, 2020
Author Contributor

OK, please give me a good sentence to add.

This comment has been minimized.

@jzakrzewski

jzakrzewski Sep 14, 2020
Contributor

Maybe like:
"The number of response headers in the most recent request (restarted at each redirect). Note that the status line IS NOT a header."

This comment has been minimized.

@anio

anio Sep 14, 2020
Author Contributor

Thanks! Added.

@anio anio force-pushed the anio:new-writeout-var-header_count branch from a88904e to 62616be Sep 13, 2020
@anio anio changed the title tool_writeout: add new writeout variable, header_count tool_writeout: add new writeout variable, num_headers Sep 13, 2020
@anio anio requested a review from bagder Sep 13, 2020
docs/cmdline-opts/write-out.d Outdated Show resolved Hide resolved
src/tool_writeout_json.c Outdated Show resolved Hide resolved
src/tool_writeout_json.c Outdated Show resolved Hide resolved
src/tool_writeout_json.h Outdated Show resolved Hide resolved
@anio anio force-pushed the anio:new-writeout-var-header_count branch from 62616be to 4711135 Sep 14, 2020
@anio anio requested a review from bagder Sep 14, 2020
@bagder
bagder approved these changes Sep 14, 2020
This variable gives the number of headers.
@anio anio force-pushed the anio:new-writeout-var-header_count branch from 4711135 to 695b59b Sep 14, 2020
@bagder bagder closed this in 0c1e767 Sep 14, 2020
@bagder
Copy link
Member

@bagder bagder commented Sep 14, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.