[PATCHES] Collection, Ref Array, ELAPI

[Dmitri Pal]

Hi,

This is resubmission of the two earlier withdrawn patches (Ref Array and
ELAPI) and addition of a new patch to collection.
For details see the patch comments.

Note about ELAPI patch: it is huge so it is zipped. This patch includes
the first cut of eliminating the elapi_test directory altogether. The
directory is not built any more so there is no need to spend much time
looking at the files that were updated in this directory. They will be
either removed in follow up patches or moved to elapi directory.

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


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

[Stephen Gallagher]

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

On 12/14/2009 04:59 PM, Dmitri Pal wrote:
>  Hi,
>  
>  This is resubmission of the two earlier withdrawn patches (Ref Array and
>  ELAPI) and addition of a new patch to collection.
>  For details see the patch comments.
>  
>  Note about ELAPI patch: it is huge so it is zipped. This patch includes
>  the first cut of eliminating the elapi_test directory altogether. The
>  directory is not built any more so there is no need to spend much time
>  looking at the files that were updated in this directory. They will be
>  either removed in follow up patches or moved to elapi directory.
>  
Patch 0001:
Nack.

COL_DSP_INDEX should not allow you to pass an index greater than one
past the current size of the collection. This should throw an error
rather than just placing it at the end, because it indicates that the
calling program has a flawed expectation of the size of the collection.

In general, since the implementation is a linked-list, you almost
certainly don't want to allow retrieval access by index, since every
time you insert something, you change the indexes of subsequent
requests. The expectation of a program inserting a value by index is
usually that they will be able to retrieve that entry by requesting the
same index later. It's much safer to force them to search or walk the
collection to find the entry they are looking for.

Do not leave a semicolon at the end of the do...while(0) loop for
COLOUT. The whole purpose of using do...while(0) is so that it is safe
to apply a semicolon when the macro is used later in the code. (Some
compilers throw an error if preprocessed code has two adjacent semicolons)

It would be preferable if the verbosity could be set by an environment
variable (in addition to the commandline flag) (the unit tests in the
server directory use "CK_VERBOSITY=verbose", so it wouldn't be a
terrible idea to use the same variable)


Patch 0002:
Nack.

../../../../common/elapi/refarray/ref_array.c: In function
‘ref_array_debug’:
../../../../common/elapi/refarray/ref_array.c:260: warning: format ‘%d’
expects type ‘int’, but argument 2 has type ‘size_t’

You did not add the necessary changes to common/elapi/Makefile.am and
configure.ac to build the new refarray directory.

As above, I'd like to see the unit test verbosity set by an environment
variable. You also left the print statements in main() outside of RAOUT()

Your unit test for copying and assignment is flawed. You need to print
both results and assert that they strcmp()==0. Also, it's dangerously
unsafe to do an assignment to the same variable to which you are
returning a value by reference. They need to be different.

The grow_by parameter needs to have a default setting if 0 is passed.
Right now, it's possible to pass 0 and the realloc will create a buffer
of the same size as the previous buffer, potentially resulting in a
buffer overrun vulnerability when we copy in the new value.

Similarly, ref_array_create() needs to return an error if elemsz < 1.

In ref_array_destroy(), please change the code layout so that you first do
if (!ra) {
    TRACE_ERROR_STRING("Uninitialized array.", "Coding error???");
    return;
}

if (ra->refcount < 1) {
    /* Should never be here...
    * This can happen if the caller by mistake would try to
    * destroy the object from within the callback. Brrr....
    */
    TRACE_ERROR_STRING("Reference count is 0.", "Coding error???");
}

And then have the rest of the code two fewer indentation levels. It's
easier to read. Please also note that you need to check that the
refcount is < 1, not just that it's zero. A negative value could
indicate a memory error or a forgotten initializer somewhere.

Speling misteak at line 233 in ref_array.c. "retuning" should be
"returning"

ref_array_len() needs to have a way to return an error instead of
returning 0 when ra isn't valid.

I'd recommend that you want:
int ref_array_len(struct ref_array *ra, uint32_t *ret)
{
    TRACE_FLOW_STRING("ref_array_len", "Entry");

    if (!ra) {
        TRACE_ERROR_STRING("Uninitialized argument.", "");
        return EINVAL;
    }

    TRACE_FLOW_STRING("ref_array_len", "Exit");
    *ret = ra->len;
    return EOK;
}

Instead of having your debug function call printf directly (which is
buffered and might notprint completely), I'd recommend that this
function should allocate memory to store the complete output string and
return a pointer to it. Then the caller of ref_array_debug() can choose
to output or store it to a file in whatever manner they choose.



Review of the elapi megapatch coming later.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAksw74wACgkQeiVVYja6o6MjVQCgqtX888+qQfV+hOSOoUIxxp1s
> rQoAnicuy5Js/YSrNlvVZaH9CTlr3Bf3
> =FykH
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Stephen Gallagher wrote:
>  On 12/14/2009 04:59 PM, Dmitri Pal wrote:
>  > Hi,
>
>  > This is resubmission of the two earlier withdrawn patches (Ref Array and
>  > ELAPI) and addition of a new patch to collection.
>  > For details see the patch comments.
>
>  > Note about ELAPI patch: it is huge so it is zipped. This patch includes
>  > the first cut of eliminating the elapi_test directory altogether. The
>  > directory is not built any more so there is no need to spend much time
>  > looking at the files that were updated in this directory. They will be
>  > either removed in follow up patches or moved to elapi directory.
>
>
>  Patch 0001:
>  Nack.
>
>  COL_DSP_INDEX should not allow you to pass an index greater than one
>  past the current size of the collection. This should throw an error
>  rather than just placing it at the end, because it indicates that the
>  calling program has a flawed expectation of the size of the collection.
>
>  In general, since the implementation is a linked-list, you almost
>  certainly don't want to allow retrieval access by index, since every
>  time you insert something, you change the indexes of subsequent
>  requests. The expectation of a program inserting a value by index is
>  usually that they will be able to retrieve that entry by requesting the
>  same index later. It's much safer to force them to search or walk the
>  collection to find the entry they are looking for. 
This logic have been there for 6 months.
I will open a ticket for that. It should not block a review for this patch.


>  Do not leave a semicolon at the end of the do...while(0) loop for
>  COLOUT. The whole purpose of using do...while(0) is so that it is safe
>  to apply a semicolon when the macro is used later in the code. (Some
>  compilers throw an error if preprocessed code has two adjacent semicolons) 
Ok. Will do.

>  It would be preferable if the verbosity could be set by an environment
>  variable (in addition to the commandline flag) (the unit tests in the
>  server directory use "CK_VERBOSITY=verbose", so it wouldn't be a
>  terrible idea to use the same variable)
>  
Should it be one variable per component or should it be same variable
for collection, INI, ELAPI and other tools?
Will do.

>  Patch 0002:
>  Nack.
>
>  ../../../../common/elapi/refarray/ref_array.c: In function
>  ref_array_debug:
>  ../../../../common/elapi/refarray/ref_array.c:260: warning: format %d
>  expects type int, but argument 2 has type size_t
>  
I will take a look at this one.
Will do.


>  You did not add the necessary changes to common/elapi/Makefile.am
> and
>  configure.ac to build the new refarray directory. 
I think these changes are in the second patch. They have not been
included intentionally because it was hard to split patches for the
review purposes.

>  As above, I'd like to see the unit test verbosity set by an environment
>  variable. You also left the print statements in main() outside of RAOUT()
>  
Will do.
Same or different?

>  Your unit test for copying and assignment is flawed. You need to
> print
>  both results and assert that they strcmp()==0. Also, it's dangerously
>  unsafe to do an assignment to the same variable to which you are
>  returning a value by reference. They need to be different.
>  
Can you point to the line number here?
Probably will do.

>  The grow_by parameter needs to have a default setting if 0 is
> passed.
>  Right now, it's possible to pass 0 and the realloc will create a buffer
>  of the same size as the previous buffer, potentially resulting in a
>  buffer overrun vulnerability when we copy in the new value. 
Ah, good catch, agree.
Will do.

>  Similarly, ref_array_create() needs to return an error if elemsz < 1.
>  
Can size_t be negative? I thought it is always positive. So it should
check for 0.

Comment: These are all correct suggestions except that ref_array so far
is sort of "quick and dirty" internal interface.
The whole point is not add these "niceties" until it becomes clear if it
should be an externally usable interface or not.
This is why I put it under ELAPI because it is not clear how reusable it is.
I am leaning more to opening a bug and saying to address these issues if
ref_array becomes a public interface.
So far I agree that it is a good defensive programming approach but an
overhead for a specific situation.

Will do.

>  In ref_array_destroy(), please change the code layout so that you
> first do
>  if (!ra) {
>      TRACE_ERROR_STRING("Uninitialized array.", "Coding error???");
>      return;
>  }
>  
Makes sense.
Will do.


>  if (ra->refcount < 1) {
>      /* Should never be here...
>      * This can happen if the caller by mistake would try to
>      * destroy the object from within the callback. Brrr....
>      */
>      TRACE_ERROR_STRING("Reference count is 0.", "Coding error???");
>  }
>
>  And then have the rest of the code two fewer indentation levels. It's
>  easier to read. Please also note that you need to check that the
>  refcount is < 1, not just that it's zero. A negative value could
>  indicate a memory error or a forgotten initializer somewhere.
>  
It is unsigned. It can't be less than 0.

>  Speling misteak at line 233 in ref_array.c. "retuning"
> should be
>  "returning"
>  
Ok. Will do.


>  ref_array_len() needs to have a way to return an error instead of
>  returning 0 when ra isn't valid.
>
>  I'd recommend that you want:
>  int ref_array_len(struct ref_array *ra, uint32_t *ret)
>  {
>      TRACE_FLOW_STRING("ref_array_len", "Entry");
>
>      if (!ra) {
>          TRACE_ERROR_STRING("Uninitialized argument.", "");
>          return EINVAL;
>      }
>
>      TRACE_FLOW_STRING("ref_array_len", "Exit");
>      *ret = ra->len;
>      return EOK;
>  }
>  
The whole point of the function was not check for an error and suppress it.
I would rather add an error as a parameter if you insist.
Again good defensive practice but IMO an overhead here.




>  Instead of having your debug function call printf directly (which is
>  buffered and might notprint completely), I'd recommend that this
>  function should allocate memory to store the complete output string and
>  return a pointer to it. Then the caller of ref_array_debug() can choose
>  to output or store it to a file in whatever manner they choose.
>  
Too much work.
It is sufficient for my debugging purposes.
Patches are welcome :-)
 
>  Review of the elapi megapatch coming later.
>  
If you agree with my assessment I will provide fixes for items that I
said I "will do".
I will open a ticket for collection and will defer the rest.

_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/

[Jeff Schroeder]

On Tue, Dec 22, 2009 at 9:44 AM, Dmitri Pal <dpal(a)redhat.com&gt; wrote:
>  Stephen Gallagher wrote: > On 12/14/2009 04:59 PM, Dmitri Pal
> wrote: > > Hi, > > This is resubmission of the two earlier withdrawn patches
> (Ref Array and > > ELAPI) and addition of a new patch to collection. > > For
> details see the patch comments. > > Note about ELAPI patch: it is huge so it is
> zipped. This patch includes > > the first cut of eliminating the elapi_test
> directory altogether. The > > directory is not built any more so there is no need to
> spend much time > > looking at the files that were updated in this directory. They
> will be > > either removed in follow up patches or moved to elapi directory. >
> Patch 0001: > Nack. > COL_DSP_INDEX should not allow you to pass an index greater
> than one > past the current size of the collection. This should throw an error >
> rather than just placing it at the end, because it indicates that the > calling program
> has a flawed expectation of the size of the collection. > In general, since the
> implementation is a linked-list, you almost > certainly don't want to allow
> retrieval access by index, since every > time you insert something, you change the
> indexes of subsequent > requests. The expectation of a program inserting a value by
> index is > usually that they will be able to retrieve that entry by requesting the >
> same index later. It's much safer to force them to search or walk the > collection
> to find the entry they are looking for. This logic have been there for 6 months. I will
> open a ticket for that. It should not block a review for this patch. > Do not leave a
> semicolon at the end of the do...while(0) loop for > COLOUT. The whole purpose of using
> do...while(0) is so that it is safe > to apply a semicolon when the macro is used later
> in the code. (Some > compilers throw an error if preprocessed code has two adjacent
> semicolons) Ok. Will do. > It would be preferable if the verbosity could be set by an
> environment > variable (in addition to the commandline flag) (the unit tests in the
> > server directory use "CK_VERBOSITY=verbose", so it wouldn't be a >
> terrible idea to use the same variable) Should it be one variable per component or should
> it be same variable for collection, INI, ELAPI and other tools? Will do. > Patch 0002:
> > Nack. > ../../../../common/elapi/refarray/ref_array.c: In function ref_array_debug
> > ../../../../common/elapi/refarray/ref_array.c:260: warning: format  > expects type
>  , but argument 2 has type  size_t I will take a look at this one. Will do. > You did
> not add the necessary changes to common/elapi/Makefile.am and > configure.ac to build
> the new refarray directory. I think these changes are in the second patch. They have not
> been included intentionally because it was hard to split patches for the review purposes.
> > As above, I'd like to see the unit test verbosity set by an environment >
> variable. You also left the print statements in main() outside of RAOUT() Will do. Same or
> different? > Your unit test for copying and assignment is flawed. You need to print
> > both results and assert that they strcmp()==0. Also, it's dangerously > unsafe
> to do an assignment to the same variable to which you are > returning a value by
> reference. They need to be different. Can you point to the line number here? Probably will
> do. > The grow_by parameter needs to have a default setting if 0 is passed. > Right
> now, it's possible to pass 0 and the realloc will create a buffer > of the same
> size as the previous buffer, potentially resulting in a > buffer overrun vulnerability
> when we copy in the new value. Ah, good catch, agree. Will do. > Similarly,
> ref_array_create() needs to return an error if elemsz < 1. Can size_t be negative? I
> thought it is always positive. So it should check for 0. Comment: These are all correct
> suggestions except that ref_array so far is sort of "quick and dirty" internal
> interface. The whole point is not add these "niceties" until it becomes clear if
> it should be an externally usable interface or not. This is why I put it under ELAPI
> because it is not clear how reusable it is. I am leaning more to opening a bug and saying
> to address these issues if ref_array becomes a public interface. So far I agree that it is
> a good defensive programming approach but an overhead for a specific situation. Will do.
> > In ref_array_destroy(), please change the code layout so that you first do > if
> (!ra) { >     TRACE_ERROR_STRING("Uninitialized array.", "Coding
> error???"); >     return; Makes sense. Will do. > if (ra->refcount < 1) {
> >     /* Should never be here... >     * This can happen if the caller by mistake
> would try to >     * destroy the object from within the callback. Brrr.... >     */
> >     TRACE_ERROR_STRING("Reference count is 0.", "Coding
> error???"); > And then have the rest of the code two fewer indentation levels.
> It's > easier to read. Please also note that you need to check that the >
> refcount is < 1, not just that it's zero. A negative value could > indicate a
> memory error or a forgotten initializer somewhere. It is unsigned. It can't be less
> than 0. > Speling misteak at line 233 in ref_array.c. "retuning" should be
> > "returning" Ok. Will do. > ref_array_len() needs to have a way to return
> an error instead of > returning 0 when ra isn't valid. > I'd recommend that
> you want: > int ref_array_len(struct ref_array *ra, uint32_t *ret) >    
> TRACE_FLOW_STRING("ref_array_len", "Entry"); >     if (!ra) { >
>         TRACE_ERROR_STRING("Uninitialized argument.", ""); >      
>   return EINVAL; >     } >     TRACE_FLOW_STRING("ref_array_len",
> "Exit"); >     *ret = ra->len; >     return EOK; The whole point of the
> function was not check for an error and suppress it. I would rather add an error as a
> parameter if you insist. Again good defensive practice but IMO an overhead here. >
> Instead of having your debug function call printf directly (which is > buffered and
> might notprint completely), I'd recommend that this > function should allocate
> memory to store the complete output string and > return a pointer to it. Then the
> caller of ref_array_debug() can choose > to output or store it to a file in whatever
> manner they choose. Too much work. It is sufficient for my debugging purposes. Patches are
> welcome :-) > Review of the elapi megapatch coming later. If you agree with my
> assessment I will provide fixes for items that I said I "will do". I will open a
> ticket for collection and will defer the rest.
> _______________________________________________ sssd-devel mailing list
> sssd-devel(a)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(a)lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel 
Dmitri, your mua completely ate that message. Either it stripped all
newline characters or gmail freaked out. Just wanted to give you a
heads up.

-- 
Jeff Schroeder

Don't drink and derive, alcohol and analysis don't mix.
http://www.digitalprognosis.com

[Stephen Gallagher]

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

On 12/22/2009 12:44 PM, Dmitri Pal wrote:
>  Stephen Gallagher wrote:
> >
> > COL_DSP_INDEX should not allow you to pass an index greater than one
> > past the current size of the collection. This should throw an error
> > rather than just placing it at the end, because it indicates that the
> > calling program has a flawed expectation of the size of the collection.
> >
> > In general, since the implementation is a linked-list, you almost
> > certainly don't want to allow retrieval access by index, since every
> > time you insert something, you change the indexes of subsequent
> > requests. The expectation of a program inserting a value by index is
> > usually that they will be able to retrieve that entry by requesting the
> > same index later. It's much safer to force them to search or walk the
> > collection to find the entry they are looking for.
>  
>  This logic have been there for 6 months.
>  I will open a ticket for that. It should not block a review for this patch.
>   
Agreed.

>  
>  
> >
> > It would be preferable if the verbosity could be set by an environment
> > variable (in addition to the commandline flag) (the unit tests in the
> > server directory use "CK_VERBOSITY=verbose", so it wouldn't be a
> > terrible idea to use the same variable)
> >
>  
>  Should it be one variable per component or should it be same variable
>  for collection, INI, ELAPI and other tools?
>  Will do.
>   
I'd prefer it to be a single variable, perhaps SSSD_TEST_VERBOSE=1


> > You did not add the necessary changes to common/elapi/Makefile.am
> and
> > configure.ac to build the new refarray directory.
>  
>  I think these changes are in the second patch. They have not been
>  included intentionally because it was hard to split patches for the
>  review purposes.
>   
Patches need to be reviewable independently so that dependency patches
like this one can theoretically be committed while subsequent patches
are still under review.

Please fix this.

> >
> > As above, I'd like to see the unit test verbosity set by an environment
> > variable. You also left the print statements in main() outside of RAOUT()
> >
>  
>  Will do.
>  Same or different? 
Same as above.

>  
> > Your unit test for copying and assignment is flawed. You need to print
> > both results and assert that they strcmp()==0. Also, it's dangerously
> > unsafe to do an assignment to the same variable to which you are
> > returning a value by reference. They need to be different.
> >
>  
>  Can you point to the line number here?
>  Probably will do. 
Line 149 of ref_array_ut.c

> >
> > Similarly, ref_array_create() needs to return an error if elemsz < 1.
> >
>  
>  Can size_t be negative? I thought it is always positive. So it should
>  check for 0.
>   
You're right, size_t is unsigned. So checking for zero is fine. I
couldn't remember if it was signed or unsigned.

>  Comment: These are all correct suggestions except that ref_array so
> far
>  is sort of "quick and dirty" internal interface.
>  The whole point is not add these "niceties" until it becomes clear if it
>  should be an externally usable interface or not.
>  This is why I put it under ELAPI because it is not clear how reusable it is.
>  I am leaning more to opening a bug and saying to address these issues if
>  ref_array becomes a public interface.
>  So far I agree that it is a good defensive programming approach but an
>  overhead for a specific situation.
>  
>  Will do. 
Internal interfaces should be as correct as possible still


> > And then have the rest of the code two fewer indentation levels.
> It's
> > easier to read. Please also note that you need to check that the
> > refcount is < 1, not just that it's zero. A negative value could
> > indicate a memory error or a forgotten initializer somewhere.
> >
>  
>  It is unsigned. It can't be less than 0. 
Sorry, I misread. That's fine.

> > ref_array_len() needs to have a way to return an error instead
> of
> > returning 0 when ra isn't valid.
> >
> > I'd recommend that you want:
> > int ref_array_len(struct ref_array *ra, uint32_t *ret)
> > {
> >     TRACE_FLOW_STRING("ref_array_len", "Entry");
> >
> >     if (!ra) {
> >         TRACE_ERROR_STRING("Uninitialized argument.", "");
> >         return EINVAL;
> >     }
> >
> >     TRACE_FLOW_STRING("ref_array_len", "Exit");
> >     *ret = ra->len;
> >     return EOK;
> > }
> >
>  
>  The whole point of the function was not check for an error and suppress it.
>  I would rather add an error as a parameter if you insist.
>  Again good defensive practice but IMO an overhead here. 
Not just good defensive practice. A consumer of this API may use the
result of this call as the source of a conditional. If you return 0 for
an invalid structure, then they may still attempt to use the object.


> > Instead of having your debug function call printf directly (which
> is
> > buffered and might not print completely), I'd recommend that this
> > function should allocate memory to store the complete output string and
> > return a pointer to it. Then the caller of ref_array_debug() can choose
> > to output or store it to a file in whatever manner they choose.
> >
>  
>  Too much work.
>  It is sufficient for my debugging purposes.
>  Patches are welcome :-)
>    
Agreed. We can improve on this later.

> >
> >
> > Review of the elapi megapatch coming later.
> >
>  
>  If you agree with my assessment I will provide fixes for items that I
>  said I "will do".
>  I will open a ticket for collection and will defer the rest.
>  
Please see my comments above.


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAksxCXAACgkQeiVVYja6o6OrbwCcCz7EtaKZYUh56XnVBljxWJYn
> f0cAmwaC5LTG3M6CcmbX63z7gNVRyP8V
> =tm2e
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Stephen Gallagher wrote:
>  On 12/22/2009 12:44 PM, Dmitri Pal wrote:
>  > Stephen Gallagher wrote:
>  >> COL_DSP_INDEX should not allow you to pass an index greater than one
>  >> past the current size of the collection. This should throw an error
>  >> rather than just placing it at the end, because it indicates that the
>  >> calling program has a flawed expectation of the size of the collection.
>  >>
>  >> In general, since the implementation is a linked-list, you almost
>  >> certainly don't want to allow retrieval access by index, since every
>  >> time you insert something, you change the indexes of subsequent
>  >> requests. The expectation of a program inserting a value by index is
>  >> usually that they will be able to retrieve that entry by requesting the
>  >> same index later. It's much safer to force them to search or walk the
>  >> collection to find the entry they are looking for.
>  > This logic have been there for 6 months.
>  > I will open a ticket for that. It should not block a review for this
>  patch.
>
>
>  Agreed.
>
>
>  >> It would be preferable if the verbosity could be set by an environment
>  >> variable (in addition to the commandline flag) (the unit tests in the
>  >> server directory use "CK_VERBOSITY=verbose", so it wouldn't be a
>  >> terrible idea to use the same variable)
>  >>
>  > Should it be one variable per component or should it be same variable
>  > for collection, INI, ELAPI and other tools?
>  > Will do.
>
>
>  I'd prefer it to be a single variable, perhaps SSSD_TEST_VERBOSE=1 
But think long term: it should probably be COMMON_TEST_VERBOSE then
since eventually it will be broken out from the SSSD.
And may be it should be one for COMMON and one for ELAPI since it would
make sense IMO to break out eventually these two too.

>  >> You did not add the necessary changes to common/elapi/Makefile.am and
>  >> configure.ac to build the new refarray directory.
>  > I think these changes are in the second patch. They have not been
>  > included intentionally because it was hard to split patches for the
>  > review purposes.
>
>
>  Patches need to be reviewable independently so that dependency patches
>  like this one can theoretically be committed while subsequent patches
>  are still under review.
>
>  Please fix this.
>  
I disagree and I think it is an unneeded overhead.
git a -i is a good tool that I was not aware of but anyways with a huge
patch like this it is just asking for trouble.
If we think it should be reusable I can put it under common.
This I can agree with then the makefile changes would make sense.

>  >> As above, I'd like to see the unit test verbosity set by an environment
>  >> variable. You also left the print statements in main() outside of
>  RAOUT()
>  >>
>  > Will do.
>  > Same or different?
>
>  Same as above.
>
>  >> Your unit test for copying and assignment is flawed. You need to print
>  >> both results and assert that they strcmp()==0. Also, it's dangerously
>  >> unsafe to do an assignment to the same variable to which you are
>  >> returning a value by reference. They need to be different.
>  >> 
I do not understand what is wrong with line
        ret = *((char **)ref_array_get(ra2, i, &ret));

Yes it will crash if the array element is NULL but how it is used is a
perfectly valid use.
It returns the pointer to the element in the array and we store char *
in the array.
We know that.  And it will never be NULL untill you are out of boundaries.
It is also used from within the loop for up to the length and this
guarantees that the returned value is not NULL and conversion can happen.
With this unit test I illustrate the two different uses on the
ref_array_get that can be implemented.
One is the for loop and another is while loop. It is up to the collect
to pick the right approach for his case.
 
I do not see why anything should be changed here.

>  > Can you point to the line number here?
>  > Probably will do.
>
>  Line 149 of ref_array_ut.c
>
>  >> Similarly, ref_array_create() needs to return an error if elemsz < 1.
>  >>
>  > Can size_t be negative? I thought it is always positive. So it should
>  > check for 0.
>
>
>  You're right, size_t is unsigned. So checking for zero is fine. I
>  couldn't remember if it was signed or unsigned.
>
>  > Comment: These are all correct suggestions except that ref_array so far
>  > is sort of "quick and dirty" internal interface.
>  > The whole point is not add these "niceties" until it becomes clear if it
>  > should be an externally usable interface or not.
>  > This is why I put it under ELAPI because it is not clear how
>  reusable it is.
>  > I am leaning more to opening a bug and saying to address these issues if
>  > ref_array becomes a public interface.
>  > So far I agree that it is a good defensive programming approach but an
>  > overhead for a specific situation.
>
>  > Will do.
>
>  Internal interfaces should be as correct as possible still 
When you have time to do this.
I have a bigger fish to fry.
I am not arguing that these changes would make sense.
I argue that these changes are not required for the first patch that
introduces the interface.
If can't be expected to be perfect from get go.
It should be good enough to be able to move on with main goals.
Just a difference in the approach.


>  >> And then have the rest of the code two fewer indentation levels. It's
>  >> easier to read. Please also note that you need to check that the
>  >> refcount is < 1, not just that it's zero. A negative value could
>  >> indicate a memory error or a forgotten initializer somewhere.
>  >>
>  > It is unsigned. It can't be less than 0.
>
>  Sorry, I misread. That's fine.
>
>  >> ref_array_len() needs to have a way to return an error instead of
>  >> returning 0 when ra isn't valid.
>  >>
>  >> I'd recommend that you want:
>  >> int ref_array_len(struct ref_array *ra, uint32_t *ret)
>  >> {
>  >>     TRACE_FLOW_STRING("ref_array_len", "Entry");
>  >>
>  >>     if (!ra) {
>  >>         TRACE_ERROR_STRING("Uninitialized argument.", "");
>  >>         return EINVAL;
>  >>     }
>  >>
>  >>     TRACE_FLOW_STRING("ref_array_len", "Exit");
>  >>     *ret = ra->len;
>  >>     return EOK;
>  >> }
>  >>
>  > The whole point of the function was not check for an error and
>  suppress it.
>  > I would rather add an error as a parameter if you insist.
>  > Again good defensive practice but IMO an overhead here.
>
>  Not just good defensive practice. A consumer of this API may use the
>  result of this call as the source of a conditional. If you return 0 for
>  an invalid structure, then they may still attempt to use the object.
>  
Hm? I do not buy it. If you want to test the validity of the object you
check if it is NULL rather then call a length function.
The whole point of such simplified inteface is that you check for the
object validity once at the top and then just get it length when you
need to.
 
something like

int myfun(ref_array *ra)
{
    ...
   if (!ra) {
       ...
       return EINVAL;
    }

    len = ref_array_len();

    ....
}

 Now why check for ra twice and add overhead of error handling to a very
simple function that returns a property of the object?
 It is like a get method in C++. It just returns value.
>  >> Instead of having your debug function call printf directly (which is
>  >> buffered and might not print completely), I'd recommend that this
>  >> function should allocate memory to store the complete output string and
>  >> return a pointer to it. Then the caller of ref_array_debug() can choose
>  >> to output or store it to a file in whatever manner they choose.
>  >>
>  > Too much work.
>  > It is sufficient for my debugging purposes.
>  > Patches are welcome :-)
>
>
>  Agreed. We can improve on this later.
>
>  >>
>  >> Review of the elapi megapatch coming later.
>  >>
>  > If you agree with my assessment I will provide fixes for items that I
>  > said I "will do".
>  > I will open a ticket for collection and will defer the rest.
>
>
>  Please see my comments above.
>
>  
See my comments above :-)

