| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| | |
|
| | |
|
| |
|
|
| |
Code motion and import fixups.
|
| |
|
|
|
|
|
| |
We put this in cfg.rs, rather than (say) socks.rs, because it has
config relating to both socks.rs and dns.rs.
Code motion and import fixups.
|
| |
|
|
| |
Code motion and import fixups.
|
| |
|
|
| |
Code motion and import fixups.
|
| |\
| |
| |
| |
| | |
Disable clippy::clone_on_ref_ptr
See merge request tpo/core/arti!352
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
This lint is IMO inherently ill-conceived.
I have looked for the reasons why this might be thought to be a good
idea and there were basically two (and they are sort of contradictory):
I. "Calling ‘.clone()` on an Rc, Arc, or Weak can obscure the fact
that only the pointer is being cloned, not the underlying data."
This is the wording from
https://rust-lang.github.io/rust-clippy/v0.0.212/#clone_on_ref_ptr
It is a bit terse; we are left to infer why it is a bad idea to
obscure this fact. It seems to me that if it is bad to obscure some
fact, that must be because the fact is a hazard. But why would it be
a hazard to not copy the underlying data ?
In other languages, faliing to copy the underlying data is a serious
correctness hazard. There is a whose class of bugs where things were
not copied, and then mutated and/or reused in multiple places in ways
that were not what the programmer intended. In my experience, this is
a very common bug when writing Python and Javascript. I'm told it's
common in golang too.
But in Rust this bug is much much harder to write. The data inside an
Arc is immutable. To have this bug you'd have use interior mutability
- ie mess around with Mutex or RefCell. That provides a good barrier
to these kind of accidents.
II. "The reason for writing Rc::clone and Arc::clone [is] to make it
clear that only the pointer is being cloned, as opposed to the
underlying data. The former is always fast, while the latter can
be very expensive depending on what is being cloned."
This is the reasoning found here
https://github.com/rust-lang/rust-clippy/issues/2048
This is saying that *not* using Arc::clone is hazardous.
Specifically, that a deep clone is a performance hazard.
But for this argument, the lint is precisely backwards. It's linting
the "good" case and asking for it to be written in a more explicit
way; while the supposedly bad case can be written conveniently.
Also, many objects (in our codebase, and in all the libraries we use)
that are Clone are in fact simply handles. They contain Arc(s) (or
similar) and are cheap to clone. Indeed, that is the usual case.
It does not make sense to distinguish in the syntax we use to clone
such a handle, whether the handle is a transparent Arc, or an opaque
struct containing one or more other handles.
Forcing Arc::clone to be written as such makes for code churn when a
type is changed from Arc<Something> to Something: Clone, or vice
versa.
|
| | | |
|
| |/ |
|
| |
|
|
| |
Closes #338.
|
| | |
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
| |
I'm slightly concerned about whether this is behavior people would
expect to have on-by-default, so let's make this off-by-default for
now.
Maybe the `application` and `system` sections should merge?
|
| |
|
|
| |
Closes #270
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This is by no means our final API, but should represent an
improvement. Here instead of having to specify a list of files and
their is-this-optional status, along with a list of command-line
options, we have a single structure that encapsulates all of that
information.
Two advantages here:
- Callers no longer have to remember what the boolean means.
- We can "reload" more easily, by keeping the source object around.
This change also implements the correct behavior for our default
configuration file in `arti::main`: if the file is absent and the
user doesn't list a config file, that's no problem. But if the user
lists _that very same config file, we should insist that it be
present.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Previously we could only configure one global tracing filter that
applied to stdout and journald. There was no support for log files,
either.
This patch fixes both issues, by substantially revising the
configuration format: There are now separate filters for each log
file, for journald, and for the console log. Because we want to
allow multiple logfiles, they have to go into an array in the
configuration.
The configuration logic has grown a bit complicated in its types,
since the tracing_subscriber crate would prefer to have the complete
structure of tracing Layers known statically. That's fine when you
know how many you have, and which kinds there will be, but for
the runtime-configuration case we need to mess around with
`Box<dyn Layer ...>`.
I also had to switch from tracing_subscriber's EnvFilter to its
Targets filter. It seems "EnvFilter" can only be applied as a Layer
in itself, and won't work as a Filter on an individual Layer.
Closes #166.
Closes #170.
|
| | |
|
| |\ |
|
| | |
| |
| |
| |
| |
| |
| |
| | |
This commit implements the "metabuilder" pattern and the "builder
reconstruction" pattern for the ArtiConfig type.
I'm not 100% that this will be necessary, but it will certainly help
with testing.
|
| |/ |
|
| | |
|
| |
|
|
|
|
|
|
| |
Since these shell-variables are hardwired to use org.torproject.Arti as
the program name, it isn't appropriate to call them "app-specific".
If we someday reinstate APP_FOO, it should be based on a user-provided
application name.
|
| | |
|
| |
|
|
| |
Now there are no options that aren't in a toml section.
|
|
|
To do this at all neatly, I had to split out `tor-config` from
`arti-config` again, and putting the lower level stuff (paths,
builder errors) into tor-config. I also changed our use of
derive_builder to always use a common error type, to avoid
error type proliferation.
|
