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
Contributor

dfandrich commented Sep 9, 2020 via email

@anio
Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need these braces.

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

value = memchr(ptr, ':', cb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I don't know what you mean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still something you can change :-)

@anio
Copy link
Contributor Author

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 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 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 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 new-writeout-var-header_count branch from c2d3858 to a88904e Compare September 11, 2020 13:17
@ghost
Copy link

ghost 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 September 12, 2020 09:31
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, please give me a good sentence to add.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added.

@anio anio force-pushed the new-writeout-var-header_count branch from a88904e to 62616be Compare September 13, 2020 17:08
@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 September 13, 2020 17:09
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 new-writeout-var-header_count branch from 62616be to 4711135 Compare September 14, 2020 09:53
@anio anio requested a review from bagder September 14, 2020 10:02
This variable gives the number of headers.
@anio anio force-pushed the new-writeout-var-header_count branch from 4711135 to 695b59b Compare September 14, 2020 12:52
@bagder bagder closed this in 0c1e767 Sep 14, 2020
@bagder
Copy link
Member

bagder commented Sep 14, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants