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

new command line option to customize the location of transcode logs #117

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

irsl
Copy link
Contributor

@irsl irsl commented Apr 1, 2023

This change also addresses the issue raised in #113, when user lookup fails in a docker container, logging will be silently turned off (instead of panicing).
Tested with the following dynamic stream:

"Command": "bash -c 'echo stdout; >&2 echo stderr'"

Some earlier formatting mistakes (of mine) should also be fixed in README.rst.

@anacrolix
Copy link
Owner

Could you move all the path resolution stuff to the earliest point possible? That includes stderrPath and TranscodeLogPath. That way you can handle errors resolving the user home etc. where it's suitable too. Also the special casing for /dev isn't ideal, is there someway to improve or remove that? I would rather another flag or something for that.

@irsl
Copy link
Contributor Author

irsl commented Apr 2, 2023

How about calling it -transcodeLogPattern that would support a [tsName] placeholder to be substituted in serveDLNATranscode?
This aproach would fit well the following use cases:

  • the original dedault behavior logging into separate files ($HOME/.dms/log/[tsname]) one for each video
  • logging to the same file
  • logging turned off (/dev/null)
  • logging to stderr (/dev/stderr)

The last one is useful in a container environment where the container runtime takes care about log rotation/retention.

@anacrolix
Copy link
Owner

That sounds fair. Can you do that with resolving it early too? Expose the necessary configuration fields wherever they're used and compute their values after flag parsing rather than on on demand for each log site.

@irsl
Copy link
Contributor Author

irsl commented Apr 2, 2023

Yup, that will require adding a new user in Dockerfile - otherwise early resolution would break it, even if transcoding is never used. I will take care of that.

This change also addresses the issue raised in anacrolix#113,
when user lookup fails in a docker container, logging will be silently
turned off (instead of panicing).
Tested with the following dynamic stream:

"Command": "bash -c 'echo stdout; >&2 echo stderr'"

Some earlier formatting mistakes (of mine) should also be fixed in README.rst.
@irsl
Copy link
Contributor Author

irsl commented Apr 2, 2023

Refactored as discussed. TDD of the early error before the Dockerfile is adjusted:

root@a798962a645e:/data/fileserver-geza/dms# docker run --rm -it dmsx
2023-04-02T19:14:55+0000 NIL [main.getDefaultFFprobeCachePath:83]: user: unknown userid 1000
2023/04/02 19:14:55 error in main: unable to resolve current user: "user: unknown userid 1000"

@anacrolix anacrolix merged commit b137217 into anacrolix:master Apr 3, 2023
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