Add custom error handling #104

captain-yossarian · 2020-01-09T19:23:13Z

Regarding #73 issue
It is only prototype.
@imsnif Which cases I have to consider in custom error trait?

imsnif · 2020-01-09T22:09:00Z

So, what we're trying to do here is forward all errors to the user and provide useful hints about what to do about the ones we know. Which is currently only the permission error. We also throw a custom error for an unknown interface (because pnet doesn't do it for us).

So essentially, what we'd like to get to is a generic error type that would wrap or mirror the io error, and allow us to slap on our own "unknown interface error".

Makes sense? (sorry if the explanation is not 100%, I'm a little tired and didn't want to keep you stuck until I have more time :) ).

captain-yossarian · 2020-01-11T21:35:11Z

src/os/shared.rs

-        )),
-        Err(e) => Err(e),
+        Ok(_) => {
+            let error = MyError::new(MyErrorKind::PermissionError("Please do something".to_string()));


@imsnif What do you think about this approach? Here we can define our own kind of error along with error message.
GItHub warns about files conflict, but I have no clue how to fix it. There is no conflicts on my local GitBash.

Hey @SerhiiBilyk, the conflicts appear because GitHub merges your work with the current master. To reproduce this locally, you need the latest changes.

You will have to add this repo (as opposed to your fork) as a remote if you didn't do it already. See here for instructions. After that, you can either rebase your branch on top of the current master, or merge master with your branch.

I hope this helps!

captain-yossarian · 2020-01-11T22:07:50Z

@ebroto Thank you very much!

imsnif · 2020-01-14T17:32:00Z

Hey @SerhiiBilyk - is this ready for a review?

captain-yossarian · 2020-01-14T21:37:31Z

Hey @imsnif , yes

imsnif

Hey, good progress! I like the approach - just be sure to place the errors in the right place. :) You can check with the permission error (the most important to us right now) locally as I wrote in the comments.

Otherwise, could we change the name from MyError to something like GetInterfaceError?

imsnif · 2020-01-15T18:11:40Z

src/os/shared.rs

+            let error = MyError::new(MyErrorKind::PermissionError("Please do something".to_string()));
+            Err(error)
+        },
+        Err(e) => Err(MyError::new(MyErrorKind::OtherError(e.to_string()))),


The permission error actually happens here. You can test for it locally by running without sudo and seeing where the error comes from.

captain-yossarian · 2020-01-15T20:47:36Z

@imsnif I've changed error name. It is looks like it work. Could you please check?

imsnif

Hey, thanks for changing the name. Good progress - I left some comments about what needs to be changed. Please let me know if it's unclear and I'll try to explain further.

imsnif · 2020-01-18T00:46:57Z

src/os/shared.rs

-        )),
-        Err(e) => Err(e),
+        Ok(_) => {
+            let error = GetInterfaceError::new(GetInterfaceErrorKind::OtherError("Please do something".to_string()));


I think here we can go with "Unsupported interface type" as it was before

imsnif · 2020-01-18T00:48:04Z

src/os/shared.rs

+            let error = GetInterfaceError::new(GetInterfaceErrorKind::OtherError("Please do something".to_string()));
+            Err(error)
+        },
+        Err(e) => Err(GetInterfaceError::new(GetInterfaceErrorKind::PermissionError("Permission error".to_string()))),


Permission error is only one error type we can get here. Essentially, we would like to forward all of these errors to the user, and if the error is a permission error, display the "use sudo" string below. Does this make sense?

captain-yossarian · 2020-02-04T20:02:08Z

@imsnif could you please check if everything is ok ?

imsnif · 2020-02-05T14:18:06Z

Hey @SerhiiBilyk - correct me if I'm wrong, but I think even after the changes we still send a permission error for whatever error we get, don't we?

captain-yossarian · 2020-02-05T19:38:55Z

@imsnif if you mean this line (shared.rs, 48 line) then - yes. Do I need to check if this error is permission error ?
Maybe I did not understand you. I thought here we can get only permission error.