_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/

[Stephen Gallagher]

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

On 12/22/2009 02:45 PM, Dmitri Pal wrote:

> > I'd prefer it to be a single variable, perhaps
> SSSD_TEST_VERBOSE=1
>  
>  But think long term: it should probably be COMMON_TEST_VERBOSE then
>  since eventually it will be broken out from the SSSD.
>  And may be it should be one for COMMON and one for ELAPI since it would
>  make sense IMO to break out eventually these two too.
>   
Fine with me.

>  
>  I disagree and I think it is an unneeded overhead.
>  git a -i is a good tool that I was not aware of but anyways with a huge
>  patch like this it is just asking for trouble.
>  If we think it should be reusable I can put it under common.
>  This I can agree with then the makefile changes would make sense.
>   
I'd like to see it under common. I can see this having use beyond just
ELAPI.


> >>> Your unit test for copying and assignment is flawed. You
> need to print
> >>> both results and assert that they strcmp()==0. Also, it's dangerously
> >>> unsafe to do an assignment to the same variable to which you are
> >>> returning a value by reference. They need to be different.
> >>>
>  
>  I do not understand what is wrong with line
>          ret = *((char **)ref_array_get(ra2, i, &ret));
>  
>  Yes it will crash if the array element is NULL but how it is used is a
>  perfectly valid use.
>  It returns the pointer to the element in the array and we store char *
>  in the array.
>  We know that.  And it will never be NULL untill you are out of boundaries.
>  It is also used from within the loop for up to the length and this
>  guarantees that the returned value is not NULL and conversion can happen.
>  With this unit test I illustrate the two different uses on the
>  ref_array_get that can be implemented.
>  One is the for loop and another is while loop. It is up to the collect
>  to pick the right approach for his case.
>   
>  I do not see why anything should be changed here. 
The problem is the use of "ret" in both the return from the function and
as the memory location for the copied code. It's the same variable
having data returned to it in two different ways from the same function
call. This is dangerous. It should look like

