Skip site navigation (1) Skip section navigation (2)

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

From: Yeb Havinga <yebhavinga(at)gmail(dot)com>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>, Joshua Brindle <jbrindle(at)tresys(dot)com>
Subject: Re: [v9.2] Add GUC sepgsql.client_label
Date: 2012-03-03 14:12:33
Message-ID: 4F5226D1.3020707@gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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


Attachment: pgsql-v9.2-sepgsql-setcon.part2-v3.patch
Description: text/x-patch (24.5 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Dimitri FontaineDate: 2012-03-03 14:26:28
Subject: Re: Command Triggers, patch v11
Previous:From: Thom BrownDate: 2012-03-03 14:03:01
Subject: Re: Command Triggers, patch v11

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group