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?= <mzidek(a)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]