Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Network: support down NIC #130
Network: support down NIC #130
Changes from all commits
bacff83
d836369
b94544b
6a7e0a7
c591c0b
d0336c5
e965e69
f74fd55
be47675
2aef494
aab691f
04f85b0
ad42bf6
e19ff4a
b910e22
757cca1
6d9be7b
f569070
899ed84
a250152
9d0c306
54e8072
2af752c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this should be
len(attack.Duration) != 0
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len(attack.Duration)
has been checked before this function,-1
means attack will run continuously. It will be marked in doc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check this field before executing this attack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field has been checked for necessity before executing,
if len(n.Duration) == 0 { return errors.New("duration is required") }
, the judgement here is only to decide whether to run continuously.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember you add a note that the
attack.Duration
is required, but if users set it to-1
, this attack won't be recovered automatically. This behavior is very dangerous. Can we forbid users to set it to-1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this
-1
option is added on purpose, because I think some user may need this option to let the experiment run continuously, especially those who run NIC down locally rather than remotely. Andattack.Duration
is hardly ever set to negative numbers, I think-1
is enough to be a warning sign (maybe-2333
or-32768
is better).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this way, we need to set the experiment's status to
Destroyed
.I think it's better to refine the
schedule
framework, and let it recover automatically by theschedule
framework.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can refine the
schedule
framework in next prThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this solution is temporary, real
schedule
is need, especially for some dangerous experiments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments to explain why need to trim
\x00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE IT!