ret1 = *((char **)ref_array_get(ra2, i, &ret2));
if (strcmp(ret1, ret2) != 0) {
 //FAIL
}


>  
>  When you have time to do this.
>  I have a bigger fish to fry.
>  I am not arguing that these changes would make sense.
>  I argue that these changes are not required for the first patch that
>  introduces the interface.
>  If can't be expected to be perfect from get go.
>  It should be good enough to be able to move on with main goals.
>  Just a difference in the approach. 
The whole purpose of a review is so that only stable code reaches the
repository (hopefully).


>  Hm? I do not buy it. If you want to test the validity of the object
> you
>  check if it is NULL rather then call a length function.
>  The whole point of such simplified inteface is that you check for the
>  object validity once at the top and then just get it length when you
>  need to.
>   
>  something like
>  
>  int myfun(ref_array *ra)
>  {
>      ...
>     if (!ra) {
>         ...
>         return EINVAL;
>      }
>  
>      len = ref_array_len();
>  
>      ....
>  }
>  
>   Now why check for ra twice and add overhead of error handling to a very
>  simple function that returns a property of the object?
>   It is like a get method in C++. It just returns value. 
I understand what you're saying, but you're attempting to enforce a
coding standard in a function prototype. You can't know how a consumer
of the API is going to use this call, so you have to protect against it.
A NULL check is a very cheap operation.




- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAksxI7QACgkQeiVVYja6o6PSaACfcMcdnmjafn6GnzGruDJlLhEX
> A4IAnjvpi2TI/rMO/dDRbln0682hQmqX
> =44iE
> -----END PGP SIGNATURE-----

[Dmitri Pal]

>  > I do not see why anything should be changed here.
>
>  The problem is the use of "ret" in both the return from the function and
>  as the memory location for the copied code. It's the same variable
>  having data returned to it in two different ways from the same function
>  call. This is dangerous. It should look like
>
>  ret1 = *((char **)ref_array_get(ra2, i, &ret2));
>  if (strcmp(ret1, ret2) != 0) {
>   //FAIL
>  }
>
>  
Ah now I understand what you are talking about.
Yes I agree it is generally dangerous and should be discouraged but the
unit-test  checks that even if it is done this way it is Ok.
I should probably just add a comment about it to discourage this kind of
use and explain that this is a test to validate that it will not crash
if it is done this way.


>  > When you have time to do this.
>  > I have a bigger fish to fry.
>  > I am not arguing that these changes would make sense.
>  > I argue that these changes are not required for the first patch that
>  > introduces the interface.
>  > If can't be expected to be perfect from get go.
>  > It should be good enough to be able to move on with main goals.
>  > Just a difference in the approach.
>
>  The whole purpose of a review is so that only stable code reaches the
>  repository (hopefully).
>  
IMO this is a flaw in the process.


>  > Hm? I do not buy it. If you want to test the validity of the object you
>  > check if it is NULL rather then call a length function.
>  > The whole point of such simplified inteface is that you check for the
>  > object validity once at the top and then just get it length when you
>  > need to.
>
>  > something like
>
>  > int myfun(ref_array *ra)
>  > {
>  >     ...
>  >    if (!ra) {
>  >        ...
>  >        return EINVAL;
>  >     }
>
>  >     len = ref_array_len();
>
>  >     ....
>  > }
>
>  >  Now why check for ra twice and add overhead of error handling to a very
>  > simple function that returns a property of the object?
>  >  It is like a get method in C++. It just returns value.
>
>  I understand what you're saying, but you're attempting to enforce a
>  coding standard in a function prototype. You can't know how a consumer
>  of the API is going to use this call, so you have to protect against it.
>  A NULL check is a very cheap operation.
>
>  
This is exactly what I do internally:

/* Get length */
uint32_t ref_array_len(struct ref_array *ra)
{
    TRACE_FLOW_STRING("ref_array_len", "Entry");

    if (!ra) {
        TRACE_ERROR_STRING("Uninitialized argument.", "");
        return 0;
    }

    TRACE_FLOW_STRING("ref_array_len", "Exit");
    return ra->len;
}


The function is written this way for convenience so that I do not need
to check and handle the error (and this is where the cost is, not the
check itself).
This is the whole point and value of the call.
You just asking to make it less usable and force the caller to add more
code in the error handling for now reason.
IMO it is a wrong approach.


>  
_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/

[Stephen Gallagher]

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

On 12/22/2009 03:03 PM, Dmitri Pal wrote:
>  
> >
> >> I do not see why anything should be changed here.
> >
> > The problem is the use of "ret" in both the return from the function and
> > as the memory location for the copied code. It's the same variable
> > having data returned to it in two different ways from the same function
> > call. This is dangerous. It should look like
> >
> > ret1 = *((char **)ref_array_get(ra2, i, &ret2));
> > if (strcmp(ret1, ret2) != 0) {
> >  //FAIL
> > }
> >
> >
>  Ah now I understand what you are talking about.
>  Yes I agree it is generally dangerous and should be discouraged but the
>  unit-test  checks that even if it is done this way it is Ok.
>  I should probably just add a comment about it to discourage this kind of
>  use and explain that this is a test to validate that it will not crash
>  if it is done this way.
>   
Fine, but please implement additionally the test I described above, as
it will prove that the two return values are equivalent.

>  
> >> When you have time to do this.
> >> I have a bigger fish to fry.
> >> I am not arguing that these changes would make sense.
> >> I argue that these changes are not required for the first patch that
> >> introduces the interface.
> >> If can't be expected to be perfect from get go.
> >> It should be good enough to be able to move on with main goals.
> >> Just a difference in the approach.
> >
> > The whole purpose of a review is so that only stable code reaches the
> > repository (hopefully).
> >
>  
>  IMO this is a flaw in the process.
>   
Stable code is not a flaw :)

>  
> >
> >> Hm? I do not buy it. If you want to test the validity of the object you
> >> check if it is NULL rather then call a length function.
> >> The whole point of such simplified inteface is that you check for the
> >> object validity once at the top and then just get it length when you
> >> need to.
> >
> >> something like
> >
> >> int myfun(ref_array *ra)
> >> {
> >>     ...
> >>    if (!ra) {
> >>        ...
> >>        return EINVAL;
> >>     }
> >
> >>     len = ref_array_len();
> >
> >>     ....
> >> }
> >
> >>  Now why check for ra twice and add overhead of error handling to a very
> >> simple function that returns a property of the object?
> >>  It is like a get method in C++. It just returns value.
> >
> > I understand what you're saying, but you're attempting to enforce a
> > coding standard in a function prototype. You can't know how a consumer
> > of the API is going to use this call, so you have to protect against it.
> > A NULL check is a very cheap operation.
> >
> >
>  
>  This is exactly what I do internally:
>  
>  /* Get length */
>  uint32_t ref_array_len(struct ref_array *ra)
>  {
>      TRACE_FLOW_STRING("ref_array_len", "Entry");
>  
>      if (!ra) {
>          TRACE_ERROR_STRING("Uninitialized argument.", "");
>          return 0;
>      }
>  
>      TRACE_FLOW_STRING("ref_array_len", "Exit");
>      return ra->len;
>  }
>  
>  
>  The function is written this way for convenience so that I do not need
>  to check and handle the error (and this is where the cost is, not the
>  check itself).
>  This is the whole point and value of the call.
>  You just asking to make it less usable and force the caller to add more
>  code in the error handling for now reason.
>  IMO it is a wrong approach.
>   
How about setting errno=EINVAL in the if(!ra) block? If the caller
cares, they can check its value if they get zero back.

I'm not a fan of errno, but this might fit your workflow better as you
describe it.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAksxMA4ACgkQeiVVYja6o6NXfwCbB3z0UE7K5FYIqDcGMoCKoJIY
> +DwAn2MwEIxrto/qNd8vkAfOm4wI/QHY
> =44zO
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Hi,

* I am not resending the COLLECTION patch. Ticket #350 has been open to
foster further discussion about the issue.
* The refarray is now made common and the attached (or should I say
attacked? :-) ) patch addresses ALL issues that have been brought up.

The ELAPI patch will be sent later. I need to update it to factor in the
change in the location of the ref-array.
Also I checked that the patch will get much smaller if I ask it to track
deletes or moves/copies.
It actually gets twice smaller.

It is really not that big (read bad :-) ). It has about 2000-3000 lines
of new code thanks for async processing (not that I wanted it to happen
:-) )...
But all the rest is just due to the cleanup. For example the module
elapi_dispatcher had too many functions that were not related to
dispatcher so they got moved around to other modules.
Same with elapi_sink, elapi_log etc.
Now the code is much better arranged in layers and objects.

Also the wiki is your helper in review. It has now the write up about
ELAPI that I created before I submitted this patch last time.


Happy holidays!
Talk to you next year!

Dmitri

Stephen Gallagher wrote:
>  On 12/22/2009 03:03 PM, Dmitri Pal wrote:
>  >>> I do not see why anything should be changed here.
>  >> The problem is the use of "ret" in both the return from the
>  function and
>  >> as the memory location for the copied code. It's the same variable
>  >> having data returned to it in two different ways from the same function
>  >> call. This is dangerous. It should look like
>  >>
>  >> ret1 = *((char **)ref_array_get(ra2, i, &ret2));
>  >> if (strcmp(ret1, ret2) != 0) {
>  >>  //FAIL
>  >> }
>  >>
>  >>
>  > Ah now I understand what you are talking about.
>  > Yes I agree it is generally dangerous and should be discouraged but the
>  > unit-test  checks that even if it is done this way it is Ok.
>  > I should probably just add a comment about it to discourage this kind of
>  > use and explain that this is a test to validate that it will not crash
>  > if it is done this way.
>
>
>  Fine, but please implement additionally the test I described above, as
>  it will prove that the two return values are equivalent.
>
>  >>> When you have time to do this.
>  >>> I have a bigger fish to fry.
>  >>> I am not arguing that these changes would make sense.
>  >>> I argue that these changes are not required for the first patch that
>  >>> introduces the interface.
>  >>> If can't be expected to be perfect from get go.
>  >>> It should be good enough to be able to move on with main goals.
>  >>> Just a difference in the approach.
>  >> The whole purpose of a review is so that only stable code reaches the
>  >> repository (hopefully).
>  >>
>  > IMO this is a flaw in the process.
>
>
>  Stable code is not a flaw :)
>
>  >>> Hm? I do not buy it. If you want to test the validity of the
>  object you
>  >>> check if it is NULL rather then call a length function.
>  >>> The whole point of such simplified inteface is that you check for the
>  >>> object validity once at the top and then just get it length when you
>  >>> need to.
>  >>> something like
>  >>> int myfun(ref_array *ra)
>  >>> {
>  >>>     ...
>  >>>    if (!ra) {
>  >>>        ...
>  >>>        return EINVAL;
>  >>>     }
>  >>>     len = ref_array_len();
>  >>>     ....
>  >>> }
>  >>>  Now why check for ra twice and add overhead of error handling to
>  a very
>  >>> simple function that returns a property of the object?
>  >>>  It is like a get method in C++. It just returns value.
>  >> I understand what you're saying, but you're attempting to enforce a
>  >> coding standard in a function prototype. You can't know how a consumer
>  >> of the API is going to use this call, so you have to protect
>  against it.
>  >> A NULL check is a very cheap operation.
>  >>
>  >>
>  > This is exactly what I do internally:
>
>  > /* Get length */
>  > uint32_t ref_array_len(struct ref_array *ra)
>  > {
>  >     TRACE_FLOW_STRING("ref_array_len", "Entry");
>
>  >     if (!ra) {
>  >         TRACE_ERROR_STRING("Uninitialized argument.", "");
>  >         return 0;
>  >     }
>
>  >     TRACE_FLOW_STRING("ref_array_len", "Exit");
>  >     return ra->len;
>  > }
>
>
>  > The function is written this way for convenience so that I do not need
>  > to check and handle the error (and this is where the cost is, not the
>  > check itself).
>  > This is the whole point and value of the call.
>  > You just asking to make it less usable and force the caller to add more
>  > code in the error handling for now reason.
>  > IMO it is a wrong approach.
>
>
>  How about setting errno=EINVAL in the if(!ra) block? If the caller
>  cares, they can check its value if they get zero back.
>
>  I'm not a fan of errno, but this might fit your workflow better as you
>  describe it.
>  
_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/

[Dmitri Pal]

Dmitri Pal wrote:
>  Hi,
>
>  * I am not resending the COLLECTION patch. Ticket #350 has been open to
>  foster further discussion about the issue.
>     
I mean that it can be acked and pushed as is and we can look at the
issue separately.

>  * The refarray is now made common and the attached (or should I say
>  attacked? :-) ) patch addresses ALL issues that have been brought up.
>
>  The ELAPI patch will be sent later. I need to update it to factor in the
>  change in the location of the ref-array.
>  Also I checked that the patch will get much smaller if I ask it to track
>  deletes or moves/copies.
>  It actually gets twice smaller.
>
>  It is really not that big (read bad :-) ). It has about 2000-3000 lines
>  of new code thanks for async processing (not that I wanted it to happen
>  :-) )...
>  But all the rest is just due to the cleanup. For example the module
>  elapi_dispatcher had too many functions that were not related to
>  dispatcher so they got moved around to other modules.
>  Same with elapi_sink, elapi_log etc.
>  Now the code is much better arranged in layers and objects.
>
>  Also the wiki is your helper in review. It has now the write up about
>  ELAPI that I created before I submitted this patch last time.
>
>
>  Happy holidays!
>  Talk to you next year!
>
>  Dmitri
>
>  Stephen Gallagher wrote:
>    
> > On 12/22/2009 03:03 PM, Dmitri Pal wrote:
> >     
> >>>> I do not see why anything should be changed here.
> >>>>           
> >>> The problem is the use of "ret" in both the return from the
> >>>         
> > function and
> >     
> >>> as the memory location for the copied code. It's the same variable
> >>> having data returned to it in two different ways from the same function
> >>> call. This is dangerous. It should look like
> >>>
> >>> ret1 = *((char **)ref_array_get(ra2, i, &ret2));
> >>> if (strcmp(ret1, ret2) != 0) {
> >>>  //FAIL
> >>> }
> >>>
> >>>
> >>>         
> >> Ah now I understand what you are talking about.
> >> Yes I agree it is generally dangerous and should be discouraged but the
> >> unit-test  checks that even if it is done this way it is Ok.
> >> I should probably just add a comment about it to discourage this kind of
> >> use and explain that this is a test to validate that it will not crash
> >> if it is done this way.
> >>       
> > Fine, but please implement additionally the test I described above, as
> > it will prove that the two return values are equivalent.
> >
> >     
> >>>> When you have time to do this.
> >>>> I have a bigger fish to fry.
> >>>> I am not arguing that these changes would make sense.
> >>>> I argue that these changes are not required for the first patch that
> >>>> introduces the interface.
> >>>> If can't be expected to be perfect from get go.
> >>>> It should be good enough to be able to move on with main goals.
> >>>> Just a difference in the approach.
> >>>>           
> >>> The whole purpose of a review is so that only stable code reaches the
> >>> repository (hopefully).
> >>>
> >>>         
> >> IMO this is a flaw in the process.
> >>       
> > Stable code is not a flaw :)
> >
> >     
> >>>> Hm? I do not buy it. If you want to test the validity of the
> >>>>           
> > object you
> >     
> >>>> check if it is NULL rather then call a length function.
> >>>> The whole point of such simplified inteface is that you check for the
> >>>> object validity once at the top and then just get it length when you
> >>>> need to.
> >>>> something like
> >>>> int myfun(ref_array *ra)
> >>>> {
> >>>>     ...
> >>>>    if (!ra) {
> >>>>        ...
> >>>>        return EINVAL;
> >>>>     }
> >>>>     len = ref_array_len();
> >>>>     ....
> >>>> }
> >>>>  Now why check for ra twice and add overhead of error handling to
> >>>>           
> > a very
> >     
> >>>> simple function that returns a property of the object?
> >>>>  It is like a get method in C++. It just returns value.
> >>>>           
> >>> I understand what you're saying, but you're attempting to enforce a
> >>> coding standard in a function prototype. You can't know how a consumer
> >>> of the API is going to use this call, so you have to protect
> >>>         
> > against it.
> >     
> >>> A NULL check is a very cheap operation.
> >>>
> >>>
> >>>         
> >> This is exactly what I do internally:
> >>       
> >> /* Get length */
> >> uint32_t ref_array_len(struct ref_array *ra)
> >> {
> >>     TRACE_FLOW_STRING("ref_array_len", "Entry");
> >>       
> >>     if (!ra) {
> >>         TRACE_ERROR_STRING("Uninitialized argument.", "");
> >>         return 0;
> >>     }
> >>       
> >>     TRACE_FLOW_STRING("ref_array_len", "Exit");
> >>     return ra->len;
> >> }
> >>       
> >     
> >> The function is written this way for convenience so that I do not need
> >> to check and handle the error (and this is where the cost is, not the
> >> check itself).
> >> This is the whole point and value of the call.
> >> You just asking to make it less usable and force the caller to add more
> >> code in the error handling for now reason.
> >> IMO it is a wrong approach.
> >>       
> > How about setting errno=EINVAL in the if(!ra) block? If the caller
> > cares, they can check its value if they get zero back.
> >
> > I'm not a fan of errno, but this might fit your workflow better as you
> > describe it.
> >
> >     
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>    
>  ------------------------------------------------------------------------
>
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)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/

[Stephen Gallagher]

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

On 12/23/2009 03:59 PM, Dmitri Pal wrote:
>  Hi,
>  
>  * I am not resending the COLLECTION patch. Ticket #350 has been open to
>  foster further discussion about the issue.
>  * The refarray is now made common and the attached (or should I say
>  attacked? :-) ) patch addresses ALL issues that have been brought up.
>  
>  The ELAPI patch will be sent later. I need to update it to factor in the
>  change in the location of the ref-array.
>  Also I checked that the patch will get much smaller if I ask it to track
>  deletes or moves/copies.
>  It actually gets twice smaller.
>  
>  It is really not that big (read bad :-) ). It has about 2000-3000 lines
>  of new code thanks for async processing (not that I wanted it to happen
>  :-) )...
>  But all the rest is just due to the cleanup. For example the module
>  elapi_dispatcher had too many functions that were not related to
>  dispatcher so they got moved around to other modules.
>  Same with elapi_sink, elapi_log etc.
>  Now the code is much better arranged in layers and objects.
>  
>  Also the wiki is your helper in review. It has now the write up about
>  ELAPI that I created before I submitted this patch last time.
>  
>  
>  Happy holidays!
>  Talk to you next year!
>  
>  Dmitri
>   
Nack.

Line 282 should be:
printf("Element = %u\n", (unsigned int)ra->elsize);
Otherwise you're throwing a compilation warning:

../../../common/refarray/ref_array.c: In function ‘ref_array_debug’:
../../../common/refarray/ref_array.c:282: warning: format ‘%u’ expects
type ‘unsigned int’, but argument 2 has type ‘size_t’

ref_array_len is still completely broken. Now, instead of returning 0 on
error, you're returning 22. This is even worse.

Please either use the approach I showed in an earlier email, or return 0
and set errno=EINVAL:

    if (!ra) {
        TRACE_ERROR_STRING("Uninitialized argument.", "");
        errno=EINVAL;
        return 0;
    }

This might be the better approach for you, since if the caller doesn't
care about an error, they can just ignore errno.


Almost there. :)

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAktXeMkACgkQeiVVYja6o6PSEgCdFPUY1QY3RlJZUEDNeL78uGH7
> AtcAoIcGqbV5IkSaR+LwE631evpkbgWt
> =VDXk
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Stephen Gallagher wrote:
>  On 12/23/2009 03:59 PM, Dmitri Pal wrote:
>  > Hi,
>
>  > * I am not resending the COLLECTION patch. Ticket #350 has been open to
>  > foster further discussion about the issue.
>  > * The refarray is now made common and the attached (or should I say
>  > attacked? :-) ) patch addresses ALL issues that have been brought up.
>
>  > The ELAPI patch will be sent later. I need to update it to factor in the
>  > change in the location of the ref-array.
>  > Also I checked that the patch will get much smaller if I ask it to track
>  > deletes or moves/copies.
>  > It actually gets twice smaller.
>
>  > It is really not that big (read bad :-) ). It has about 2000-3000 lines
>  > of new code thanks for async processing (not that I wanted it to happen
>  > :-) )...
>  > But all the rest is just due to the cleanup. For example the module
>  > elapi_dispatcher had too many functions that were not related to
>  > dispatcher so they got moved around to other modules.
>  > Same with elapi_sink, elapi_log etc.
>  > Now the code is much better arranged in layers and objects.
>
>  > Also the wiki is your helper in review. It has now the write up about
>  > ELAPI that I created before I submitted this patch last time.
>
>
>  > Happy holidays!
>  > Talk to you next year!
>
>  > Dmitri
>
>
>  Nack.
>
>  Line 282 should be:
>  printf("Element = %u\n", (unsigned int)ra->elsize);
>  Otherwise you're throwing a compilation warning:
>
>  ../../../common/refarray/ref_array.c: In function ref_array_debug:
>  ../../../common/refarray/ref_array.c:282: warning: format %u expects
>  type unsigned int, but argument 2 has type size_t
>  
Interesting. It does not produce this warning on my system...
I made the change anyways though I have seen this issue before and then
it went away.
May be it was different part of code.


>  ref_array_len is still completely broken. Now, instead of returning 0
> on
>  error, you're returning 22. This is even worse.
>
>  Please either use the approach I showed in an earlier email, or return 0
>  and set errno=EINVAL:
>
>      if (!ra) {
>          TRACE_ERROR_STRING("Uninitialized argument.", "");
>          errno=EINVAL;
>          return 0;
>      }
>
>  This might be the better approach for you, since if the caller doesn't
>  care about an error, they can just ignore errno.
>
>  
Uhhhh. Good catch!


>  Almost there. :)
>  
_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/

[Dmitri Pal]

Dmitri Pal wrote:
>  Stephen Gallagher wrote:
>    
> > On 12/23/2009 03:59 PM, Dmitri Pal wrote:
> >     
> >> Hi,
> >>       
> >> * I am not resending the COLLECTION patch. Ticket #350 has been open to
> >> foster further discussion about the issue.
> >> * The refarray is now made common and the attached (or should I say
> >> attacked? :-) ) patch addresses ALL issues that have been brought up.
> >>       
> >> The ELAPI patch will be sent later. I need to update it to factor in the
> >> change in the location of the ref-array.
> >> Also I checked that the patch will get much smaller if I ask it to track
> >> deletes or moves/copies.
> >> It actually gets twice smaller.
> >>       
> >> It is really not that big (read bad :-) ). It has about 2000-3000 lines
> >> of new code thanks for async processing (not that I wanted it to happen
> >> :-) )...
> >> But all the rest is just due to the cleanup. For example the module
> >> elapi_dispatcher had too many functions that were not related to
> >> dispatcher so they got moved around to other modules.
> >> Same with elapi_sink, elapi_log etc.
> >> Now the code is much better arranged in layers and objects.
> >>       
> >> Also the wiki is your helper in review. It has now the write up about
> >> ELAPI that I created before I submitted this patch last time.
> >>       
> >     
> >> Happy holidays!
> >> Talk to you next year!
> >>       
> >> Dmitri
> >>       
> > Nack.
> >
> > Line 282 should be:
> > printf("Element = %u\n", (unsigned int)ra->elsize);
> > Otherwise you're throwing a compilation warning:
> >
> > ../../../common/refarray/ref_array.c: In function ref_array_debug:
> > ../../../common/refarray/ref_array.c:282: warning: format %u expects
> > type unsigned int, but argument 2 has type size_t
> >
> >     
>
>  Interesting. It does not produce this warning on my system...
>  I made the change anyways though I have seen this issue before and then
>  it went away.
>  May be it was different part of code.
>
>
>    
> > ref_array_len is still completely broken. Now, instead of returning 0 on
> > error, you're returning 22. This is even worse.
> >
> > Please either use the approach I showed in an earlier email, or return 0
> > and set errno=EINVAL:
> >
> >     if (!ra) {
> >         TRACE_ERROR_STRING("Uninitialized argument.", "");
> >         errno=EINVAL;
> >         return 0;
> >     }
> >
> > This might be the better approach for you, since if the caller doesn't
> > care about an error, they can just ignore errno.
> >
> >
> >     
>  Uhhhh. Good catch!
>
>
>     
Forgot to mention that the patch was attached.


> > Almost there. :)
> >
> >     
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>    
>  ------------------------------------------------------------------------
>
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)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/

[Stephen Gallagher]

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

On 01/20/2010 11:43 PM, Dmitri Pal wrote:

>  
>  Forgot to mention that the patch was attached.
>  
>   
Ack

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAktYsicACgkQeiVVYja6o6MNXwCfeB/FKjS0sKHtO4Ikhu4pAQDW
> kG0An0Hb3SfPT35LHduTAy6z7vW3+zDx
> =QlwH
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

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

On 01/21/2010 02:59 PM, Stephen Gallagher wrote:
>  On 01/20/2010 11:43 PM, Dmitri Pal wrote:
>
>
> > Forgot to mention that the patch was attached.
>
>
>
>  Ack 
Pushed to master, fixing one merge conflict with the new VERSION
mechanism.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAktYs0sACgkQeiVVYja6o6OHFACeIHcZhX87NSOzAFDQPQCXPELy
> iaAAn0r+SiL6pT2Zam4IW5Dlii/JdZkb
> =rgLz
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Stephen Gallagher wrote:
>  On 01/21/2010 02:59 PM, Stephen Gallagher wrote:
>  > On 01/20/2010 11:43 PM, Dmitri Pal wrote:
>
>
>  >> Forgot to mention that the patch was attached.
>
>
>  > Ack
>
>  Pushed to master, fixing one merge conflict with the new VERSION
>  mechanism.
>  
Thanks!
Do not forget about the Collection patch.
It has been reviewed I think twice but has not been committed yet.


_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/