local memory contexts to avoid a discard_const_p()

[Jelmer Vernooij]

Hi Matthias,

+1 on the first patch.

The other patches to remove const seem right to me, but I'm curious why
these functions used const at all.  Tridge, can you perhaps confirm it's
ok to drop the const modifier in those asn1 functions ? I'm just
wondering if there's something I'm missing there.

Cheers,

Jelmer

On Wed, 2010-12-15 at 18:58 +0100, Matthias Dieter Wallnöfer wrote:
> Jelmer Vernooij wrote:
> > Hi Matthias,
> >
> > Attaching the files works for me.
> >
> > Cheers, jelmer
> >
> > Matthias Dieter Wallnöfer<mdw at samba.org>  wrote:
> >
> >    
> >> Hi Jelmer,
> >>
> >> could I simply attach the patch files or all patches in one mbox file to
> >> the message?
> >>
> >> Cheers,
> >> Matthias
> >>
> >> Jelmer Vernooij wrote:
> >>      
> >>> Hi Matthias,
> >>>
> >>> Can you perhaps send your changes to the list, perhaps using git send-mail? Gitweb is down at the moment and it is very useful to be able to comment inline.
> >>>
> >>> Cheers, jelmer
> >>>
> >>> Matthias Dieter Wallnöfer<mdw at samba.org>   wrote:
> >>>
> >>>
> >>>        
> >>>> 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.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>              
> >>>>>
> >>>>>            
> >>>>
> >>>>          
> >>      
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101216/aed89bce/attachment.pgp>