[ConsoleKit] Review for bug 26055

[Ray Strode]

Hi,

On Tue, May 18, 2010 at 7:52 AM, Halton Huo <halton.huo at gmail.com> wrote:
> On Tue, 2010-05-18 at 07:15 -0400, Ray Strode wrote:
>> Hi,
>>
>> On Tue, May 18, 2010 at 3:17 AM, Halton Huo <halton.huo at gmail.com> wrote:
>>
>> > Can somebody review patch for
>> > https://bugs.freedesktop.org/show_bug.cgi?id=26055?
>> There's certainly nothing wrong with the concept, but the patch has a
>> few issues that need to be fixed.
>>
>> 1) Internal functions have to have return types and be declared static
> Add gboolean as return value for is_vt_enabled()
>
>> 2) Functions have to be in all lower case
> Rename to is_vt_enabled()
>
>> 3) presumably you need to link libscf in?
> Yes, $(SCF_LIBS) is added to console_kit_daemon_LDADD in src/Makefile.am
>
>> 4) you check for HAVE_SYS_VT_H in one place, HAVE_SCF in another.  I
>> have vt.h on linux and i don't have those functions, so you probably
>> shouldn't check for HAVE_VT_H.
> Yes, should use HAVE_SCF instead of HAVE_SYS_VT_H
>
> There is another logic error in old patch. is_vt_enabled() should always
> return TRUE in case of not having HAVE_SCF.
>
> I updated the patch in the bug report, please check here
> https://bugs.freedesktop.org/attachment.cgi?id=35724
>
> Look okay?
So looking at ConsoleKit more, I see that it puts ifdefs in very
specific parts of the code and avoids it elsewhere.

Reading through ConsoleKit more, i think the right approach is one of:

1) drop is_vt_enabled and use ck_get_max_num_consoles (&num_consoles);
... if (.... && num_consoles > 0)

or if that won't work

2) Add ck_supports_activatable_consoles to ck-sysdeps.h.  In
ck-sysdeps-linux and ck-sysdeps-freebsd unconditionally return TRUE.
In ck-sysdeps-solaris, have your code.

1 is probably the smallest patch, assuming get_max_num_consoles
returns 0 like I think it should in solaris.

--Ray