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

bsmtp bls bextract: fixes for command line parsing #1455

Conversation

alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Apr 25, 2023

Description

This PR makes sure the -h or --mailhost option is not required and can be left out. It can then be set to the SMTPSERVER environment variable, or default localhost.

  • SMTPSERVER can handle fqdn:port/0.0.0.0:port/[::]:port syntaxes
  • debug level -d20 show now from which source the host and port is taken.

This PR also include a small fix to handle --include-list (-i) flag in bextract and bls

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR

@bruno-at-bareos
Copy link
Contributor

It is almost working well except if someone use the SMTPSERVER env var with a :PORT

export SMTPSERVER=smtp.domain.tld:25
bsmtp -f 'Bareos daemon<hostmaster@ioda.net>' -s "bsmtp test without env var" -c ops@domain.tld admin@domain.tld <<< "New test of bsmtp with env var and 
 option -h non required"
bsmtp: tools/bsmtp.cc:407-0 Error unknown mail host "smtp.domain.tld:25": ERR=Name or service not known
bsmtp: tools/bsmtp.cc:410-0 Retrying connection using "localhost".
bsmtp: tools/bsmtp.cc:436-0 Failed to connect to mailhost localhost

using the var without the port works.

I've done this test as if you bsmtp --help indicate this should be possible

-h,--mailhost <mailhost/IPv4_address:port>,<[mailhost/IPv6_address]:port>
    Use mailhost:port as the SMTP server.

Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Looks good for the moment, check my comment about the test with SMTPSERVER:PORT.

@alaaeddineelamri
Copy link
Contributor Author

I've done this test as if you bsmtp --help indicate this should be possible

-h,--mailhost <mailhost/IPv4_address:port>,<[mailhost/IPv6_address]:port>
    Use mailhost:port as the SMTP server.

The help indicates using host:port within the options, but not as the environment variable. That could doable though, but should be documented where providing the environment variable is mentionned.

@bruno-at-bareos bruno-at-bareos self-requested a review May 2, 2023 08:43
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Can we check if we can better handle the default?
Also bsmtp.cc need to have copyright updated.

core/src/tools/bsmtp.cc Outdated Show resolved Hide resolved
core/src/tools/bsmtp.cc Outdated Show resolved Hide resolved
@bruno-at-bareos bruno-at-bareos self-requested a review May 3, 2023 07:59
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

--removed--

Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Good Work, I think we can squash the commit concerning bsmtp in one and the second concerning bextract and bls

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5465-bsmtp-mailhost-default branch from 9009f89 to e94a854 Compare May 3, 2023 08:36
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Thanks a lot!!!

Ready for merge

@pstorz pstorz changed the title bsmtp: make mailhost not required and update docs bsmtp bls bextract: fixes for command line parsing May 11, 2023
alaaeddineelamri and others added 3 commits May 11, 2023 12:37
bsmtp: parse `SMTPSERVER` value


bsmtp: update default mailport handling


bsmtp: get environment variable earlier

renamed `cp` and `mail_port`
@pstorz pstorz force-pushed the dev/alaaeddineelamri/master/s5465-bsmtp-mailhost-default branch from 9ef9817 to 48ab1d8 Compare May 11, 2023 10:37
@pstorz pstorz merged commit b721365 into bareos:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants