local memory contexts to avoid a discard_const_p()

[Matthias Dieter Wallnöfer]

Jelmer,

I've uploaded a proposal: in my private repo on git.samba.org the branch 
"oid_const".

Cheers,
Matthias

Jelmer Vernooij wrote:
> On Mon, 2010-12-13 at 10:55 +0000, Matthias Dieter Wallnöfer wrote:
>
>    
>> but is there some particular reason that "oid" from
>> "dsdb_schema_pfm_oid_from_attid" is returned "const" - as far as I've
>> seen it's an allocated thing? If it's not then we would not need
>> "discard_const_p" at all.
>>      
> No, that indeed doesn't seem necessary. At first glance, changing that
> function to return a non-const seems like a more reasonable approach. It
> seems like it would also require changing some of the ber_read_*
> functions though.
>
> Cheers,
>
> Jelmer
>    
>> On Sun, 2010-12-12 at 21:11 +0100, Jelmer Vernooij wrote:
>>      
>>> Hi Matthias,
>>>
>>> As discussed earlier, can you please send this kind of patch to the
>>>        
>> list
>>      
>>> for review first?
>>>
>>> I have my doubts about the usefulness of these changes in general
>>> compared to the benefit. Eliminating talloc contexts doesn't really
>>>        
>> gain
>>      
>>> us much. The overhead of creating an extra talloc context compared
>>>        
>> to
>>      
>>> the overhead of using Python in the first place is negligible.
>>>        
>>      
>>>> @@ -248,27 +253,19 @@ static PyObject
>>>>          
>> *py_dsdb_get_oid_from_attid(PyObject *self, PyObject *args)
>>      
>>>>
>>>>       PyErr_LDB_OR_RAISE(py_ldb, ldb);
>>>>
>>>> -    mem_ctx = talloc_new(NULL);
>>>> -    if (mem_ctx == NULL) {
>>>> -      PyErr_NoMemory();
>>>> -      return NULL;
>>>> -    }
>>>> -
>>>>       schema = dsdb_get_schema(ldb, NULL);
>>>>
>>>>       if (!schema) {
>>>>           PyErr_SetString(PyExc_RuntimeError, "Failed to find a
>>>>          
>> schema from ldb \n");
>>      
>>>> -        talloc_free(mem_ctx);
>>>>           return NULL;
>>>>       }
>>>>
>>>>       status = dsdb_schema_pfm_oid_from_attid(schema->prefixmap,
>>>>          
>> attid,
>>      
>>>> -                                            mem_ctx,&oid);
>>>> +                                            NULL,&oid);
>>>>       PyErr_WERROR_IS_ERR_RAISE(status);
>>>>
>>>>       ret = PyString_FromString(oid);
>>>> -
>>>> -    talloc_free(mem_ctx);
>>>> +    talloc_free(discard_const_p(char, oid));
>>>>          
>>> ^^ Is this really necessary? I'd rather have the extra memory
>>>        
>> context
>>      
>>> than add an extra discard_const_p.
>>>        
>> I agree, it's really useful to be able to avoid the const issues by
>> using a new memory context here.  Every discard_const_p() hides a
>> potentially serious bug, and so should be avoided where other
>> reasonable
>> alternatives exist).  The local memory context seems a very reasonable
>> alternative, and so it seems a backwards step to eliminate it.
>>
>> Andrew Bartlett
>>
>> -- 
>> Andrew Bartlett
>> http://samba.org/~abartlet/
>> Authentication Developer, Samba Team          http://samba.org
>> Samba Developer, Cisco Inc.
>>
>>
>>      
>