While it doesn't hurt, it's unnecessary since the big protobuf
modernisation, that also introduced types separate from the generated
ones for internal use. Those fields are already dropped when converting
to the wire in protocol.
In practice we already always call SetPassword and ClusterConfig
together. However it's not just "sensible" to do that, it's required: If
the passwords change, the remote device needs to know about that to
check that the enc. setup is valid/consistent (e.g. tokens match,
folder-type is appropriate, ...).
And with the passwords set later, there's no point in adding them as
part of creating a new connection.
This is a "followup" (if one can call it that 4 years later :) ) to
resp. fix for the following commit:
924b96856f
Co-authored-by: Jakob Borg <jakob@kastelo.net>
Flattened the copier code more. Also removing and moving some
parameters/return values to simplify things. Generally rely less on
return values, e.g. by handling errors right away and using `state` to
do the right thing (e.g. abort on failure).
Supposed to be a refactor without any behaviour changes, except for
fixing a tiny regression on folder order: We used to try copying from
the same folder first, but lost that property at some point (also sent a
PR fixing only that, I'd merge that first making this refactor only).
Where `folderFilesystems` and `folders` is built, there's a comment
spelling out the purpose: To have the same folder first, as that's the
most likely to get hits. Plus a copy is possibly more efficient than
from another folder, e.g. if that's on a different filesystem. We lost
that behaviour during some unrelated change.
(Also sneaking in a comment fix on yesterdays change.)
This is a draft because I haven't adjusted all the tests yet, I'd like
to get feedback on the change overall first, before spending time on
that.
In my opinion the main win of this change is in it's lower complexity
resp. fewer moving parts. It should also be faster as it only does one
query instead of two, but I have no idea if that's practically
relevant.
This also mirrors the v1 DB, where a block map key had the name
appended. Not that this is an argument for the change, it was mostly
reassuring me that I might not be missing something key here
conceptually (I might still be of course, please tell me :) ).
And the change isn't mainly intrinsically motivated, instead it came
up while fixing a bug in the copier. And the nested nature of that code
makes the fix harder, and "un-nesting" it required me to understand
what's happening. This change fell out of that.
Currently we log on every single one of 10 retries deep in the upnp
stack. However we also return the failure as an error, which is bubbled
up a while until it's logged at debug level. Switch that around, such
that the repeat logging happens at debug level but the top-level happens
at info. There's some chance that this will newly log errors from
nat-pmp that were previously hidden in debug level - I hope those are
useful and not too numerous.
Also potentially this can even close#9324, my (very limited)
understanding of the reports/discussion there is that there's likely no
problem with syncthing beyond the excessive logging, it's some weird
router behaviour.
These CSS overrides address issues that are already present on wider
screens, so apply it there. Some experiments show we might even want to
up the limit more, but I am chicken and lazy, so I propose to use the
existing 470px media block.
Supersedes another PR after not getting any reaction to feedback there:
https://github.com/syncthing/syncthing/pull/9591#issuecomment-2212586134
Co-authored-by: Jakob Borg <jakob@kastelo.net>
In ignores, normalize the input when parsing it.
When scanning, normalize earlier such that the path is already
normalized when checking ignores. This requires splitting normalization
of the string from normalization of the file, as we don't want to
attempt the latter if the file is ignored.
Closes#9598
---------
Co-authored-by: Jakob Borg <jakob@kastelo.net>
Reasoning in comments. The main motivation is to avoid all the case
checks when walking the filesystem.
"again" as we already tried once, but it caused a major issue ragarding
mtimefs layer. The root of this problem has been fixed in the meantime
in ac8b3342a
The read/write loops may keep going for a while on a closing connection
with lots of read/write activity, as it's random which select case is
chosen. And if the connection is slow or even broken, a single
read/write
can take a long time/until timeout. Add initial non-blocking selects
with only the cases relevant to closing, to prioritize those.
This would have addressed a recent issue that arose when re-ordering our
"filesystem layers". Specifically moving the caseFilesystem to the
outermost layer. The previous cache included the filesystem, and as such
all the layers below. This isn't desirable (to put it mildly), as you
can create different variants of filesystems with different layers for
the same path and options. Concretely this did happen with the mtime
layer, which isn't always present. A test for the mtime related breakage
was added in #9687, and I intend to redo the caseFilesystem reordering
after this.
Ref: #9677
Followup to: #9687
The test is quite odd and specific, but it does reproduce the issue that
caused #9677, so I'd propose to add it to have a simple regression test
for the basic scenario. Also the option to the fakefs might come handy
for other scenarios where you want to quickly test some behaviour on a
filesystem without nanosecond precision, without actually needing access
to one.
Tiny cleanup I noticed while trying to fix/test another issue
(https://github.com/syncthing/syncthing/pull/9600). I shortly tried to
figure out what it was used for in the past, but gave up without
results.
Previously we queried cache with backslashes, and stored entries with
slashes. As in no cache hits ever for non-toplevel files. I also
eventually remembered that cache is disabled by default, so this is a
bit pointless, but still right :P
Before introducing the service map and using it for folder runners, the
entries in folderCfgs and folderRunners for the same key/folder were
removed under a single lock. Stopping the folder happens separately
before that with just the read lock. Now with the service map stopping
the folder and removing it from the map is a single operation. And that
still happens with just a read-lock. However even with a full lock it’s
still problematic: After the folder stopped, the runner isn’t present
anymore while the folder-config still is and sais the folder isn't
paused.
The index handler in turn looks at the folder config that is not paused,
thus assumes the runner has to be present -> nil deref on the runner.
A better solution might be to push most of these fmut maps into the
folder - they anyway are needed in there. Then there's just a single
map/source of info that's necessarily consistent. That's quite a bit of
work though, and probably/likely there will be corner cases there too.
We usually want to ensure that our own device is present. However if the
given device ID is the empty ID, we shouldn't do that. This is a
legimate (though way too non-obvious) use-case when opening the config
without knowing/caring about the device ID.
In the original fix in #8563 I simply forgot this. Which meant #8556
wasn't actually fixed, as the trialer size would have been 0 (default),
and thus we would have still sent the inflated size to encrypted peers.
lib/model: Fix file size inconsisency due to enc. trailer
Fixes a regression due to PR #8563, while arguable the bug was actually
introduced in a much older PR #7155, but didn't have any bad effects so
far:
We account for the encryption trailer in the db updater routine,
calculating the file-info size there. However there's no guarantee that
the file-info at this point is still the exact same as when it was
written. It was before, but isn't anymore since introducing the new
EncryptedTrailerSize field.
Fix: Adjust the size in the info at the same place where the trailer is
written, i.e. we definitely have the actual size on disk.
Simon Frei
