Challenges in merging the charcnv code

Andrew Bartlett abartlet at samba.org
Tue Mar 31 06:00:38 GMT 2009


On Fri, 2009-03-27 at 09:27 +1100, Andrew Bartlett wrote:
> On Thu, 2009-03-26 at 22:26 +0100, Volker Lendecke wrote:
> > On Fri, Mar 27, 2009 at 08:18:01AM +1100, Andrew Bartlett wrote:
> > > > To be honest, I'm too busy with other stuff to got through
> > > > and put in the talloc_tos(). And I would have to understand
> > > > all the push_string_fn macros which is quite intricate to
> > > > me.
> > > 
> > > That leaves me rather confused:  Do you want this code merged or not? Do
> > > you want me to do it?
> > 
> > As I said: The patches look good. But I would really have to
> > take a lot of time to review it, this is just too central,
> > and I would have to really learn about all the push_string
> > macro magic. I stumbled over the very first patch of yours
> > where you split up a function for two different use cases.
> > 
> > I might be too slow due to my age, but just from looking at
> > the patch I could not quickly enough figure out what those
> > two use cases are. So I would have to dive in an figure out
> > what this is. Maybe I will do that over the weekend when no
> > customers want me to do stuff, but I can't promise anything.
> 
> The two use cases are were we use 'base', and where we do not.
> 
> The base pointer points at the start of the SMB packet.  The comments
> claimed that the pointer was used to determine if the packet was
> unicode, but this was removed a while back (fortunately - replaced with
> the flags2 parameter).  It is used to determine the alignment the string
> should be on (assuming that it is possible - perhaps it is not - that
> the start of the smb packet is on an odd alignment). 
> 
> Most of the cases do not use a base pointer, which is not provided by
> the Samba4 charcnv API (which I was trying to get as close to as
> possible).  These now call a simpler API (push_string_check - which
> still checks buffer sizes using macro foo)
> 
> The remaining calls that did care about the base pointer are made via
> other functions in clistr.c and srvstr.c, and so do not need a macro -
> calling push_string_base directly.  
> 
> (Samba4 does not need the _base call because all smb buffers are
> allocated with talloc(), and are therefore aligned.  I understand this
> history in Samba3 comes from a time when smb packets were constructed on
> the stack, and this may not apply in Samba3 either, but I was not
> willing to make that call for this conversion). 
> 
> Finally, as to the macro renaming:  If we can avoid having a macro of
> name 'push_string', then we can provide the Samba4 API as a normal
> function, and my goal of having a common smbencrypt.c (yes, that was
> what started all this) can be advanced.  
> 
> The pull_string macro was removed as it had no callers.
> 
> The patch stream does include a bug:  push_string_fn still references
> flags2 in the first patch, and this is fixed in the last patch in the
> series.  

Has this helped you with your review of the code?  

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Red Hat Inc.                  http://redhat.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20090331/14e1b503/attachment.bin


More information about the samba-technical mailing list