Hi Jim,
Thanks for reply.
I got even more confused now reading with the mail indentation and
comment to the code
that does not exist :)
One slightly unrelated question, embedded_su(1M) nclaims its stability
level to be
stable, why is it then it is install in /usr/lib, should it not be in
/usr/sbin?
(BTW, if this is already answered in the ARC case, just tell me to shut
up okay :)
-Ghee
Jim Li wrote:
> Hi Ghee,
>
> I totally agree your point, it's hard to review a patch's patch. So I
> add some comments in this case, hope it help
>
> thank you, Ghee, for your review and suggestion!
>
> Jim
> Ghee Teo wrote:
>
>
>> This is just a general comment, can the submitter in general provide a
>> short description of the fix.
>> Especially when the patch is a patch to an existing patch. Since the
>> idea of the review is
>> to improve the quality of patches, and the idea of more eyes on a
>> piece of code will help.
>> However if the code is just a snipplet, with out some explanation text
>> that would be hard.
>> I don't believe anyone who does not own the component can make
>> reasonable comment
>> with this patch for example.
>>
>> So some minimum explanation text please in the future review patches
>> :), please
>>
>> -Ghee
>>
>>
>> Jim Li wrote:
>>
>>
>>> Hey,
>>>
>>> Please review the following change to libgksu1.2-04-rabc-support.diff.
>>> Welcome any suggestion and comments.
>>>
>>> Thanks,
>>>
>>> Jim
>>> ------------------------------------------------------------------------
>>>
>>> Index: ChangeLog
>>> ===================================================================
>>> --- ChangeLog (revision 9602)
>>> +++ ChangeLog (working copy)
>>> @@ -1,3 +1,9 @@
>>> +2006-10-30 Jim Li <jim.li at sun.com>
>>> +
>>> + * patches/libgksu1.2-04-rbac-support.diff:
>>> + add echo command when try to validate user's password.
>>> + fix a bug that it hang when it's invoded as a root.
>>> +
>>> 2006-10-30 Irene Huang <irene.huang at sun.com>
>>>
>>> * gnome-session.spec: add patch 10-gnome-volcheck-default-session.diff
>>> Index: patches/libgksu1.2-04-rbac-support.diff
>>> ===================================================================
>>> --- patches/libgksu1.2-04-rbac-support.diff (revision 9602)
>>> +++ patches/libgksu1.2-04-rbac-support.diff (working copy)
>>> @@ -1,5 +1,5 @@
>>> ---- libgksu1.2-1.3.1.orig/libgksu/gksu-context.c 2005-06-18
>>> 22:16:33.000000000 +0800
>>> -+++ libgksu1.2-1.3.1/libgksu/gksu-context.c 2006-10-19
>>> 16:41:10.678082000 +0800
>>> +--- libgksu1.2-1.3.1.orig/libgksu/gksu-context.c Sat Jun 18 22:16:33
>>> 2005
>>> ++++ libgksu1.2-1.3.1/libgksu/gksu-context.c Mon Oct 30 19:30:25 2006
>>> @@ -23,7 +23,13 @@
>>> #include <unistd.h>
>>> #include <string.h>
>>> @@ -329,7 +329,7 @@
>>> if (!context->command)
>>> {
>>> -@@ -1496,3 +1697,977 @@
>>> +@@ -1496,3 +1697,983 @@
>>> return FALSE;
>>> }
>>> @@ -417,6 +417,12 @@
>>>
> add some source code above this diff:
>
> argcount = 0;
>
> cmd[argcount] = g_strdup("/usr/lib/embedded_su");
>
>
>>> + cmd[argcount] = g_strdup (context->user);
>>> + argcount++;
>>> +
>>> ++ cmd[argcount] = g_strdup ("-c");
>>> ++ argcount++;
>>> ++
>>> ++ cmd[argcount] = g_strdup ("echo > /dev/null");
>>> ++ argcount++;
>>> ++
>>> + cmd[argcount] = NULL;
>>> +
>>> + pid = fork();
>>>
>>
> execn(cmd[0], cmd);
>
> the goal of this paragraph code is to check if need password when try to
> assume another user ro role ( context->user ), the old code doesn't
> provide any command just like:
> /usr/lib/embedded_su user
> When it find the embedded_su need a password, it simply kill the child
> process and return TRUE.
> if embedded_su doesn't need a password, it should execute a null command
> and immediately return instead of fork a shell and hang there
>
