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

report write errors if sqlite can not be created #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drewwells
Copy link

@drewwells drewwells commented Nov 26, 2024

Resolve relative paths and attempt create before passing to sqlite

#12

@drewwells
Copy link
Author

drewwells commented Nov 26, 2024

The error was the path I was writing to was read only to the user. The imported library may have a wat to surface the underlying write error. This error was a little vague

Error setup sqlite SqliteFailure(Error { code: CannotOpen, extended_code: 14 }, Some("unable to open database file: speedtest.db"))

@SudoDios
Copy link
Collaborator

SudoDios commented Nov 27, 2024

@drewwells
Thank you 🌹.
Very good, but :

  1. No need to create the file, because sqlite does it itself.
    We just need to create the directories std::fs::create_dir_all
  2. Remove the prints (only write the log when setup encounters a problem).

@drewwells
Copy link
Author

Okay I dug into the library a little bit (asking chatgpt) and came up with this. I wanted to get specific file i/o errors when the config was missing and if the sqlite db was not-writeable. This was the case when I first started using speedtest-rust with my systemd setup

DEMO

sqlite database is not writeable

speedtest[3856781]: [2024-11-27 09:29:43.712 ERROR src/main.rs:59] SQLite permission error: SqliteFailure(Error { code: CannotOpen, extended_code: 14 }, Some("unable to open database file: /var/lib/speedtest/speedtest.db"))
systemd[1]: speedtest.service: Main process exited, code=exited, status=1/FAILURE

config file was missing or directory not readable

speedtest[3851365]: [2024-11-27 09:26:52.884 ERROR src/main.rs:28] Could not load config from speedtest-settings.toml: No such file or directory (os error 2)

@SudoDios
Copy link
Collaborator

ChatGPT doesn't work very well ☺️.
But a few points:

  1. There is still error code 14, when the directory for the database does not exist.
  2. Where the directory is only readable or writable, it should be stated in the error (easier to understand).
  3. There is no need to write the full source path.

@drewwells
Copy link
Author

Looks okay to me, I assume people will figure out the directory doesn't exist based on the path being reported in the error.

[2024-11-28 06:00:06.043 INFO src/config/mod.rs:128] Configs initialized file : configs.toml
[2024-11-28 06:00:06.043 ERROR src/main.rs:56] Error setup sqlite SqliteFailure(Error { code: CannotOpen, extended_code: 14 }, Some("unable to open database file: /home/src/github.com/librespeed/speedtest-rust/boom/speedtest.db"))

What error message were you expecting? I think full path is helpful to debug missing directories. Especially since config doesn't define a root path so they're subject to relative pathing from dockerfile or systemd. I wouldn't be opposed to adding a mkdir in rust to attempt to create a root directory and stateful directories like sqlite

@SudoDios
Copy link
Collaborator

I mean, you don't need to show rec.file().unwrap_or("unknown") and rec.line().unwrap_or(0) in the log.
Step to show error :

  • db directory exist
    • only readable or writable -> show error to user with full db path
    • continue;
  • db directory not exist
    • mkdir
      • failure -> show error to user with full db path
      • success -> continue;

As much as possible, the user should not see the error.

@drewwells
Copy link
Author

drewwells commented Nov 28, 2024

I mean, you don't need to show rec.file().unwrap_or("unknown") and rec.line().unwrap_or(0) in the log. Step to show error :

The error messages are not unique across the codebase, so I did not know which line was throwing the error. It's pretty standard to show the line number. The dev logger I use gives me a full stack trace when I log an error.

  • db directory exist

    • only readable or writable -> show error to user with full db path
    • continue;
  • db directory not exist

    • mkdir

      • failure -> show error to user with full db path
      • success -> continue;

As much as possible, the user should not see the error.

You want to attempt to create the directory before emitting an error? That shouldn't be hard to add.
I can attempt to make the directory even if it exists, ignore already existing errors. The rest of the code calling sqlite will then be the same but at least we attempted to create a missing directory first.

@SudoDios
Copy link
Collaborator

You want to attempt to create the directory before emitting an error? That shouldn't be hard to add.

Yes, it's easy.
I wanted you to complete it.
An error should be when nothing more can be done.

@drewwells
Copy link
Author

DEMO
pointed sqlite to a non-existent directory. It created it and continued on

[2024-11-28 20:19:48.809 INFO src/config/mod.rs:128] Configs initialized file : configs.toml
[2024-11-28 20:19:48.809 INFO src/database/mod.rs:49] Database Sqlite initialized successfully

@SudoDios
Copy link
Collaborator

I have an idea bro.
Undo the edits you made in the /config/mod.rs file (because they are not needed).
and check in sqlite.rs that the directories are not read-only and writable (if they are, the user will be explicitly told in the error).
Okay ?

@drewwells
Copy link
Author

I have an idea bro. Undo the edits you made in the /config/mod.rs file (because they are not needed). and check in sqlite.rs that the directories are not read-only and writable (if they are, the user will be explicitly told in the error). Okay ?

It's rustfmt and two lines to add filename:linenumber to log messages. Both those are useful, I can figure out how to revert rustfmt if you don't want the formatting changes.

    create parent directory, ignoring errors when doing so
@drewwells
Copy link
Author

Updated...

@SudoDios
Copy link
Collaborator

SudoDios commented Dec 2, 2024

@drewwells
This good ☺️.
Should I merge this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants