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

send details=1 for get_report #1640

Merged
merged 4 commits into from
Sep 24, 2019

Conversation

sarahd93
Copy link
Contributor

Adjust gsa to send details=1 for get_report and change gsad to forward details to gvmd.

Checklist:

@sarahd93 sarahd93 marked this pull request as ready for review September 23, 2019 15:45
Copy link
Member

@timopollmeier timopollmeier left a comment

Choose a reason for hiding this comment

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

Overall the C part looks okay to me, although you should keep in mind that the params_value_bool function may not always behave intuitively:

If the parameter is missing or invalid the return value will default to 0, but for empty strings it will be 1.

If it doesn't conflict with any other uses it might make sense to change this behavior eventually.

@swaterkamp
Copy link
Member

We should change the type to whatever is used in the other "Details" params then.

Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

I find it very strange that get_report didn't support details=1 yet and that get_report in gsad is not using the gmp_arguments_t yet.

@sarahd93
Copy link
Contributor Author

I looked into the gsad code again and all the other times details params are used it is together with the gmp_arguments_t. I am not comfortable working with that, though, I feel like any further changes I could try to make now would involve too much guessing on my part, because I do not know enough about C.
Should we just merge the PR like it is for now so at least the report details page will work again?

@bjoernricks
Copy link
Contributor

I am fine with merging it

@bjoernricks bjoernricks merged commit 14805b3 into greenbone:master Sep 24, 2019
@sarahd93 sarahd93 deleted the fix_get_report_master branch September 24, 2019 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants