Re: [v9.2] Add GUC sepgsql.client_label

On 2012-02-24 17:25, Yeb Havinga wrote:
> On 2012-02-23 12:17, Kohei KaiGai wrote:
>> 2012/2/20 Yeb Havinga<yebhavinga(at)gmail(dot)com>:
>>> On 2012-02-05 10:09, Kohei KaiGai wrote:
>>>> The attached part-1 patch moves related routines from hooks.c to
>>>> label.c
>>>> because of references to static variables. The part-2 patch
>>>> implements above
>>>> mechanism.
>>>
>>> I took a short look at this patch but am stuck getting the
>>> regression test
>>> to run properly.
>>>
>>> First, patch 2 misses the file sepgsql.sql.in and therefore the
>>> creation
>>> function command for sepgsql_setcon is missing.
>>>
>> Thanks for your comments.
>>
>> I added the definition of sepgsql_setcon function to sepgsql.sql.in
>> file,
>> in addition to patch rebasing.
>
> Very brief comments due to must leave keyboard soon:
>
> I read the source code and played a bit with setcon and the debugger,
> no strange things found.
>
> Code comments / questions:

I took the liberty to change a few things, mostly comments, in the
attached patch:
>
> maybe client_label_committed is a better name for client_label_setcon?

this change was made.
>
> Is the double negation in the sentence below intended?

several comments were changed / moved. There is now one place where te
behaviour of the different client_label variables are explained.

>
> sepgsql_set_client_label(), maybe add a comment to !new_label that it
> is reset to the peer label.

done.

>
> Is the assert client_label_peer != NULL in sepgsql_get_client_label
> necessary?
> new_label == NULL / pending_label->label == NULL means use the peer
> label. Why not use the peer label instead?

It turned out that pending_label->label is invariantly non null. Changed
code to assume that and added some Asserts.

>
> set_label: if new_label == current label according to getcon, is it
> necessary to add to the pending list?

this question still remains. Maybe there would be reasons to hit selinux
with the question: can I change from A->A.
>
> sepgsql_subxact_callback(), could this be made easier to read by just
> taking llast(client_label_pending), assert that plabel->subid ==
> mySubId and then list_delete on pointer of that listcell?

no this was a naieve suggestion, which fails in any case of a
subtransaction with not exactly one call to sepgsql_setcon()

> Some comments contain typos, I can spend some time on this, though I'm
> not a native english speaker so it won't be perfect.

sgml documentation must still be added. If time permits I can spend some
time on that tomorrow.

regards,
Yeb Havinga

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data