On (29/06/15 14:17), Pavel Reichl wrote:
On 06/29/2015 11:56 AM, Lukas Slebodnik wrote:
>On (26/06/15 17:31), Pavel Reichl wrote:
>So it seems you still do not understand what's wrong from desing point of view.
>If you do not understand something please ask it before. We would save your
>time with preparing patches and time of other developers reviewing unnecessary
>round of patches.
Sorry, that you feel that way.
I don't agree with essence of your concerns. I don't think I'm forcing new
paradigm into SSSD. You can discuss this topic with pbrezina, I'm sure he
has plenty to say.
I'm sorry I do not see any correlation between your patches
He does not include inconsistencies into sssd. He mostly reduces them.
introduced 2 new functions into sysdb which operated on
higher level than other functions in sysdb. I can agree that this was not
completely consistent with the rest of sysdb API but I don't think I was
lowering the quality of code.
I didn't say that code has low quality. So please
or do not put words in my mouth.
I wrote many times that it is inconsistent approach. We have many hacks/
inconsistencies in sssd so we needn't introduce a new one.
I didn't rejected getters and setters therefore I proposed to replace all
current functions + public constants with getters and setters.
However Jakub agreed with you and proposed
solution. So I had to respect the majority.
Have I somehow strayed from Jakub's proposal? If it is so, how?
If you don't like Jakub proposal please respond to his post directly.
sorry but you were faster and sent patches earlier than
I was able to respond to Jakub's mail. I explained what was wrong with
the latest version and described three clean ways from design point of view
in my previous mail. I didn't want to force you to implement the concrete one.
You had a choice and you could implement way which you like the most.
>I would like to repeat what was not right from design point of
view in previous
>version. You used totally different approach for functions. They weren't wrong
>they just did not fit among other sysdb functions.
>Let's imagine pure object oriented library and someone decides to create new
>class with just static functions. He wrote this class because he wanted to
>emulate procedural paradigm. It wouldn't be a problem if other classes
>were written in such way. But mixing them together make a stinky code.
>It's the same as let chimpanzee sit in an auditorium. it is not dangerous and
>can clap as well but it doesn't fit among other people. It would be fine
>if auditorious contained just chimpanzees.
>And now why is current version even worse then previous one from design point
>of view. You renamed public functions and moved them to tottaly unrelated part
>of code. It is an equivalent of get dressed chimpanzee to different clothes and
>let it sit in city council. It still does not fit there.
Lukas, I'm getting old and honestly I don't find this kind of message funny
but plainly hurtful and it makes me feel really sick. If you want to get rid
of me just talk to my manager instead of trying to induce me a heart attack.
The communication is two way channel. You wanted me to elaborate. I did.
The long mail was written but it seems it was not clear. So I tried to
write a more figurative mail and I hoped it would be easier to understand.
I encouraged you to ask questions. Unfortunately I miss a response from your
side what was not clear. I just saw a mails with patches.
So I'm really disappointed from lack of communication from your side.
Even this mail contains just complaints and still I do not know what wasn't
clearly explained. Which of the proposals is bad from your POV or what you
don't like or which one you like the most. I'm really opened to discussion
to have the nice and clear design but it's not possible with lack of discussion
from your side. (sending patches is not a real communication)
>>From 38dc66b619d6838610940f23ef84e0851a003319 Mon Sep 17
>>From: Pavel Reichl <preichl(a)redhat.com>
>>Date: Wed, 24 Jun 2015 09:07:55 -0400
>>Subject: [PATCH 1/3] sysdb: new attribute lastOnlineAuthWithCurrentToken
>>Introduce new user attribute lastOnlineAuthWithCurrentToken.
>>src/db/sysdb.h | 1 +
>>src/responder/pam/pam_helpers.c | 98 ++++++++++++++++++++++++++++++++++++++++-
>>src/responder/pam/pam_helpers.h | 15 +++++++
>>src/tests/cmocka/test_pam_srv.c | 37 ++++++++++++++++
>>4 files changed, 150 insertions(+), 1 deletion(-)
>>diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>>@@ -87,6 +87,7 @@
>>#define SYSDB_LAST_ONLINE_AUTH "lastOnlineAuth"
>>#define SYSDB_LAST_FAILED_LOGIN "lastFailedLogin"
>>#define SYSDB_FAILED_LOGIN_ATTEMPTS "failedLoginAttempts"
>>#define SYSDB_LAST_UPDATE "lastUpdate"
>>#define SYSDB_CACHE_EXPIRE "dataExpireTimestamp"
>>diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
>>@@ -23,6 +23,8 @@
>>errno_t pam_initgr_cache_set(struct tevent_context *ev,
>> hash_table_t *id_table,
>> char *name,
>>@@ -35,4 +37,17 @@ errno_t pam_initgr_cache_set(struct tevent_context *ev,
>>errno_t pam_initgr_check_timeout(hash_table_t *id_table,
>> char *name);
>>+pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>>+ const char *username);
>>+pam_get_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>>+ const char *name,
>>+ uint64_t *_value);
>>+pam_set_last_online_auth_with_curr_token(struct sss_domain_info *domain,
>>+ const char *username,
>>+ uint64_t value);
>>#endif /* PAM_HELPERS_H_ */
>Here are three proposal which you can use.
>a) create reusable public functions in sysdb and not a single purpose
> functions. Reusable public functions would fit to current sysdb API.
>b) create internal helper functions in pam_responder. They should be internal
> (static) and they purpose should be to improve readalibity of code; avoid
> long functions. If you want them in separate module then create a new file
> for fast pam authentication with public pam functions.
> Helper functions (wrappers) must be internal in this module. So compiler can
> inline them and readability is not decreased.
> If you warry about code coverage of static functions then you needn't.
> They already use tested public functions from sysdb and it's recommended to
> write tests for new functionality. So they will be indirectly tested.
>c) Relace all SYSDB_* constants with setters and getters. So internal sysdb
> library will be consistent.
>Here is a list. (I hope I didn't forget anything)
> SYSDB_SUBDOMAIN_CLASS, SYSDB_USER_CLASS, SYSDB_GROUP_CLASS,
> SYSDB_NETGROUP_CLASS, SYSDB_HOST_CLASS, SYSDB_HOSTGROUP_CLASS,
> SYSDB_SELINUX_USERMAP_CLASS, SYSDB_SELINUX_CLASS, SYSDB_ID_RANGE_CLASS,
> SYSDB_DOMAIN_ID_RANGE_CLASS, SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS,
> SYSDB_NAME, SYSDB_NAME_ALIAS, SYSDB_OBJECTCLASS, SYSDB_NEXTID,
> SYSDB_UIDNUM, SYSDB_GIDNUM, SYSDB_CREATE_TIME, SYSDB_PWD, SYSDB_FULLNAME,
> SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_MEMBEROF, SYSDB_DISABLED, SYSDB_MEMBER,
> SYSDB_MEMBERUID, SYSDB_GHOST, SYSDB_POSIX, SYSDB_USER_CATEGORY,
> SYSDB_HOST_CATEGORY, SYSDB_GROUP_TYPE, SYSDB_GECOS, SYSDB_LAST_LOGIN,
> SYSDB_LAST_ONLINE_AUTH, SYSDB_LAST_FAILED_LOGIN,
> SYSDB_FAILED_LOGIN_ATTEMPTS, SYSDB_LAST_ONLINE_AUTH_WITH_CURR_TOKEN,
> SYSDB_LAST_UPDATE, SYSDB_CACHE_EXPIRE, SYSDB_INITGR_EXPIRE,
> SYSDB_IFP_CACHED, SYSDB_AUTHORIZED_SERVICE, SYSDB_AUTHORIZED_HOST,
> SYSDB_NETGROUP_TRIPLE, SYSDB_ORIG_NETGROUP_MEMBER,
> SYSDB_ORIG_NETGROUP_EXTERNAL_HOST, SYSDB_NETGROUP_DOMAIN,
> SYSDB_NETGROUP_MEMBER, SYSDB_DESCRIPTION , SYSDB_FQDN,
> SYSDB_SERVERHOSTNAME, SYSDB_CACHEDPWD, SYSDB_CACHEDPWD_TYPE,
> SYSDB_CACHEDPWD_FA2_LEN, SYSDB_UUID, SYSDB_SID, SYSDB_PRIMARY_GROUP,
> SYSDB_PRIMARY_GROUP_GIDNUM, SYSDB_SID_STR, SYSDB_UPN, SYSDB_CANONICAL_UPN,
> SYSDB_CCACHE_FILE, SYSDB_ORIG_DN, SYSDB_ORIG_MODSTAMP, SYSDB_ORIG_MEMBEROF,
> SYSDB_ORIG_MEMBER, SYSDB_ORIG_MEMBER_USER, SYSDB_ORIG_MEMBER_HOST,
> SYSDB_USN, SYSDB_HIGH_USN, SYSDB_SSH_PUBKEY, SYSDB_AUTH_TYPE,
> SYSDB_USER_CERT, SYSDB_SUBDOMAIN_REALM, SYSDB_SUBDOMAIN_FLAT,
> SYSDB_SUBDOMAIN_ID, SYSDB_SUBDOMAIN_MPG, SYSDB_SUBDOMAIN_ENUM,
> SYSDB_SUBDOMAIN_FOREST, SYSDB_SUBDOMAIN_TRUST_DIRECTION, SYSDB_BASE_ID,
> SYSDB_ID_RANGE_SIZE, SYSDB_BASE_RID, SYSDB_SECONDARY_BASE_RID,
> SYSDB_DOMAIN_ID, SYSDB_ID_RANGE_TYPE, SYSDB_DEFAULT_OVERRIDE_NAME,
> SYSDB_AD_ACCOUNT_EXPIRES, SYSDB_AD_USER_ACCOUNT_CONTROL, SYSDB_VIEW_CLASS,
> SYSDB_VIEW_NAME, SYSDB_DEFAULT_VIEW_NAME, SYSDB_OVERRIDE_CLASS,
> SYSDB_OVERRIDE_ANCHOR_UUID, SYSDB_OVERRIDE_USER_CLASS,
> SYSDB_OVERRIDE_GROUP_CLASS, SYSDB_OVERRIDE_DN, SYSDB_OVERRIDE_OBJECT_DN,
> SYSDB_HAS_ENUMERATED, SYSDB_IDMAP_SUBTREE, SYSDB_IDMAP_MAPPING_OC,
> SYSDB_IDMAP_SID_ATTR, SYSDB_IDMAP_SLICE_ATTR, SYSDB_GPO_OC,
> SYSDB_GPO_GUID_ATTR, SYSDB_GPO_VERSION_ATTR, SYSDB_GPO_TIMEOUT_ATTR,
Please be constructive and choose one of these clean proposals from design
point of view. Or propose the better one.