[SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names

Stephen Gallagher sgallagh at redhat.com
Tue Sep 8 12:54:48 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/08/2009 08:41 AM, Dmitri Pal wrote:
> Thank you for review, comments inline.
> 
> Stephen Gallagher wrote:
>> On 09/07/2009 10:05 AM, Jakub Hrozek wrote:
>>> On 09/05/2009 02:32 AM, Dmitri Pal wrote:
>>>> See patch comment.
>>>> This patch most likely will not apply cleanly without my previous ELAPI
>>>> patches.
>>>> So far it is 4 th pending patch.
>>>> More to come.
>>> In general, I think the patch is OK. Some comments, though:
>>
>>> * col_copy_traverse_handler got pretty long, one function now accounts
>>> for more that 200 lines doing different things, esp. with different copy
>>> mode handling. Maybe it would be more readable if it was split into
>>> smaller ones?
> 
> It got a bit long, I agree but splitting will IMO break the internal
> line of thought related to this function.
> I took a look at it and could not see how to better slice it.
> Allowing longer lines would have made it twice shorter. :-)
>>
>>> * the copy mode could be made unsigned (or even enum copy_mode {...}),
>>> you wouldn't have to test for mode<0
> 
> I wanted to be consistent with other modes in similar functions that are
> integers.
> May be they all should have been unsigned but I would not go for a
> sweeping change at least now.
> Fill file a bug though: #168
> 
>>
>>> * if you really need assigning const* to non-const*, can you just use
>>> something like the discard_const_p macro in server/util/util.h, I think
>>> it's way better that memcpy-ing the memory
>>
>>>     Jakub
>>
>> I think I prefer Dmitri's approach here, actually. name_to_use is not
>> under Dmitri's control (unless I'm misreading the patch), so it's
>> theoretically possible for the owner of that memory to change its
>> contents underneath you. If you're going to need that data to remain
>> constant and trustworthy further on in the code (e.g. used for
>> comparisons), you need to copy its current state.
> Steve I do not think I get your statement. I copy the pointer instead
> of direct assignment since any attempt with direct assignment
> was producing warnings.
> 
> Here is the situation:
> The internal function is recursive. The pointer I use need to be
> initialized to a value.
> During the recursion the state changes and the pointer points to
> different strings
> To make this happen I had to event something. And I hate warnings.
> So I came with the implemented approach and put a comment there to explain.
> 
> Jakub I also looked at the macro in util.h.
> I tried similar approaches but all of them caused different kinds of
> warnings.
> I am not sure that this typecast will help. At least I do not see how
> this typecast
> is different from any other typecast. May be there is something specific
> about  uintptr_t?
>  
> I am unclear about couple things here:
> a) Is my solution acceptable for the given situation?
> b) If not what is the preferred solution?
> c) What is different about the given typecast and why this typecast
> suppresses
> warning (if it does) while others do not?
> 
> Please advise.

I think you misunderstood what I was saying. I think you did it the
right way, Dmitri. (provided of course that you free that memory
responsibly when you no longer need it)

> 
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
> 


- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqmVBIACgkQeiVVYja6o6PUXgCaAgiRRC6DEr01Ozeex16GFFmh
a9oAoK9+jn43SG/MEnW+MdS57XGq9txO
=WpCM
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list