On (29/07/16 12:55), Jakub Hrozek wrote:
On Fri, Jul 29, 2016 at 09:31:32AM +0200, Lukas Slebodnik wrote:
> 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?= <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"
I would say yes, we should be. We can remove the commands later, but we
shouldn't break backwards compatibility if the tarball was already
released.
Nobody will comply if we remove it because
sssd-1.14.0 is used very very rarely
and if somebody complain we can provide old aliases in 1.14.x
IMHO, it does not worth to add complexity for unused feature
because of "backward compatibility"
>
> 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
Fine by me.
> and
> change will be accepted by others.
Others who? :-)
non developers (The person who requested this change; I assume this
change was not requested by developers)
In other words, the same process as with creating new design document.
LS