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

Be more clear in --help what the value of --log is supposed to be #184

Merged
merged 1 commit into from Feb 18, 2022

Conversation

lgommans
Copy link
Contributor

@lgommans lgommans commented Feb 15, 2022

Currently, --help says:

...
   --listen string        listen address (default ":8000")
   --log string           log HTTP requests in the combined log format
   --max-size int         the maximum size of the repository in bytes
...

I looked through the documentation and even gave the source code a glance to find what this "string" is supposed to be, which options exist? I just want it to dump the URI or something, whatever the syntax is for that in this combined format, why is an argument even required and can't I use some default?

Some %s-like guesses later and not seeing any log messages at all upon doing requests to the server, I moved on and did an ls for unrelated reasons, finding that it had created my attempts as filenames. Aha, it has to be a filename! It doesn't simply log to stdout!

This pull request hopefully makes that more clear for those who come after me. I found it hard to find good phrasing without making it overly long. I'd actually rather replace the word "string" with "filename" (or just "file") and leave the rest as-is, but I'm not sure if that's possible without overcomplicating things.

The proposed change would appear as:

...
   --listen string        listen address (default ":8000")
   --log string           write HTTP requests in the combined log format to the specified filename
   --max-size int         the maximum size of the repository in bytes
...

The --no-verify-upload option is the only one that is still (significantly) longer, but it's also a bit of an exception so that's not a great measure.

(If a changelog entry is needed for this change, just let me now. I've marked it as not applicable in the checklist because it seems too trivial for an entry.)

Checklist

  • I have enabled maintainer edits for this PR
  • n/a I have added tests for all changes in this PR
  • n/a I have added documentation for the changes (in the manual)
  • n/a There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the cleanup.

@MichaelEischer MichaelEischer merged commit cb85fb3 into restic:master Feb 18, 2022
@mirabilos
Copy link

Huh, so a filename is expected… I was wondering what the string was supposed to be…

… but it doesn’t work with a filename either:

error: open /dev/stdout: permission denied

@lgommans
Copy link
Contributor Author

That's a special file, try a regular file that you have permissions to create and write to?

For me, though, it also works with this special /dev "file": rest-server --log /dev/stdout --no-auth --path /tmp/test and then curl localhost:8000 (default port) shows up in the stdout whereas without --log /dev/stdout it does not.

@mirabilos
Copy link

mirabilos commented Apr 17, 2022

@lgommans I need it on stdout though, so it can be combined with the rest of the output into the service manager’s logging (and timestamping and rotating) system (here: DJB dæmontools)

A separate actual file is no option.

(Ideally, they’d allow - for stdout.)

@lgommans
Copy link
Contributor Author

lgommans commented Apr 17, 2022

Might it be that you're not running it from bash or a similar shell and /dev/stdout thus does not exist? I think it's some special file that bash somehow emulates (I'm not sure if I remember this correctly).

If that's the case, maybe run it with bash -c 'rest-server --...', else I don't really have an idea how to solve it. Might be that you have to contribute a patch to rest-server such that - is stdout; or it might be that these djb tools need a patch to work with log files or fifos or so.

(The above occurred to me while I was actually typing that I can't reproduce your issue so I wasn't sure how to further help without further information. If this missing stdout 'device' isn't it, then you'll have to give more info.)

@mirabilos
Copy link

The link does exist, but I suspect that, as soon as stdout is piped to somewhere, the opening fails, because:

l-wx------ 1 resticd resticd 64 Apr 17 23:42 1 -> pipe:[41829]

(from /proc/$pid/fd/)

I guess it opens for more than appending?

@mirabilos
Copy link

Hm, no, it isn’t even that.

nobody@cafe:/ $ /tmp/rest-server --log /dev/stdout --no-auth --path /tmp/test
Data directory: /tmp/test
Authentication disabled
error: open /dev/stdout: permission denied
1|nobody@cafe:/ $ /tmp/rest-server --log /dev/fd/1 --no-auth --path /tmp/test
Data directory: /tmp/test
Authentication disabled
error: open /dev/fd/1: permission denied
1|nobody@cafe:/ $ ls -l /dev/stdout /dev/fd/1
lrwx------ 1 nobody nogroup 64 Apr 17 23:45 /dev/fd/1 -> /dev/pts/0
lrwxrwxrwx 1 root   root     4 Apr 14 00:01 /dev/stdout -> fd/1
nobody@cafe:/ $ ls -l /dev/fd
lrwxrwxrwx 1 root root 13 Apr 14 00:01 /dev/fd -> /proc/self/fd

@lgommans
Copy link
Contributor Author

I guess it opens for more than appending?

--log sets the server.Log option. When searching for use of server.Log, it shows up in mux.go#L95. The next line calls into a function defined earlier in the file and there the next line contains the file open options.

The code base is not that large. I often have trouble navigating established (read: large) projects in languages which I'm not very familiar with, but this project is imo quite accessible so I figured I'd show how I approached this :). At any rate, the call looks like this:

os.OpenFile(s.Log, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)

Since that works fine for me, I'm not sure if/how this helps, though. Maybe you know more about this.

@mirabilos

This comment was marked as off-topic.

@fd0

This comment was marked as off-topic.

@mirabilos
Copy link

--log - was added in #217 (thanks!)

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.

None yet

4 participants