Re: [SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 09/08/2009 09:25 AM, Dmitri Pal wrote: > >> >>> 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) > > This is exactly where I get confused. > I free memory I allocated and I do not free memory I did not. > The pointer in question can point to the external memory > passed in that I do not control on this level or to the allocated > memory that I free at some point. This is why I kind of > confused... > > > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel Pushed to master. - -- 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/ iEYEARECAAYFAkqnqoEACgkQeiVVYja6o6Oe5wCeLxXX9PXnh7usk8GbuVMN93Ug pcoAniq6ByC0DWbmpKx4lCbJwqA4OumU =GGxQ -END PGP SIGNATURE- ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
> > > 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) This is exactly where I get confused. I free memory I allocated and I do not free memory I did not. The pointer in question can point to the external memory passed in that I do not control on this level or to the allocated memory that I free at some point. This is why I kind of confused... ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
-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@lists.fedorahosted.org
>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>
> ___
> sssd-devel mailing list
> sssd-devel@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-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
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@lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>
___
sssd-devel mailing list
sssd-devel@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/
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
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?
>
> * the copy mode could be made unsigned (or even enum copy_mode {...}),
> you wouldn't have to test for mode<0
>
> * 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.
___
sssd-devel mailing list
sssd-devel@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/
iEYEARECAAYFAkqmOGkACgkQeiVVYja6o6PJaQCeK5w8DxkaQIqwgkovfB4N8+ye
VisAnifuUxLIzMEHkcx5zhh7fl6xZ7W/
=e6rl
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
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?
* the copy mode could be made unsigned (or even enum copy_mode {...}),
you wouldn't have to test for mode<0
* 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
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkqlEyEACgkQHsardTLnvCUX9ACfQ1Y2XZqsqukSZD72IPilJNxq
dqoAoMGhB5/3AR+NnW6pysEQtt/23rhA
=bQMa
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] COLLECTION Copy collection flat with concatenated names
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.
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
---
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
>From b977001de994b660da7dd73399cb95cba70bd13d Mon Sep 17 00:00:00 2001
From: Dmitri Pal
Date: Fri, 4 Sep 2009 20:28:06 -0400
Subject: [PATCH] COLLECTION Copy collection flat with concatenated names
This patch addresses several issues:
a) Adds capability to add or copy the collections
in flattened mode but construct names of attributes
in dotted notation. For example when you append collection
"sub" with items "foo" and "bar" previously you
could add them as "foo" and "bar" not you can
flatten them and the names will be "sub.foo"
and "sub.bar" this allows better processing of the
attributes in the elapi message.
b) Removes old implemntation of the
copy collection function.
c) Removes the col_set_timestamp,
this functionality has been moved to ELAPI
long ago.
d) Updates collection unit test.
e) Updates elapi to use new functionality
f) Updates elapi unit test
Have run under valgrind with no problems.
---
common/collection/collection.c | 505
common/collection/collection.h | 56 +++--
common/collection/collection_ut.c | 14 +-
common/elapi/elapi_event.c |3 +-
common/elapi/elapi_test/elapi_ut.c | 26 ++
5 files changed, 365 insertions(+), 239 deletions(-)
diff --git a/common/collection/collection.c b/common/collection/collection.c
index 8d3b3b5..3075eff 100644
--- a/common/collection/collection.c
+++ b/common/collection/collection.c
@@ -68,6 +68,25 @@ struct update_property {
int found;
};
+/* This struct is used to construct path
+ * to an item in the collection (tree)
+ */
+struct path_data {
+char *name;
+int length;
+struct path_data *previous_path;
+};
+
+/* Structure to keep data needed to
+ * copy collection
+ * while traversing it
+ */
+struct col_copy {
+int mode;
+struct path_data *current_path;
+char *given_name;
+int given_len;
+};
/ FUNCTION DECLARATIONS /
@@ -1113,11 +1132,6 @@ static void col_delete_collection(struct collection_item *ci)
/* Internal data structures used for search */
-struct path_data {
-char *name;
-int length;
-struct path_data *previous_path;
-};
struct find_name {
const char *name_to_find;
@@ -1133,7 +1147,8 @@ struct find_name {
/* Create a new name */
static int col_create_path_data(struct path_data **name_path,
const char *name, int length,
-const char *property, int property_len)
+const char *property, int property_len,
+char sep)
{
int error = EOK;
struct path_data *new_name_path;
@@ -1162,7 +1177,7 @@ static int col_create_path_data(struct path_data **name_path,
if(length > 0) {
memcpy(new_name_path->name, name, length);
new_name_path->length = length;
-new_name_path->name[new_name_path->length] = '!';
+new_name_path->name[new_name_path->length] = sep;
new_name_path->length++;
new_name_path->name[new_name_path->length] = '\0';
TRACE_INFO_STRING("Name so far:", new_name_path->name);
@@ -1792,7 +1807,8 @@ static int col_act_traverse_handler(struct collection_item *head,
TRACE_INFO_STRING("col_act_traverse_handler", "About to create path data.");
error = col_create_path_data(&(traverse_data->current_path),
- name, length, property, property_len);
+ name, length,
+ property, property_len, '!');
TRACE_INFO_NUMBER("col_create_path_data returned:", error);
return error;
@@ -1899,62 +1915,198 @@ static int col_copy_traverse_handler(struct collection_item *head,
{
int error = EOK;
struct collection_item *parent;
-struct collection_item *item;
-struct collection_item *new_collection = NULL;
+struct collection_item *other = NULL;
+struct col_copy *traverse_data;
+struct path_data *temp;
+char *name;
+int length;
+char *property = NULL;
+int property_len;
+struct collection_header *header;
TRACE_FLOW_STRING("col_copy_traverse_handler", "Entry.");
-parent = (struct collection_item *)passed_traverse_data;
+parent = (struct collection_item *)custom_data;
+traverse_data = (struct col_copy *)passed_traverse_data;
+
+/* We can be called when current points to NULL */
+/* This will happen only in the FLATDOT case */
+if (current == NULL) {
+TRACE_INFO_STRING("col_copy_traverse_handler",
+ "Special call at the end of the collect