Could you please highlight those line of code which I have to change ? Thank you for your patient ;)

imsnif · 2020-02-06T15:09:37Z

@imsnif if you mean this line (shared.rs, 48 line) then - yes. Do I need to check if this error is permission error ?

Yes, exactly.

Maybe I did not understand you. I thought here we can get only permission error.

Ah, sorry. I think I wrote that quickly and I wasn't very clear. We want to handle only the permission error (by suggesting to the user to user sudo). All the rest we just want to forward to them.

captain-yossarian · 2020-02-06T19:21:18Z

src/os/shared.rs

+            ErrorKind::PermissionDenied => Err(GetInterfaceError::new(
+                GetInterfaceErrorKind::PermissionError(format!("{}. Try running with sudo.", e)),
+            )),
+            _ => Err(GetInterfaceError::new(


@imsnif I did not find Result error type which will allow to return here simple std::io::Error Err(e)

Do you think we might be able to store its text in the GetInterfaceError you created, so that we can display it to the user?

@imsnif If user will run cargo build without sudo, he will see Permission error (sudo).
If you comment 46-48 lines and run cargo build without sudo, you will see Failed to find any network, because in this case we throw our custom error only in case of Permission Error.

If you want print to user other error Err(e) , we should change this failure::bail!("Failed to find any network interface to listen on.");

Answer on your question: yes.
Check this 35 line of errors.rs

If you think we should change it, let's do that. :)
The final end goal is:

If it's a permission error, the user should get a permission error and have sudo suggested to them.

If there's another error, let's forward it to the user.

If we get several other errors, we can forward all of them to the user. Maybe one per line with the name of the network interface in front of it.

How does that sound?

Sounds good. I will do that

@imsnif done.
I made a small refactor to reduce boilerplate

P.S. Could you please look at this comment ? I did all my best

Hey, this is great! I'm good to go with this, just one little nitpick:
With this, if there is more than 1 interface that has an error, we will bail and show the error for just the first one.
Would you like to fix this, or shall we leave it for next time?

@imsnif I will do it in scope of this PR

Awesome! Just another addition: if there is at least one permission error, let's just show the permission message. I think that would be clearer. Sounds good?

@imsnif Could you please check ?

imsnif · 2020-03-01T11:59:35Z

Hey @captain-yossarian.

First let me start by apologizing for my spotty availability over the past few weeks.

I really liked the changes you made for displaying multiple errors. I also learned about the .. spread syntax which was totally new to me. Thank you for that :)

I made some minor changes, mainly in order to display the network interface name on all errors, and to make sure all the errors we get are displayed in some edge cases (eg. permission errors on multiple interfaces).

In order to find these edge cases, I mostly just started hacking away at the get_datalink_channel method in os/shared.rs. By making it return different error types (eg. one permission error and one "other" error, two permission errors, two "other" errors, etc.) I could see how we behave in those situations and which errors we display.

I found, for example, that if we get a permission error, we get a nice explanation for it, but then afterwards we just get the printed errors from the interfaces (sometimes without the interface names). I thought that output could be friendlier, so I added an "Additional Errors" display.

Thank you for all your work on this, and your persistence even when I wasn't 100% available to assist. I hope to see more contributions from you in the future.

captain-yossarian · 2020-03-01T16:43:53Z

@imsnif Hi, thank you for your feedback. I have learn a lot from your codebase! This is my first Rust PR. So thank you very much for your patient and guidance.
I'm definitely will contribute to your project in the future!

captain-yossarian added 2 commits January 9, 2020 20:05

Initial

ca64e61

Add errors file

42bac89

captain-yossarian added 7 commits January 11, 2020 19:23

Update

a9756b3

Add error message

e3179a4

Update

4f83ab8

Add kind method

3602c2e

Update error kind

ebfb91a

Update get_datalink_channel

914f710

Update unused var

a19173b

captain-yossarian commented Jan 11, 2020

View reviewed changes

captain-yossarian added 9 commits January 12, 2020 13:35

Initial

bc44d13

Add errors file

81027b8

Update

989314f

Add error message

90fc5dc

Update

cc396c8

Add kind method

1737164

Update error kind

9545879

Resolve conflicts

29db326

Update error message

c7f4bdc

captain-yossarian changed the title ~~[WIP]Add custom error handling~~ Add custom error handling Jan 13, 2020

imsnif requested changes Jan 15, 2020

View reviewed changes

Update error name

594a216

imsnif requested changes Jan 18, 2020

View reviewed changes

captain-yossarian added 2 commits January 18, 2020 16:39

Update error messages

27f609b

Update message

899d71d

captain-yossarian added 4 commits February 4, 2020 20:04

merge

7378afc

format

31a080e

format2

d2e1270

update

e03975e

Update error checking

3020a6f

captain-yossarian commented Feb 6, 2020

View reviewed changes

captain-yossarian and others added 16 commits February 6, 2020 20:30

format

889fbf4

refactor

7136bd4

reduce message

8d452e9

remove unused code

8f65f00

fix lint

d39be34

wip

8d6c8db

add multiline

d656f6d

add handle_errors

015a7ef

update fold

3a1b72e

update

9365479

clippy

101dd25

Update

099785e

spaces

5994e2b

merge

594f67c

fix(error-handling): display interface names in errors

a8b732b

style(format): rustfmt

ce1d396

imsnif approved these changes Mar 1, 2020

View reviewed changes

imsnif merged commit c45b3c8 into imsnif:master Mar 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom error handling #104

Add custom error handling #104

captain-yossarian commented Jan 9, 2020 •

edited

Loading

imsnif commented Jan 9, 2020

captain-yossarian Jan 11, 2020 •

edited

Loading

ebroto Jan 11, 2020 •

edited

Loading

captain-yossarian commented Jan 11, 2020

imsnif commented Jan 14, 2020

captain-yossarian commented Jan 14, 2020

imsnif left a comment

imsnif Jan 15, 2020

captain-yossarian commented Jan 15, 2020

imsnif left a comment

imsnif Jan 18, 2020

imsnif Jan 18, 2020

captain-yossarian commented Feb 4, 2020

imsnif commented Feb 5, 2020

captain-yossarian commented Feb 5, 2020

imsnif commented Feb 6, 2020 •

edited

Loading

captain-yossarian Feb 6, 2020

imsnif Feb 6, 2020

captain-yossarian Feb 6, 2020

imsnif Feb 6, 2020

captain-yossarian Feb 7, 2020

captain-yossarian Feb 7, 2020 •

edited

Loading

imsnif Feb 11, 2020

captain-yossarian Feb 12, 2020

imsnif Feb 12, 2020

captain-yossarian Feb 14, 2020

imsnif commented Mar 1, 2020

captain-yossarian commented Mar 1, 2020

Add custom error handling #104

Add custom error handling #104

Conversation

captain-yossarian commented Jan 9, 2020 • edited Loading

imsnif commented Jan 9, 2020

captain-yossarian Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

ebroto Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

captain-yossarian commented Jan 11, 2020

imsnif commented Jan 14, 2020

captain-yossarian commented Jan 14, 2020

imsnif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian commented Jan 15, 2020

imsnif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian commented Feb 4, 2020

imsnif commented Feb 5, 2020

captain-yossarian commented Feb 5, 2020

imsnif commented Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsnif commented Mar 1, 2020

captain-yossarian commented Mar 1, 2020

captain-yossarian commented Jan 9, 2020 •

edited

Loading

captain-yossarian Jan 11, 2020 •

edited

Loading

ebroto Jan 11, 2020 •

edited

Loading

imsnif commented Feb 6, 2020 •

edited

Loading

captain-yossarian Feb 7, 2020 •

edited

Loading