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

[bug] unexpected panic when specifying --directory of a tmpfs #11

Closed
brandon1024 opened this issue Apr 16, 2024 · 3 comments
Closed

[bug] unexpected panic when specifying --directory of a tmpfs #11

brandon1024 opened this issue Apr 16, 2024 · 3 comments

Comments

@brandon1024
Copy link

Issue Description

When specifying a tmpfs location as an argument to --directory, the application panics unexpectedly.

$ atac -d /tmp
parsing: /tmp/mat-debug-91998.log
thread 'main' panicked at src/app/startup/startup.rs:55:17:
unhandled file type
stack backtrace:
   0:     0x55b7dbc36f86 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h410d4c66be4e37f9
   1:     0x55b7dbc64e40 - core::fmt::write::he40921d4802ce2ac
   2:     0x55b7dbc3355f - std::io::Write::write_fmt::h5de5a4e7037c9b20
   3:     0x55b7dbc36d64 - std::sys_common::backtrace::print::h11c067a88e3bdb22
   4:     0x55b7dbc38797 - std::panicking::default_hook::{{closure}}::h8c832ecb03fde8ea
   5:     0x55b7dbc384f9 - std::panicking::default_hook::h1633e272b4150cf3
   6:     0x55b7dbc38c28 - std::panicking::rust_panic_with_hook::hb164d19c0c1e71d4
   7:     0x55b7dbc38ac9 - std::panicking::begin_panic_handler::{{closure}}::h0369088c533c20e9
   8:     0x55b7dbc37486 - std::sys_common::backtrace::__rust_end_short_backtrace::hc11d910daf35ac2e
   9:     0x55b7dbc38854 - rust_begin_unwind
  10:     0x55b7db6a8d85 - core::panicking::panic_fmt::ha6effc2775a0749c
  11:     0x55b7db6d177c - atac::app::startup::startup::<impl atac::app::app::App>::startup::hb0dbb4ca5eb46b2f
  12:     0x55b7db7742b1 - tokio::runtime::park::CachedParkThread::block_on::he0f8c1c457bbf1b1
  13:     0x55b7db75039a - tokio::runtime::context::runtime::enter_runtime::hf17d553f5e0f270a
  14:     0x55b7db78297e - tokio::runtime::runtime::Runtime::block_on::h54ffd2db751dfad6
  15:     0x55b7db7570d9 - atac::main::h26d3a8c116b6fb46
  16:     0x55b7db73e903 - std::sys_common::backtrace::__rust_begin_short_backtrace::hea90534575b9fc13
  17:     0x55b7db79200d - std::rt::lang_start::{{closure}}::h5c94db54965fe6bb
  18:     0x55b7dbc2d491 - std::rt::lang_start_internal::h4d236095b69a230b
  19:     0x55b7db7571b5 - main
  20:     0x7f2819dce24a - __libc_start_call_main
                               at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  21:     0x7f2819dce305 - __libc_start_main_impl
                               at ./csu/../csu/libc-start.c:360:3
  22:     0x55b7db6a95d5 - _start
  23:                0x0 - <unknown>

I haven't dug any deeper. It should be easily reproducible.

Looks like it should be a quick fix. Cool project! :-)

Merci!

@Julien-cpsn
Copy link
Owner

Julien-cpsn commented Apr 16, 2024

Hi!

The error says unhandled file type which is done "on purpose" I would say.
Actually, you cannot use the application on directory that have other files than the ATAC ones (config atac.toml, log atac.log and collections my_collection.json).

TL;DR: Start with an empty directory and then it will be "atac-only".

If you think this was a bad idea, it can change without problem :)

Merci également

@brandon1024
Copy link
Author

In that case, perhaps a better error message would help. Additionally it's not stated anywhere in the documentation the requirements for this directory.

I'll be honest, I do find this behaviour a bit strange (and concerning). Correct me if I'm wrong, but I get the impression that atac reads everything in the config directory at startup to configure itself. I worry a bit about this; reading and parsing arbitrary files can be potentially unsafe.

I won't push the issue further though :-) It's your call if you'd like to keep existing behaviour or make changes, I'm only trying to help!

Additional Thoughts

If it were me, I would organize atac.toml such that paths to atac.log and collections are defined there, and only those files listed there are read at startup, instead of the current behaviour of reading everything in that directory.

[core]
    log-path = "atac.log"

[collections]
    my_collection = "my_collection.json"

This has the added benefit that you can separate configuration from runtime files: it's pretty common to see applications use configuration files in the user's $HOME directory or $XDG_CONFIG_HOME, with runtime files and other configuration files located in $XDG_DATA_HOME/$XDG_CONFIG_HOME.

If users could put atac.toml in their home directory, the command line option --directory <dir> would be no longer strictly necessary.

I digress 😅

@Julien-cpsn
Copy link
Owner

Julien-cpsn commented Apr 16, 2024

In that case, perhaps a better error message would help. Additionally it's not stated anywhere in the documentation the requirements for this directory.

I'll be honest, I do find this behaviour a bit strange (and concerning). Correct me if I'm wrong, but I get the impression that atac reads everything in the config directory at startup to configure itself. I worry a bit about this; reading and parsing arbitrary files can be potentially unsafe.

I won't push the issue further though :-) It's your call if you'd like to keep existing behaviour or make changes, I'm only trying to help!

It may seem unclear until you read the code, the application is not parsing anything that is not handleable. So no worries about that. I removed the panic when comming to a non-handleable file, it will just ignore it.

Additional Thoughts

If it were me, I would organize atac.toml such that paths to atac.log and collections are defined there, and only those files listed there are read at startup, instead of the current behaviour of reading everything in that directory.

[core]
    log-path = "atac.log"

[collections]
    my_collection = "my_collection.json"

This has the added benefit that you can separate configuration from runtime files: it's pretty common to see applications use configuration files in the user's $HOME directory or $XDG_CONFIG_HOME, with runtime files and other configuration files located in $XDG_DATA_HOME/$XDG_CONFIG_HOME.

If users could put atac.toml in their home directory, the command line option --directory <dir> would be no longer strictly necessary.

Yes it is planned that the config file (atac.toml) will host more of those things (e.g. log file path). However, listing the collections in the config file is not part of the main philosophy. But I'll keep that in mind, it's an interesting point of view.

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

No branches or pull requests

2 participants