On (28/07/16 19:37), Michal Židek wrote:
>On 07/28/2016 02:11 PM, Michal Židek wrote:
>> On 07/28/2016 01:57 PM, Jakub Hrozek wrote:
>> > On Thu, Jul 28, 2016 at 01:51:40PM +0200, Pavel Březina wrote:
>> > > On 07/28/2016 01:38 PM, Jakub Hrozek wrote:
>> > > > On Thu, Jul 28, 2016 at 12:23:24PM +0200, Michal Židek wrote:
>> > > > > On 07/28/2016 10:00 AM, Pavel Březina wrote:
>> > > > > > On 07/27/2016 03:28 PM, Michal Židek wrote:
>> > > > > > > On 07/27/2016 11:09 AM, Jakub Hrozek wrote:
>> > > > > > > > On Wed, Jul 27, 2016 at 11:03:34AM +0200, Pavel Březina wrote:
>> > > > > > > > > On 07/26/2016 04:19 PM, Michal Židek wrote:
>> > > > > > > > > > On 07/26/2016 01:19 PM, Pavel Březina wrote:
>> > > > > > > > > > > On 07/25/2016 02:12 PM, Michal Židek wrote:
>> > > > > > > > > > > > Hi,
>> > > > > > > > > > > >
>> > > > > > > > > > > > this patches makes the sssctl commands more similar to
>> > > > > > > > > > > > ipa tool commands. I also think this pattern makes it
>> > > > > > > > > > > > easier to remember the commands.
>> > > > > > > > > > > >
>> > > > > > > > > > > > Note that in the future, there will be more user-*
>> > > > > > > > > > > > group-* and netgroup-* commands (like seed for user,
>> > > > > > > > > > > > list of all etc.)
>> > > > > > > > > > > >
>> > > > > > > > > > > > Comments are welcome.
>> > > > > > > > > > > >
>> > > > > > > > > > > > Michal
>> > > > > > > > > > >
>> > > > > > > > > > > Hi,
>> > > > > > > > > > > ok, it looks like a good idea. When touching the code,
>> > > > > > > > > > > can
>> > > > > > > > > > > you also
>> > > > > > > > > > > convert sss_override command to use the macro instead?
>> > > > > > > > > > > And I
>> > > > > > > > > > > think it
>> > > > > > > > > > > may be nice to also add a macro for command sentinel
>> > > > > > > > > > > i.e. for
>> > > > > > > > > > > {NULL,
>> > > > > > > > > > > ...}.
>> > > > > > > > > > >
>> > > > > > > > > > > I'm not very fond of renaming local-data-* to cache-* so
>> > > > > > > > > > > it
>> > > > > > > > > > > doesn't
>> > > > > > > > > > > imply that we backup the whole cache content. We only
>> > > > > > > > > > > backup and
>> > > > > > > > > > > restore
>> > > > > > > > > > > data that are local to the client and not present in
>> > > > > > > > > > > LDAP.
>> > > > > > > > > > > Currently
>> > > > > > > > > > > only local overrides, but it may include local users and
>> > > > > > > > > > > groups in
>> > > > > > > > > > > the
>> > > > > > > > > > > future.
>> > > > > > > >
>> > > > > > > > When we have the files provider there would
>> > > > > > > > be a cache as well. Moreover, we store secrets now. The restore
>> > > > > > > > command
>> > > > > > > > backs up all *.ldb files, right?
>> > > > > > >
>> > > > > > > This is how I understood it at first, but the current backup
>> > > > > > > and restore only work for local overrides. But as Pavel
>> > > > > > > mentioned,
>> > > > > > > it may work for local users and groups in the future
>> > > > > > > (id_provider=local). My original confusion was that I also
>> > > > > > > thought it backs-up and restores all ldb files, which is
>> > > > > > > not the case.
>> > > > > >
>> > > > > > No, the intention is to backup only data that are not stored on
>> > > > > > server
>> > > > > > and would be lost when the cache is removed. In the time, only
>> > > > > > local
>> > > > > > overrides were local. If secrets creates local data, the tool
>> > > > > > should be
>> > > > > > modified.
>> > > > >
>> > > > > Yes, but users and groups from local domain would also be
>> > > > > lost if the local data is deleted. So I though we want to backup
>> > > > > them as well in the future versions (I mean the local provider,
>> > > > > not the files provider).
>> > > >
>> > > > Well, probably not in the first version, but definitely in a couple of
>> > > > months we want to add the capability to set extended attributes to the
>> > > > files provider which we'll want to back up as well. But I also think we
>> > > > will add the additional data into a new directory, just like we did
>> > > > with
>> > > > the secrets database.
>> > > >
>> > > > >
>> > > > > Btw. do we want to merge sss_override tool into sssctl?
>> > > > > Because if the local-data-* commands work currently with
>> > > > > overrides only, then we could make a new topic 'overrides' and add
>> > > > > commands
>> > > > > like
>> > > >
>> > > > I would like to merge all tools into sssctl unless there is a strong
>> > > > reason to keep them separate (the local domain tools should be kept
>> > > > separate IMO).
>> > > >
>> > > > The reason is simply discoverability, if there is a new sssd release,
>> > > > the admin would just run sssctl and if there are any new tools, their
>> > > > topics would be displayed.
>> > > >
>> > > > (Also I think the code duplication would be reduced as a side-effect).
>> > > >
>> > > > >
>> > > > > sssctl overrides-backup
>> > > > > sssctl overrides-restore
>> > > > >
>> > > > > and later also all the functionality of sss_overrides
>> > > > >
>> > > > > sssctl overrides-user-add
>> > > > > sssctl overrides-user-del
>> > > >
>> > > > Maybe, but later.
>> > > >
>> > > > >
>> > > > > etc.
>> > > > >
>> > > > > This way we could avoid confusion between local-data and
>> > > > > cache. If secrets will also create some local data, we will
>> > > > > add topic 'secrets' to deal with that separately.
>> > > > >
>> > > > > sssctl secrets-backup
>> > > > > sssctl secrets-restore
>> > > >
>> > > > I think I got lost in the thread :-) What is the benefit of having more
>> > > > backup/restore commands than one that backs up or removes of value
>> > > > under
>> > > > the /var/lib/sss/ structure? So far I can only think of cache being
>> > > > more
>> > > > intuitive to admins than local-data.
>> > >
>> > > How about client-data instead of local-data?
>> >
>> > SGTM (sounds good to me :-))
>>
>> SGTM too :)
>>
>> I will send version with client-data after the meeting.
>>
>
>I forgot to update this after the meeting.
>New patch is attached.
>
>Michal
>
>From 73c3e97013274ff644aa630547494fd79b1854c0 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
>Date: Mon, 25 Jul 2016 13:50:13 +0200
>Subject: [PATCH 1/3] sssctl: Consistent commands naming
>
>Fixes:
>https://fedorahosted.org/sssd/ticket/3087
>
>Use TOPIC-ACTION pattern for sssctl command
>names.
>---
> src/tools/common/sss_tools.c | 4 ++++
> src/tools/common/sss_tools.h | 8 ++++++--
> src/tools/sss_override.c | 26 ++++++++++++------------
> src/tools/sssctl/sssctl.c | 33 ++++++++++++++++++++----------
> src/tools/sssctl/sssctl.h | 42 +++++++++++++++++++--------------------
> src/tools/sssctl/sssctl_cache.c | 18 ++++++++---------
> src/tools/sssctl/sssctl_data.c | 16 +++++++--------
> src/tools/sssctl/sssctl_domains.c | 6 +++---
> src/tools/sssctl/sssctl_logs.c | 4 ++--
> 9 files changed, 88 insertions(+), 69 deletions(-)
>
>diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c
>index 686b53a..4977358 100644
>--- a/src/tools/common/sss_tools.c
>+++ b/src/tools/common/sss_tools.c
>@@ -283,6 +283,10 @@ void sss_tool_usage(const char *tool_name, struct
>sss_route_cmd *commands)
> min_len = sss_tool_max_length(commands);
>
> for (i = 0; commands[i].command != NULL; i++) {
>+ if (commands[i].hidden) {
>+ continue;
>+ }
>+
> if (sss_tool_is_delimiter(&commands[i])) {
> fprintf(stderr, "\n%s\n", commands[i].description);
> continue;
>diff --git a/src/tools/common/sss_tools.h b/src/tools/common/sss_tools.h
>index a9ebabe..b567ea4 100644
>--- a/src/tools/common/sss_tools.h
>+++ b/src/tools/common/sss_tools.h
>@@ -45,14 +45,18 @@ typedef errno_t
> struct sss_tool_ctx *tool_ctx,
> void *pvt);
>
>-#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn}
>-#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL}
>+#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn, false}
>+#define SSS_TOOL_COMMAND_NOMSG(cmd, err, fn) {cmd, NULL, err, fn, false}
>+#define SSS_TOOL_COMMAND_OBSOLETE(cmd, err, fn) {cmd, NULL, err, fn, true}
>+#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL, false}
>+#define SSS_TOOL_LAST {NULL, NULL, 0, NULL, false}
>
> struct sss_route_cmd {
> const char *command;
> const char *description;
> errno_t handles_init_err;
> sss_route_fn fn;
>+ bool hidden; /* Do not show this command in sssctl usage */
> };
>
> void sss_tool_usage(const char *tool_name,
>diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
>index 45a28fd..d41da52 100644
>--- a/src/tools/sss_override.c
>+++ b/src/tools/sss_override.c
>@@ -1913,19 +1913,19 @@ static int override_group_export(struct sss_cmdline
>*cmdline,
> int main(int argc, const char **argv)
> {
> struct sss_route_cmd commands[] = {
>- {"user-add", NULL, 0, override_user_add},
>- {"user-del", NULL, 0, override_user_del},
>- {"user-find", NULL, 0, override_user_find},
>- {"user-show", NULL, 0, override_user_show},
>- {"user-import", NULL, 0, override_user_import},
>- {"user-export", NULL, 0, override_user_export},
>- {"group-add", NULL, 0, override_group_add},
>- {"group-del", NULL, 0, override_group_del},
>- {"group-find", NULL, 0, override_group_find},
>- {"group-show", NULL, 0, override_group_show},
>- {"group-import", NULL, 0, override_group_import},
>- {"group-export", NULL, 0, override_group_export},
>- {NULL, NULL, 0, NULL}
>+ SSS_TOOL_COMMAND_NOMSG("user-add", 0, override_user_add),
>+ SSS_TOOL_COMMAND_NOMSG("user-del", 0, override_user_del),
>+ SSS_TOOL_COMMAND_NOMSG("user-find", 0, override_user_find),
>+ SSS_TOOL_COMMAND_NOMSG("user-show", 0, override_user_show),
>+ SSS_TOOL_COMMAND_NOMSG("user-import", 0, override_user_import),
>+ SSS_TOOL_COMMAND_NOMSG("user-export", 0, override_user_export),
>+ SSS_TOOL_COMMAND_NOMSG("group-add", 0, override_group_add),
>+ SSS_TOOL_COMMAND_NOMSG("group-del", 0, override_group_del),
>+ SSS_TOOL_COMMAND_NOMSG("group-find", 0, override_group_find),
>+ SSS_TOOL_COMMAND_NOMSG("group-show", 0, override_group_show),
>+ SSS_TOOL_COMMAND_NOMSG("group-import", 0, override_group_import),
>+ SSS_TOOL_COMMAND_NOMSG("group-export", 0, override_group_export),
>+ SSS_TOOL_LAST
> };
>
> return sss_tool_main(argc, argv, commands, NULL);
>diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
>index 86656f1..8ce93b2 100644
>--- a/src/tools/sssctl/sssctl.c
>+++ b/src/tools/sssctl/sssctl.c
>@@ -257,25 +257,36 @@ int main(int argc, const char **argv)
> {
> struct sss_route_cmd commands[] = {
> SSS_TOOL_DELIMITER("SSSD Status:"),
>- SSS_TOOL_COMMAND("list-domains", "List available domains", 0,
>sssctl_list_domains),
>+ SSS_TOOL_COMMAND("domain-list", "List available domains", 0,
>sssctl_domain_list),
> SSS_TOOL_COMMAND("domain-status", "Print information about domain",
> 0, sssctl_domain_status),
> SSS_TOOL_DELIMITER("Information about cached content:"),
>- SSS_TOOL_COMMAND("user", "Information about cached user", 0,
>sssctl_user),
>- SSS_TOOL_COMMAND("group", "Information about cached group", 0,
>sssctl_group),
>- SSS_TOOL_COMMAND("netgroup", "Information about cached netgroup", 0,
>sssctl_netgroup),
>+ SSS_TOOL_COMMAND("user-show", "Information about cached user", 0,
>sssctl_user_show),
>+ SSS_TOOL_COMMAND("group-show", "Information about cached group", 0,
>sssctl_group_show),
>+ SSS_TOOL_COMMAND("netgroup-show", "Information about cached
>netgroup", 0, sssctl_netgroup_show),
> SSS_TOOL_DELIMITER("Local data tools:"),
>- SSS_TOOL_COMMAND("backup-local-data", "Backup local data", 0,
>sssctl_backup_local_data),
>- SSS_TOOL_COMMAND("restore-local-data", "Restore local data from
>backup", 0, sssctl_restore_local_data),
>- SSS_TOOL_COMMAND("remove-cache", "Backup local data and remove cached
>content", 0, sssctl_remove_cache),
>- SSS_TOOL_COMMAND("upgrade-cache", "Perform cache upgrade",
>ERR_SYSDB_VERSION_TOO_OLD, sssctl_upgrade_cache),
>+ SSS_TOOL_COMMAND("client-data-backup", "Backup local data", 0,
>sssctl_client_data_backup),
>+ SSS_TOOL_COMMAND("client-data-restore", "Restore local data from
>backup", 0, sssctl_client_data_restore),
>+ SSS_TOOL_COMMAND("cache-remove", "Backup local data and remove cached
>content", 0, sssctl_cache_remove),
>+ SSS_TOOL_COMMAND("cache-upgrade", "Perform cache upgrade",
>ERR_SYSDB_VERSION_TOO_OLD, sssctl_cache_upgrade),
> SSS_TOOL_DELIMITER("Log files tools:"),
>- SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD log files", 0,
>sssctl_remove_logs),
>- SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log files in tarball",
>0, sssctl_fetch_logs),
>+ SSS_TOOL_COMMAND("logs-remove", "Remove existing SSSD log files", 0,
>sssctl_logs_remove),
>+ SSS_TOOL_COMMAND("logs-fetch", "Archive SSSD log files in tarball",
>0, sssctl_logs_fetch),
> #ifdef HAVE_LIBINI_CONFIG_V1_3
> SSS_TOOL_DELIMITER("Configuration files tools:"),
> SSS_TOOL_COMMAND("config-check", "Perform static analysis of SSSD
> configuration", 0, sssctl_config_check),
> #endif
>- {NULL, NULL, 0, NULL}
>+ /* Obsolete commands */
>+ SSS_TOOL_COMMAND_OBSOLETE("list-domains", 0, sssctl_domain_list),
>+ SSS_TOOL_COMMAND_OBSOLETE("user", 0, sssctl_user_show),
>+ SSS_TOOL_COMMAND_OBSOLETE("group", 0, sssctl_group_show),
>+ SSS_TOOL_COMMAND_OBSOLETE("netgroup", 0, sssctl_netgroup_show),
>+ SSS_TOOL_COMMAND_OBSOLETE("backup-local-data", 0,
>sssctl_client_data_backup),
>+ SSS_TOOL_COMMAND_OBSOLETE("restore-local-data", 0,
>sssctl_client_data_restore),
>+ SSS_TOOL_COMMAND_OBSOLETE("remove-cache", 0, sssctl_cache_remove),
>+ SSS_TOOL_COMMAND_OBSOLETE("upgrade-cache", ERR_SYSDB_VERSION_TOO_OLD,
>sssctl_cache_upgrade),
>+ SSS_TOOL_COMMAND_OBSOLETE("remove-logs", 0, sssctl_logs_remove),
>+ SSS_TOOL_COMMAND_OBSOLETE("fetch-logs", 0, sssctl_logs_fetch),
Do we really need to be so "backward compatible"
a) there was a desing[1] and nobody complained
b) 1.14.0 is used very rarely
BTW We should firstly change desing page. Then ask for review and then
send a patch. Cond-NACK to patch till desing page will be updated and
change will be accepted by others.
LS
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
