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

Customize the report file location #291

Closed
1 task done
Digitalone1 opened this issue Jul 2, 2023 · 21 comments · Fixed by #473
Closed
1 task done

Customize the report file location #291

Digitalone1 opened this issue Jul 2, 2023 · 21 comments · Fixed by #473
Assignees
Labels
enhancement New feature, request, or improvement
Milestone

Comments

@Digitalone1
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues.

Describe the solution you'd like

Please let the report file location to be modified by the user choosing a different path.

Is your feature request related to a problem?

The file is constantly updated and people with SSD may want to reduce disk writes. I'd like to change the location to /tmp on Linux (the RAM).

@GyulyVGC GyulyVGC added the enhancement New feature, request, or improvement label Jul 2, 2023
@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 2, 2023

Your request is actually meaningful in case of SSDs...
Now, Sniffnet uses confy to automatically determine a file location based on your OS.
While I agree with your request, I'd also like to keep the UI simple... so what do you think about introducing this option via a CLI argument? If not specified, the location will be that chosen by confy.

@Digitalone1
Copy link
Contributor Author

so what do you think about introducing this option via a CLI argument? If not specified, the location will be that chosen by confy.

It's OK, but there's all that empty space in the main view (I can't even make the window smaller than the size of the screenshot):

Schermata del 2023-07-02 12-19-40

You can place an input selection of the file location above the start button. If you struggle to show a file dialog for the different OS, just a string input could be good (maybe with a button to reset to default path).

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 2, 2023

I will soon introduce an option to set a port filter in that space.
Moreover, I think that a text input to insert manually the full path of a directory would go against the simplicity on which Sniffnet is based.
If you are on Linux as you said, you'll be easily able to create a custom .desktop file (or modify the existing one) including the CLI option in the invocation of /usr/bin/sniffnet
Finally, I think it's not so common for a user to change that directory, so I think it's not a good idea to place it where everyone can see it.

@Digitalone1
Copy link
Contributor Author

It's not the best, but okay.

Can I know why the file has to be updated constantly? Shouldn't it be better to save it once when the app is closed? Or when the report is opened (since I can't see the updates after I loaded it in the text editor)...

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 2, 2023

The report is currently updated once per second, for two reasons:

  • the updates are performed just on the changed entries, and performing more frequent updates ensures that a smaller time frame is used for every update (thus not holding the lock for an amount of time that could cause the loss of some packets)
  • updating the report only when the button is clicked is a possible solution, but as you pointed out it should also happen when the app is quit, and this is something that I have to better investigate (since the app can also be closed via ctrl+C from terminal... and in that case the exit is not controllable)

@Digitalone1
Copy link
Contributor Author

  • updating the report only when the button is clicked is a possible solution, but as you pointed out it should also happen when the app is quit, and this is something that I have to better investigate (since the app can also be closed via ctrl+C from terminal... and in that case the exit is not controllable)

Updating only when the report is opened or when the app is closed is the best solution and I think it won't even need to change the location (the need to change the location is to use the RAM).

I don't know how it works on Rust, but GTK has a close event where you can perform some operations before the UI is shut down. I suppose there should be something similar on Rust to perform the saving to the disk and wait for its completion, then shut down the whole process.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 2, 2023

You may be right.
Iced (the GUI library I use) has a similar event... I will dig into it and see if it works in every case.
Thanks for the suggestions!

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 3, 2023

Yesterday I've tried the CloseRequested Event but it triggers only when the app is closed through the 'x' button of the window system.
It works when closing the app with ctrl+Q, since I personally created that shortcut for the app, but doesn't trigger when closing the app with ctrl+c from the terminal, since that event is intended as a brute shutdown and not a graceful one.
Furthermore, it doesn't trigger when closing the app with cmd+Q on macOS since they set the event to the macOS default (quit the app not in a graceful way).

@Digitalone1
Copy link
Contributor Author

It seems a limitation of the iced. I tested an app which I contribute written in GTK and ctrl+c does perform all the intended tasks before the shut down. I don't even think ctrl+c is supposed to quit in an ungraceful way.

It remains the command line option, but I don't think it's the best solution. Running from the command line only to avoid the app to write constantly on the disk is not very good.

Whichever solution you come up, thanks for the app anyway, it's useful.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 3, 2023

Yeah... it's not the best solution, I'll try to figure something out.
I could also reduce the frequency of file writes (e.g. every 5 seconds), but even that would not be a scalable solution.

@Digitalone1
Copy link
Contributor Author

If it's a iced limitation, you could open an issue on their Github repo, asking for a workaround or extending the close event also to ctrl+c/cmd+Q shortcuts.

If you want, I can open it.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 4, 2023

Check iced-rs/iced#195 before.
I let them know yesterday about this issue.
Yeah, you can definitely open a dedicated issue if you want to.

@Digitalone1
Copy link
Contributor Author

@GyulyVGC Did it. See above.

@Digitalone1
Copy link
Contributor Author

I'm afraid this iced limitation on the closing event won't ever be solved.

While I still think that constantly writing stats on the disk is a bad design choice, could you please add a command line option like suggested in the first reply? At least we have a workaround to write on the /tmp. Thanks.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 14, 2023

Yeah... I also think that waiting for iced is not the best thing to do here.

I didn't include such a option in the new release because the next version will pretty much change everything about output report management: I'd like to introduce support for read/write of PCAP files and remove the current report (but it will take time).

Do you need such a feature in the short term?
If this is the case, I can bundle a dedicated version of Sniffnet just for you.

@Digitalone1
Copy link
Contributor Author

Do you need such a feature in the short term? If this is the case, I can bundle a dedicated version of Sniffnet just for you.

No, I'm not in a hurry. But if you will release the 1.2.3 with a CLI option to change the report location, it would be very appreciated.

Then you can release the new system when it will be ready in a new major release, maybe 1.3.0.

@GyulyVGC
Copy link
Owner

A version 1.2.3 is not planned, but what I can do is the following: I will share with you a modified .deb (is it the format you need, right?).
Just let me know if you are interested in the output report and in this case let me know the exact path you want it to be.
Otherwise, I can disable the output report creation completely.

For v1.3.0, I had in mind to create an advanced settings page to set the path of the new pcap format report.

@Digitalone1
Copy link
Contributor Author

No need to do this. If a new version is not planned, just ignore it.

@GyulyVGC
Copy link
Owner

I will take care of informing you and closing this issue once v1.3 is published.
This could take some time, since as described in the ROADMAP I'm no longer able to work full-time on Sniffnet.

@GyulyVGC GyulyVGC added this to the v1.3.0 milestone Aug 23, 2023
@GyulyVGC GyulyVGC self-assigned this Dec 12, 2023
@GyulyVGC
Copy link
Owner

Many progresses have been made.
In #365 I've temporarily disabled the writes on the old textual file.
That feature will be replaced by (optional) PCAP file writes to a custom path.

In #420 I'm also adding support for process interrupts handling, so at this point the only missing graceful shutdown to handle is when cmd+Q is pressed on macOS, but iced-rs/iced#1941 is in stale.

@Digitalone1
Copy link
Contributor Author

Thanks @GyulyVGC. I'll wait the next stable release.

@GyulyVGC GyulyVGC linked a pull request Mar 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, request, or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants