Closed tomd1990 closed 5 years ago
I'm created a new PR for this issue, I think merging #257 would have made things less nice in the commit history.
A summary of changes/ discussion
Great. I leaved a few more comments. BTW, there is no need to close a PR if you don't like the commit history: you can force-push.
Thanks for the all of the feedback. I closed it because I thought if I force pushed, I would lose the relevant discussion. I'll keep this in mind next time.
So I've made those changes, and in addition I've changed strncpy like so:
strncpy(name, driver_name, MaxSize);
The reason being is that it shouldn't depend on source string length, I believe this is the reason travis was throwing an error.
If MaxSize happens to be greater than the source string, it will not cause overflow. When it encounters the null char it'll just pad the remaining space with null char, if source string is greater will just take MaxSize - 1 char, and append the null char
The code looks good, but I tested it, and these commands fail:
$ roc-recv -L
roc-recv: '--source' ('-s') option required
$ roc-send -L
roc-send: '--source' ('-s') option required
This is because option "source"
is marked required
in cmdline.ggo
for both tools.
Could you please make them optional
?
roc-send already has necessary port validation so no further action is needed for it. roc-recv doesn't, so we also need to add a check; it would be reasonable to check that at least one port was specified:
if (args.source_given == 0 && args.repair_given == 0) {
roc_log(LogError, "at least one --source or --repair port should be specified");
return 1;
}
Thanks!
Thank you for all of the feedback!
References: #251, #257