| Message ID | 4f4f0ed0ce61588137a3e1e9ba96cfb190a2cd75.1699124200.git.mirai@makinata.eu |
|---|---|
| State | New |
| Headers | show |
| Series | Dovecot service refactor. | expand |
Guix pull fails with the [PATCH 4/4] commit. It could be because 'ssl? #t' is still used for the inet-listeners in the default dovecot-configuration. By the way, I see that the "required" value can be used for the ssl dovecot core setting, but for the inet-listener this is not specified: https://doc.dovecot.org/configuration_manual/service_configuration/?highlight=inet_listener#ssl. I think changing the ssl? option type from boolean to string should be done for all boolean valued options simultaneously in a separate patch, if at all. Also, can you post any amended patch series in chronological order so it's easier to apply? Cheers, Herman Rimm
Hi Herman, On 2023-11-05 18:03, Herman Rimm wrote: > Guix pull fails with the [PATCH 4/4] commit. It could be because > 'ssl? #t' is still used for the inet-listeners in the default > dovecot-configuration. Oops, this must have passed under the radar due to some stale .go file issue, nice catch! > By the way, I see that the "required" value can be used for the > ssl dovecot core setting, but for the inet-listener this is not > specified: https://doc.dovecot.org/configuration_manual/service_configuration/?highlight=inet_listener#ssl. According to the dovecot link you provided, it isn't clear whether this 'ssl' in the context of inet-listener is a dovecot boolean [1] or is a string in the same manner like the core ssl setting [2]. I'm afraid the dovecot documentation isn't the clearest here. > I think changing the ssl? option type from boolean to string should > be done for all boolean valued options simultaneously in a separate > patch, if at all. Some of the boolean options are really dovecot boolean, it's only this ssl? field in the inet-listener that is strange. I think it might be better to leave it for a separate patch as you have suggested, in that case feel free to discard the 4/4 patch. > Also, can you post any amended patch series in chronological order > so it's easier to apply? I'm afraid not since this is a limitation of email, there's no guarantee that emails arrive in the same order that they're sent. [1]: <https://doc.dovecot.org/settings/types/#boolean> [2]: <https://doc.dovecot.org/settings/core/#core_setting-ssl>
On Mon, Nov 06 2023, Bruno Victal wrote: >> By the way, I see that the "required" value can be used for the >> ssl dovecot core setting, but for the inet-listener this is not >> specified: https://doc.dovecot.org/configuration_manual/service_configuration/?highlight=inet_listener#ssl. > > According to the dovecot link you provided, it isn't clear whether this > 'ssl' in the context of inet-listener is a dovecot boolean [1] or is a > string in the same manner like the core ssl setting [2]. > I'm afraid the dovecot documentation isn't the clearest here. > >> I think changing the ssl? option type from boolean to string should >> be done for all boolean valued options simultaneously in a separate >> patch, if at all. > > Some of the boolean options are really dovecot boolean, it's only > this ssl? field in the inet-listener that is strange. > I think it might be better to leave it for a separate patch as you > have suggested, in that case feel free to discard the 4/4 patch. Indeed the inet_listener ssl is a BOOL, as code says, whereas the master one is an ENUM. So the actual guix service looks correct. The code for the master setting: --8<---------------cut here---------------start------------->8--- static const struct setting_define master_setting_defines[] = { DEF(STR, base_dir), DEF(STR, state_dir), DEF(STR, libexec_dir), DEF(STR, instance_name), DEF(STR, protocols), DEF(STR, listen), DEF(ENUM, ssl), DEF(STR, default_internal_user), DEF(STR, default_internal_group), DEF(STR, default_login_user), DEF(UINT, default_process_limit), DEF(UINT, default_client_limit), DEF(TIME, default_idle_kill), DEF(SIZE, default_vsz_limit), DEF(BOOL, version_ignore), DEF(UINT, first_valid_uid), DEF(UINT, last_valid_uid), DEF(UINT, first_valid_gid), DEF(UINT, last_valid_gid), DEFLIST_UNIQUE(services, "service", &service_setting_parser_info), SETTING_DEFINE_LIST_END }; static const struct master_settings master_default_settings = { .base_dir = PKG_RUNDIR, .state_dir = PKG_STATEDIR, .libexec_dir = PKG_LIBEXECDIR, .instance_name = PACKAGE, .protocols = "imap pop3 lmtp", .listen = "*, ::", .ssl = "yes:no:required", .default_internal_user = "dovecot", .default_internal_group = "dovecot", .default_login_user = "dovenull", .default_process_limit = 100, .default_client_limit = 1000, .default_idle_kill = 60, .default_vsz_limit = 256*1024*1024, .version_ignore = FALSE, .first_valid_uid = 500, .last_valid_uid = 0, .first_valid_gid = 1, .last_valid_gid = 0, #ifndef CONFIG_BINARY .services = ARRAY_INIT #else .services = { { &config_all_services_buf, sizeof(struct service_settings *) } }, #endif }; --8<---------------cut here---------------end--------------->8--- The code for the inet_listener setting: --8<---------------cut here---------------start------------->8--- static const struct setting_define inet_listener_setting_defines[] = { DEF(STR, name), DEF(STR, address), DEF(IN_PORT, port), DEF(BOOL, ssl), DEF(BOOL, reuse_port), DEF(BOOL, haproxy), SETTING_DEFINE_LIST_END }; --8<---------------cut here---------------end--------------->8--- Cheers, Clément
On Mon, Nov 06 2023, Bruno Victal wrote: >> Also, can you post any amended patch series in chronological order >> so it's easier to apply? > > I'm afraid not since this is a limitation of email, there's no > guarantee that emails arrive in the same order that they're sent. I don't know your workflow to apply patches but you can probably sort them by subject? (C-c C-s C-s with Gnus) It's quite reliable. More than depending on the time they are sent.
Hi Clément, On 2023-11-06 15:43, Clément Lassieur wrote: > Indeed the inet_listener ssl is a BOOL, as code says, whereas the master > one is an ENUM. So the actual guix service looks correct. […] > The code for the inet_listener setting: > > --8<---------------cut here---------------start------------->8--- > static const struct setting_define inet_listener_setting_defines[] = { > DEF(STR, name), > DEF(STR, address), > DEF(IN_PORT, port), > DEF(BOOL, ssl), > DEF(BOOL, reuse_port), > DEF(BOOL, haproxy), > > SETTING_DEFINE_LIST_END > }; > --8<---------------cut here---------------end--------------->8--- Thanks for looking into this. In that case, the description of the ssl? field in inet-listener-configuration should be fixed then. I'll send a v2 for it.
diff --git a/doc/guix.texi b/doc/guix.texi index 5242e89104..ed5ee4e583 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -26674,7 +26674,7 @@ Mail Services @item @code{port} (type: non-negative-integer) The port on which to listen. -@item @code{ssl?} (default: @code{#t}) (type: boolean) +@item @code{ssl?} (default: @code{yes}) (type: string) Whether to use SSL for this service; @samp{yes}, @samp{no}, or @samp{required}. @end table diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm index d4b050f73e..170c1f5dfc 100644 --- a/gnu/services/mail.scm +++ b/gnu/services/mail.scm @@ -269,7 +269,7 @@ (define-configuration inet-listener-configuration non-negative-integer "The port on which to listen.") (ssl? - (boolean #t) + (string "yes") "Whether to use SSL for this service; @samp{yes}, @samp{no}, or @samp{required}."))