crossbow discuss - Aug 2008 - Crossbow Project Phase I of code review starts today

(networking-discuss Bcc''ed)

Good evening,

The Crossbow team would like to invite you to participate in the 
project''s code review.

The first phase of the review starts today, and covers the library 
changes as well as the commands.
The remaining phases will follow in the next few weeks.

The webrev and links to the latest versions of the man pages included in 
this phase are available in
http://www.opensolaris.org/os/project/crossbow/Code-review1

For a refresher about the features and use cases of the project, we 
recommend the updated documents,
http://www.opensolaris.org/os/project/crossbow/Docs/

Interested? please send the code review comments by September 8th, 2008

Thanks,
    The Crossbow team.

Kais,

I just looked at some code in mac.c and noticed you''re passing flags as
uint16_t
(mac_client_open, mac_client_close, etc.)

Unless there is a specific hardware reason for using 16bits, all flags 
structures
should be 32bit.  There''s no performance penalty in doing this as
nearly all
memory accesses these days are 32bits anyway. When passing things between
functions, especially on sparc, the minimum word size passed is 32bits, so
there is nothing to gain by passing "only 16".

You also have many more bits available for later use.

ok, so strictly speaking this isn''t a phase 1 code review comment but 
... :*)

Cheers,
Darren

Hi Darren,

Thanks for the review.

we''ll take your comment into account when preparing for phase II.

    Kais.

On 09/02/08 18:01, Darren Reed wrote:
> Kais,
>
> I just looked at some code in mac.c and noticed you''re passing
flags as
> uint16_t
> (mac_client_open, mac_client_close, etc.)
>
> Unless there is a specific hardware reason for using 16bits, all flags 
> structures
> should be 32bit.  There''s no performance penalty in doing this as
nearly all
> memory accesses these days are 32bits anyway. When passing things between
> functions, especially on sparc, the minimum word size passed is 32bits, so
> there is nothing to gain by passing "only 16".
>
> You also have many more bits available for later use.
>
> ok, so strictly speaking this isn''t a phase 1 code review comment
but
> ... :*)
>
> Cheers,
> Darren
>
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
>
>

Some comments on flowadm.c
- the field printing code and structures look very generic,
  why aren''t they in a library for common use?
- 1258-1268 consider rewriting die() to use verrx()
- 594-631 #if_0''d code...remove
- 646,811,1425 "else" is redundant
- 330 argv[0] is pathname, not progname...see err.c''s __progname and
6735446
- 705 consider removing goto and putting 706-713 inside successful test of
      DLADM_STATUS_OK
- in many places there are fprintf(stderr, gettext(...), progname...), 
shouldn''t
  these all be made to die()?
- 1224 warn() is now in libc, consider another name for this function to 
avoid confusion

On 09/03/08 15:20, Darren Reed wrote:
> Some comments on flowadm.c
> - the field printing code and structures look very generic,
>   why aren''t they in a library for common use?
>   
We will look into moving the field printing code used by flowadm
and dladm into libdladm.  Eventually, this functionality may merit
a library of its own.
> - 1258-1268 consider rewriting die() to use verrx()
>   
We will look into the advantages of using verrx() in die().  This also
applies to dladm.c
> - 594-631 #if_0''d code...remove
>   
Agreed.
> - 646,811,1425 "else" is redundant
>   
646, 811 Walker functions must return DLADM_WALK_CONTINUE or
DLADM_WALK_TERMINATE to work properly. 

1425:  I agree that this is redundant.
> - 330 argv[0] is pathname, not progname...see err.c''s __progname
and 6735446
>   
Agreed.  This also applies to dladm.c
> - 705 consider removing goto and putting 706-713 inside successful test of
>       DLADM_STATUS_OK
>   
OK.  gotos should generally be avoided.
> - in many places there are fprintf(stderr, gettext(...), progname...), 
> shouldn''t
>   these all be made to die()?
>   
Most of these can be converted to use die().  We could also create a 
die_dupopt() for
duplicate option specification.
> - 1224 warn() is now in libc, consider another name for this function to 
> avoid confusion
>   
I''d prefer to use the libc warn() directly with gettext ().

  warn(gettext("kstat open operation failed"));

We''d use the libc warn() and verr() directly if they provided for 
localized strings.  Perhaps
there should be and RFE against libc to provide these as well?

-Mike

>   warn(gettext("kstat open operation failed"));
Be careful with that, as lint can''t see inside the gettext() strings
and
thus you will lose format-string checking by default.  Also, generally
speaking, cluttering all of the warn()/die() calls with gettext() crud is
a real eyesore.  All of this is why dladm has gettext() centralized in the
warn()/die() implementations themselves.

-- 
meem

On 09/04/08 17:01, Peter Memishian wrote:
>  >   warn(gettext("kstat open operation failed"));
>
> Be careful with that, as lint can''t see inside the gettext()
strings and
> thus you will lose format-string checking by default.  Also, generally
> speaking, cluttering all of the warn()/die() calls with gettext() crud is
> a real eyesore.  All of this is why dladm has gettext() centralized in the
> warn()/die() implementations themselves.
>
>   
I agree with the eyesore comment.  I was just looking around to see how 
other
cmds dealt with localized warnings.  While the original comment was directed
towards flowadm.c, it could also apply to dladm.c as well.

Here are the options

1) keep the localized version of warn() and eventually add a localized 
version
to libc.

2) change the name to warning() and have it call gettext() and the libc 
warn().

3) change calls to local warn to use gettext() in the argument list 
(eyesore).

I prefer 1).

-Mike

> > Be careful with that, as lint can''t see inside the gettext()
strings and
 > > thus you will lose format-string checking by default.  Also,
generally
 > > speaking, cluttering all of the warn()/die() calls with gettext()
crud is
 > > a real eyesore.  All of this is why dladm has gettext() centralized
in the
 > > warn()/die() implementations themselves.
 >
 > I agree with the eyesore comment.  I was just looking around to see how
 > other cmds dealt with localized warnings.  While the original comment
 > was directed towards flowadm.c, it could also apply to dladm.c as well.

Indeed.

 > Here are the options
 > 
 > 1) keep the localized version of warn() and eventually add a localized 
 > version to libc.
 > 
 > 2) change the name to warning() and have it call gettext() and the libc 
 > warn().
 > 
 > 3) change calls to local warn to use gettext() in the argument list 
 > (eyesore).
 > 
 > I prefer 1).

For (2), it''d have to call vwarn() (since there''s no way to
directly call
another varargs function), and it would end up just as complex as the
current warn() implementation.  So I agree that (1) is best.  The libc
routines are nice to have around, but here they just don''t meet the
needs
of the program.

-- 
meem