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

Dmitri Pal dpal at redhat.com
Tue Sep 8 12:41:14 UTC 2009


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.

> _______________________________________________
> 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

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


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




More information about the sssd-devel mailing list