Skip to content
This repository has been archived by the owner on Apr 24, 2022. It is now read-only.

Api: Add miner_setverbosity(). #1382

Merged

Conversation

StefanOberhumer
Copy link
Collaborator

@StefanOberhumer StefanOberhumer commented Jul 25, 2018

To identify problems on a running instance it is helpful to change the verbosity level without restarting.
This commit allows the change of verbosity via an API call.

if (!getRequestValue("verbosity", verbosity, jRequestParams, false, jResponse))
return;

if (verbosity > 9)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest to check bounds 0 <-> 9 to avoid possible issues in setting negative values.

if (verbosity < 0 || verbosity > 9)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As verbosity is unsigned I think it will never be "< 0" ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I read verbosity as int.

Nevertheless I would put the user in the condition to detect errors.

Setting an unsigned to -1 causes the same unsigned to get the max value

unsigned verbosity;
verbosity = -1;

produces verbosity = 4294967295 without throwing exception.

which of course breaks the constraint of being above 9 but we do not know if user have originally set 4294967295 or any negative number

Would rather set verbosity to int and check the range.
(only a suggestion).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm... we should settle if we interpret the json request as int or unsigned!!!
If you look at miner_setactiveconnection, miner_removeconnection or miner_pausegpu we interpret the index as an unsigned.
Not sure if we should make some specials for miner_setverbosity ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree ... in fact I believe on those methods you mentioned I have made an error not foreseeing this condition.

if (verbosity > 9)
{
jResponse["error"]["code"] = -422;
jResponse["error"]["message"] = "Verbosity out of bounds";
Copy link
Collaborator

Choose a reason for hiding this comment

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

jResponse["error"]["message"] = "Verbosity out of bounds (0-9)";

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 - Updated

@AndreaLanfranchi AndreaLanfranchi dismissed their stale review July 30, 2018 16:22

After a second and third reread of this code I realized that the overload of getRequestValue for unsigned already takes care of invalid input.

@AndreaLanfranchi
Copy link
Collaborator

Sorry for letting this PR pending for so long.

@StefanOberhumer StefanOberhumer merged commit 9dcd6a1 into ethereum-mining:master Jul 31, 2018
@StefanOberhumer StefanOberhumer deleted the ApiAddSetVerbosity branch July 31, 2018 18:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants