|
Replies:
11
-
Last Post:
Aug 17, 2009 11:05 AM
by: meem
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
[nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Jul 29, 2009 12:27 PM
|
|
Webrev is at:
http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
Most of the comments have been accepted and won't be talked about here.
The ones that aren't are detailed below. (All code-review comments are
at http://wikis.sun.com/display/NWAM/Phase+1+Code+Review+Feedback)
Also, note that the usage of nwamadm(1M) has changed since the
code-review went out. Furthermore, the line numbers have changed. Refer
to the original webrev at http://cr.opensolaris.org/~mph/nwam1_cr/ for
line numbers.
bash-3.2# nwamadm -?
nwamadm: unknown subcommand '-?'
usage: <subcommand> <args> ...
help
enable [-p <profile-type>] [-c <ncu-class>] <object-name>
disable [-p <profile-type>] [-c <ncu-class>] <object-name>
list [-p <profile-type>] [-c <ncu-class>] [<object-name>]
show-events
scan-wifi <link-name>
select-wifi <link-name>
Most important is that the event monitoring and interact functionality
have been separated, with "interact" being renamed "show-events"
> meem-010 nwamadm.c'Throughout: the ofmt API needs to be used to print
> stuff out rather than rolling your own printing logic. This will also
> provide parsable mode and user-selectable fields for free (which
> currently seem to be missing).
ofmt came after nwamadm was already written. It is definitely a good
idea to use that API. However, not at this time. It has been filed as an
RFE 10341 (http://defect.opensolaris.org/bz/show_bug.cgi?id=10341).
> meem-011 nwamadm.c'Throughout: what's up with the name zerr()? Am I to
> assume it's because this code started life as zoneadm.c? Seems like it
> should be called die(), and seems like you could also use a
> die_nwamerr() function that takes a nwamerr and does the
> nwam_strerror() a la die_dlerr() in dladm.
die(), die_nwamerr() and die_usage() have been introduced in place of
zerr().
> meem-012 nwamadm.c'Throughout: seems a shame that the libnwam walker
> APIs require a non-NULL cb_ret argument, as nothing here seems to want
> to consult it. Why not allow it to be NULL?
libnwam walker API's will be changed to handle NULL cb_ret arguments.
Bug 10362 (http://defect.opensolaris.org/bz/show_bug.cgi?id=10362) has
been filed to fix this separately from these code-review fixes.
> meem-014 nwamadm.c'58, 64-72: The use of both command numbers *and* an
> array of command entries seems muddled. Seems like all the
> functionality tied to command numbers could be folded into state
> tracked in the command entries – e.g., a cmd_help pointer-to-function
> (or just a simple const char *), and a flags field that indicates
> whether e.g. "-x" can be used or whether the type needs to be determined.
I am unclear about this. The command numbers make passing the command
being handled around easy (rather than using the strings), and the array
is the one place that lists attributes of the command.
> meem-018 nwamadm.c'151: Unclear why this is a global; seems like it
> should be declared in interact_func() and passed to eventhandler().
> meem-049 nwamadm.c'1260-1475: Is this intended as a debugging aid or
> something that end-users would actually use? If the latter, seems like
> some I18N handling is missing in the various prompts. Also, how is the
> end-user supposed to make sense of the stuff output from this?
> Further, it seems odd to me that the interactive support and event
> monitor have been glommed together into one facility.
> meem-058 nwamadm.c'1515-1528: Odd code here seems to stem from mixing
> interaction and event monitoring.
event monitoring (show-events) and user interaction (select-wifi) have
been separated into separate subcommands.
> meem-019 nwamadm.c'167-277: I'd expect these helper routines to be in
> libnwam rather than nwamadm. Some of them already appear to be there,
> such as nwam_ncu_type_to_flag(). Why are they here?
Moved to libnwam, except for obj_type_to_str(). The libnwam version of
this (nwam_object_type_to_string()) returns string in UPPERCASE, which
we don't want here. The CLI uses lowercase, so this function is still
around.
> meem-020 nwamadm.c'179, 193, 215, 230, 246, 262, 275, 297: The default
> cases here seem unhelpful. I'd expect strings of "?" and enumerations
> along the lines of NWAM_OBJECT_TYPE_UNKNOWN.
most of these have moved to libnwam. I also renamed _NONE enum to _UNKNOWN.
> meem-024 nwamadm.c'378: Why is it safe to ignore the return value from
> this function? Is it that we assume `state' will only be modified by
> this function call upon success? Seems a needless assumption.
"state" starts out UNINITIALIZED and if the libnwam call fails, state
doesn't change. It is changed only on success.
> meem-025 nwamadm.c'451: Would seem more natural to return `type' and
> remove the `num' parameter in place of an NWAM_OBJECT_TYPE_UNKNOWN value.
"num" parameter is used to determine how many objects with the same name
exists. This allows for useful error reporting (multiple objects matched
vs. no objects matched).
> meem-029 nwamadm.c'653-684: This should be factored out into a
> funciton that takes the type and then is merely called here for
> NWAM_NCU_TYPE_LINK and NWAM_NCU_TYPE_INTERFACE. For bonus points,
> could take a function pointer which would be either &nwam_ncu_enable
> or &nwam_ncu_disable.
created ncu|enm|loc_action() which take a boolean to determine whether
enable or disable is being done and appropriate libnwam functions are
called. makes enable_func() and disable_func() much slimmer.
Also, for NCUs, if no type is given and there are multiple NCUs with the
same name, then ncu_action() is called again with each _LINK and
_INTERFACE, thus no code duplication.
> meem-032 nwamadm.c'719: By the time we get here isn't the enable
> complete? That is, seems like this should be "Enabled", not "Enabling".
not really. nwamadm is asynchronous (like svcadm), so the enabling is
going on in the background.
> meem-033 nwamadm.c'724: `if' block should be replaced with assert(ret
> != NWAM_SUCCESS)' and the code should be unindented.
why assert()? changed to switch block for all the return values (see
enable|disable_func())
> meem-035 nwamadm.c'871: Would seem preferable to return a error code
> indicating whether the function succeeded rather than overloading
> *_UNINITALIZED for the job.
If we can't get the state, then it is UNINITIALIZED. No need to check if
function succeeded or not.
> meem-039 nwamadm.c'956: p_last_entry is gross. Does the order of the
> list matter? If not, why not prepend to the list? If so, would prefer
> to make the list circular and use p_prev.
p_last_entry removed. The linked-list is prepended. walks are done in
reverse order (enm, loc, ncu, ncp) to keep the output order the same.
ordering within each object type shouldn't matter.
> meem-043 nwamadm.c'1108-1156: See previous comments about using ofmt.
> As an aside, the field names are generally considered part of the
> command itself and are not something we translate – and this is
> important when -o <field> is supported.
> meem-057 nwamadm.c'1511: See previous comment about I18N with field
> names.
field name are not gettext()ified.
> meem-046 nwamadm.c'1207, 1219, 1227, 1234: Needless cast.
declared name as char *.
> meem-051 nwamadm.c'1338, 1346, 1388: This works without fflush(stdout)?
strangely, worked without fflush(stdout). But I've added it.
> meem-052 nwamadm.c'1338, 1346, 1388: Can the user ^C out? If so, how
> does that work? If not, seems a usability problem.
program exits, nothing happens.
> meem-053 nwamadm.c'1366, 1402: Why are errors going to stdout?
errors are sent to stderr with die() or die_nwamerr().
> meem-055 nwamadm.c'1432: How do you know this is a sockaddr_in? Sure
> looks like this code can be reached with a sockeddr_in6.
inet_ntop() used instead.
> meem-060 nwamadm.c'1551-1554: This means one cannot observe the events
> at NWAM startup with "interact" which seems unfortunate.
true. event monitoring needs to send a door call to nwamd. not possible
with nwam not running.
> meem-061 nwamadm.c'1531: So using the "interact" subcommand always
> exits with a non-zero status?
true. Since nwam_event_wait() is an infinitely loop, if we're out of the
loop, means there's an error.
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
Posts:
3,083
From:
US
Registered:
3/9/05
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 6, 2009 11:33 PM
in response to: anurag_m
|
|
> http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
Anurag,
I didn't go over the new webrev in detail, but I see you managed to shrink
the code by 15% which is great. Responses to your remaining items are
below.
> Most of the comments have been accepted and won't be talked about here.
> The ones that aren't are detailed below. (All code-review comments are
> at http://wikis.sun.com/display/NWAM/Phase+1+Code+Review+Feedback)
>
> Also, note that the usage of nwamadm(1M) has changed since the
> code-review went out. Furthermore, the line numbers have changed. Refer
> to the original webrev at http://cr.opensolaris.org/~mph/nwam1_cr/ for
> line numbers.
>
> bash-3.2# nwamadm -?
> nwamadm: unknown subcommand '-?'
> usage: <subcommand> <args> ...
> help
> enable [-p <profile-type>] [-c <ncu-class>] <object-name>
> disable [-p <profile-type>] [-c <ncu-class>] <object-name>
> list [-p <profile-type>] [-c <ncu-class>] [<object-name>]
> show-events
> scan-wifi <link-name>
> select-wifi <link-name>
>
> Most important is that the event monitoring and interact functionality
> have been separated, with "interact" being renamed "show-events"
OK. As an aside, it seems weird that some subcommands are <verb>-<noun>
and others are just <verb>.
> > meem-010 nwamadm.c'Throughout: the ofmt API needs to be used to print
> > stuff out rather than rolling your own printing logic. This will also
> > provide parsable mode and user-selectable fields for free (which
> > currently seem to be missing).
> ofmt came after nwamadm was already written. It is definitely a good
> idea to use that API. However, not at this time. It has been filed as an
> RFE 10341 (http://defect.opensolaris.org/bz/show_bug.cgi?id=10341).
OK.
> > meem-012 nwamadm.c'Throughout: seems a shame that the libnwam walker
> > APIs require a non-NULL cb_ret argument, as nothing here seems to want
> > to consult it. Why not allow it to be NULL?
> libnwam walker API's will be changed to handle NULL cb_ret arguments.
> Bug 10362 (http://defect.opensolaris.org/bz/show_bug.cgi?id=10362) has
> been filed to fix this separately from these code-review fixes.
OK.
> > meem-014 nwamadm.c'58, 64-72: The use of both command numbers *and* an
> > array of command entries seems muddled. Seems like all the
> > functionality tied to command numbers could be folded into state
> > tracked in the command entries e.g., a cmd_help pointer-to-function
> > (or just a simple const char *), and a flags field that indicates
> > whether e.g. "-x" can be used or whether the type needs to be determined.
> I am unclear about this. The command numbers make passing the command
> being handled around easy (rather than using the strings), and the array
> is the one place that lists attributes of the command.
My point is that if the code is properly abstracted it will not need to
know the command name because all the semantics will be encoded in its
entry in the command table, and the code will simply use the information
in that command table entry to figure out how to behave.
> > meem-019 nwamadm.c'167-277: I'd expect these helper routines to be in
> > libnwam rather than nwamadm. Some of them already appear to be there,
> > such as nwam_ncu_type_to_flag(). Why are they here?
> Moved to libnwam, except for obj_type_to_str(). The libnwam version of
> this (nwam_object_type_to_string()) returns string in UPPERCASE, which
> we don't want here. The CLI uses lowercase, so this function is still
> around.
Seems a shame to have to maintain this table in two places :-/
> > meem-020 nwamadm.c'179, 193, 215, 230, 246, 262, 275, 297: The default
> > cases here seem unhelpful. I'd expect strings of "?" and enumerations
> > along the lines of NWAM_OBJECT_TYPE_UNKNOWN.
> most of these have moved to libnwam. I also renamed _NONE enum to _UNKNOWN.
OK.
> > meem-024 nwamadm.c'378: Why is it safe to ignore the return value from
> > this function? Is it that we assume `state' will only be modified by
> > this function call upon success? Seems a needless assumption.
> "state" starts out UNINITIALIZED and if the libnwam call fails, state
> doesn't change. It is changed only on success.
My point is that this seemed needlessly fragile, but if this is part of
the interface guarantee that libnwam makes, OK.
> > meem-025 nwamadm.c'451: Would seem more natural to return `type' and
> > remove the `num' parameter in place of an NWAM_OBJECT_TYPE_UNKNOWN value.
> "num" parameter is used to determine how many objects with the same name
> exists. This allows for useful error reporting (multiple objects matched
> vs. no objects matched).
My point was that if you had separate concepts for TYPE_MULTIPLE and
TYPE_NONE you wouldn't need `num'. But seeing as how we don't have that,
at least make either `num' or `type' the return value: it's awkward to
have a function that takes two input-output arguments and returns void.
> > meem-029 nwamadm.c'653-684: This should be factored out into a
> > funciton that takes the type and then is merely called here for
> > NWAM_NCU_TYPE_LINK and NWAM_NCU_TYPE_INTERFACE. For bonus points,
> > could take a function pointer which would be either &nwam_ncu_enable
> > or &nwam_ncu_disable.
> created ncu|enm|loc_action() which take a boolean to determine whether
> enable or disable is being done and appropriate libnwam functions are
> called. makes enable_func() and disable_func() much slimmer.
>
> Also, for NCUs, if no type is given and there are multiple NCUs with the
> same name, then ncu_action() is called again with each _LINK and
> _INTERFACE, thus no code duplication.
Sounds good.
> > meem-032 nwamadm.c'719: By the time we get here isn't the enable
> > complete? That is, seems like this should be "Enabled", not "Enabling".
> not really. nwamadm is asynchronous (like svcadm), so the enabling is
> going on in the background.
Then does this mean that the error message "<foo> is already enabled"
(e.g., line 606) is not necessarily correct? What happens if I try to
disable or enable while an enable/disable is still ongoing?
> > meem-033 nwamadm.c'724: `if' block should be replaced with assert(ret
> > != NWAM_SUCCESS)' and the code should be unindented.
> why assert()? changed to switch block for all the return values (see
> enable|disable_func())
My point was that there should be no way short of programmer error to get
to line 724 with ret == NWAM_SUCCESS.
> > meem-035 nwamadm.c'871: Would seem preferable to return a error code
> > indicating whether the function succeeded rather than overloading
> > *_UNINITALIZED for the job.
> If we can't get the state, then it is UNINITIALIZED. No need to check if
> function succeeded or not.
Then how about making the state the return value?
> > meem-043 nwamadm.c'1108-1156: See previous comments about using ofmt.
> > As an aside, the field names are generally considered part of the
> > command itself and are not something we translate and this is
> > important when -o <field> is supported.
> > meem-057 nwamadm.c'1511: See previous comment about I18N with field
> > names.
> field name are not gettext()ified.
As in you changed it to not be? The version I reviewed had e.g.
`gettext("EVENT")'.
> > meem-046 nwamadm.c'1207, 1219, 1227, 1234: Needless cast.
> declared name as char *.
Then please fix the declaration to have the right type.
> > meem-052 nwamadm.c'1338, 1346, 1388: Can the user ^C out? If so, how
> > does that work? If not, seems a usability problem.
> program exits, nothing happens.
OK, you're just relying on the default disposition of SIGINT, which is
fine.
> > meem-053 nwamadm.c'1366, 1402: Why are errors going to stdout?
> errors are sent to stderr with die() or die_nwamerr().
As in, you fixed it to do that. The version I reviewed used printf().
> > meem-061 nwamadm.c'1531: So using the "interact" subcommand always
> > exits with a non-zero status?
> true. Since nwam_event_wait() is an infinitely loop, if we're out of the
> loop, means there's an error.
As per my comment above regarding SIGINT, as long as the user can ^C and
the return value is zero, I'm happy.
--
meem
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 10, 2009 7:32 AM
in response to: meem
|
|
Thanks for the comments. I've updated the webrev. responses inline
Peter Memishian wrote:
> > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
>
> Anurag,
>
> I didn't go over the new webrev in detail, but I see you managed to shrink
> the code by 15% which is great. Responses to your remaining items are
> below.
>
> > Most of the comments have been accepted and won't be talked about here.
> > The ones that aren't are detailed below. (All code-review comments are
> > at http://wikis.sun.com/display/NWAM/Phase+1+Code+Review+Feedback)
> >
> > Also, note that the usage of nwamadm(1M) has changed since the
> > code-review went out. Furthermore, the line numbers have changed. Refer
> > to the original webrev at http://cr.opensolaris.org/~mph/nwam1_cr/ for
> > line numbers.
> >
>
> > > meem-014 nwamadm.c'58, 64-72: The use of both command numbers *and* an
> > > array of command entries seems muddled. Seems like all the
> > > functionality tied to command numbers could be folded into state
> > > tracked in the command entries – e.g., a cmd_help pointer-to-function
> > > (or just a simple const char *), and a flags field that indicates
> > > whether e.g. "-x" can be used or whether the type needs to be determined.
> > I am unclear about this. The command numbers make passing the command
> > being handled around easy (rather than using the strings), and the array
> > is the one place that lists attributes of the command.
>
> My point is that if the code is properly abstracted it will not need to
> know the command name because all the semantics will be encoded in its
> entry in the command table, and the code will simply use the information
> in that command table entry to figure out how to behave.
>
> > > meem-019 nwamadm.c'167-277: I'd expect these helper routines to be in
> > > libnwam rather than nwamadm. Some of them already appear to be there,
> > > such as nwam_ncu_type_to_flag(). Why are they here?
> > Moved to libnwam, except for obj_type_to_str(). The libnwam version of
> > this (nwam_object_type_to_string()) returns string in UPPERCASE, which
> > we don't want here. The CLI uses lowercase, so this function is still
> > around.
>
> Seems a shame to have to maintain this table in two places :-/
>
The library function nwam_object_type_to_string() has been modified to
return lowercase strings, so the object_type_to_str() function in
nwamadm has been removed.
> > > meem-025 nwamadm.c'451: Would seem more natural to return `type' and
> > > remove the `num' parameter in place of an NWAM_OBJECT_TYPE_UNKNOWN value.
> > "num" parameter is used to determine how many objects with the same name
> > exists. This allows for useful error reporting (multiple objects matched
> > vs. no objects matched).
>
> My point was that if you had separate concepts for TYPE_MULTIPLE and
> TYPE_NONE you wouldn't need `num'. But seeing as how we don't have that,
> at least make either `num' or `type' the return value: it's awkward to
> have a function that takes two input-output arguments and returns void.
>
Understood. changed function to
"nwam_object_type_t determine_object_type(const char *, int *)".
> > > meem-032 nwamadm.c'719: By the time we get here isn't the enable
> > > complete? That is, seems like this should be "Enabled", not "Enabling".
> > not really. nwamadm is asynchronous (like svcadm), so the enabling is
> > going on in the background.
>
> Then does this mean that the error message "<foo> is already enabled"
> (e.g., line 606) is not necessarily correct? What happens if I try to
> disable or enable while an enable/disable is still ongoing?
>
Another enable/disable action is enqueued. The return message will be
the same "enabling ncp <foo>". When nwamd handles the second
enable/disable action, it will see that it has already happened and
won't do anything.
> > > meem-033 nwamadm.c'724: `if' block should be replaced with assert(ret
> > > != NWAM_SUCCESS)' and the code should be unindented.
> > why assert()? changed to switch block for all the return values (see
> > enable|disable_func())
>
> My point was that there should be no way short of programmer error to get
> to line 724 with ret == NWAM_SUCCESS.
>
This code path has changed. So, I think this is a non-issue.
{enable,disable}_func() switches on the return value.
> > > meem-035 nwamadm.c'871: Would seem preferable to return a error code
> > > indicating whether the function succeeded rather than overloading
> > > *_UNINITALIZED for the job.
> > If we can't get the state, then it is UNINITIALIZED. No need to check if
> > function succeeded or not.
>
> Then how about making the state the return value?
>
The function actually returns the state and auxilliary state. I changed
the function to return the nwam_error_t from nwam_*_get_state().
> > > meem-043 nwamadm.c'1108-1156: See previous comments about using ofmt.
> > > As an aside, the field names are generally considered part of the
> > > command itself and are not something we translate – and this is
> > > important when -o <field> is supported.
> > > meem-057 nwamadm.c'1511: See previous comment about I18N with field
> > > names.
> > field name are not gettext()ified.
>
> As in you changed it to not be? The version I reviewed had e.g.
> `gettext("EVENT")'.
>
Yes. The field headers no longer have gettext().
> > > meem-046 nwamadm.c'1207, 1219, 1227, 1234: Needless cast.
> > declared name as char *.
>
> Then please fix the declaration to have the right type.
>
fixed.
> > > meem-053 nwamadm.c'1366, 1402: Why are errors going to stdout?
> > errors are sent to stderr with die() or die_nwamerr().
>
> As in, you fixed it to do that. The version I reviewed used printf().
>
Yes, all errors are now die_nwamerr()
> > > meem-061 nwamadm.c'1531: So using the "interact" subcommand always
> > > exits with a non-zero status?
> > true. Since nwam_event_wait() is an infinitely loop, if we're out of the
> > loop, means there's an error.
>
> As per my comment above regarding SIGINT, as long as the user can ^C and
> the return value is zero, I'm happy.
>
The return value, however, is 130. Looking into where that came from.
Anurag
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
3,083
From:
US
Registered:
3/9/05
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 10, 2009 4:10 PM
in response to: anurag_m
|
|
> > > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
> >
> > > > meem-019 nwamadm.c'167-277: I'd expect these helper routines to be in
> > > > libnwam rather than nwamadm. Some of them already appear to be there,
> > > > such as nwam_ncu_type_to_flag(). Why are they here?
> > > Moved to libnwam, except for obj_type_to_str(). The libnwam version of
> > > this (nwam_object_type_to_string()) returns string in UPPERCASE, which
> > > we don't want here. The CLI uses lowercase, so this function is still
> > > around.
> >
> > Seems a shame to have to maintain this table in two places :-/
> >
> The library function nwam_object_type_to_string() has been modified to
> return lowercase strings, so the object_type_to_str() function in
> nwamadm has been removed.
Sounds good.
> > > > meem-032 nwamadm.c'719: By the time we get here isn't the enable
> > > > complete? That is, seems like this should be "Enabled", not "Enabling".
> > > not really. nwamadm is asynchronous (like svcadm), so the enabling is
> > > going on in the background.
> >
> > Then does this mean that the error message "<foo> is already enabled"
> > (e.g., line 606) is not necessarily correct? What happens if I try to
> > disable or enable while an enable/disable is still ongoing?
> >
> Another enable/disable action is enqueued.
So the processing of each transition is asynchronous but will wait for all
previous transitions to complete before starting?
> > > > meem-035 nwamadm.c'871: Would seem preferable to return a error code
> > > > indicating whether the function succeeded rather than overloading
> > > > *_UNINITALIZED for the job.
> > > If we can't get the state, then it is UNINITIALIZED. No need to check if
> > > function succeeded or not.
> >
> > Then how about making the state the return value?
> >
> The function actually returns the state and auxilliary state. I changed
> the function to return the nwam_error_t from nwam_*_get_state().
... and callers pay attention to that return value now?
> > > > meem-061 nwamadm.c'1531: So using the "interact" subcommand always
> > > > exits with a non-zero status?
> > > true. Since nwam_event_wait() is an infinitely loop, if we're out of the
> > > loop, means there's an error.
> >
> > As per my comment above regarding SIGINT, as long as the user can ^C and
> > the return value is zero, I'm happy.
> >
> The return value, however, is 130. Looking into where that came from.
That's the default exit status for SIGINT (128 + signal number = 130).
--
meem
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 11, 2009 5:53 PM
in response to: meem
|
|
Peter Memishian wrote:
> http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
>
> > > > > meem-032 nwamadm.c'719: By the time we get here isn't the enable
> > > > > complete? That is, seems like this should be "Enabled", not "Enabling".
> > > > not really. nwamadm is asynchronous (like svcadm), so the enabling is
> > > > going on in the background.
> > >
> > > Then does this mean that the error message "<foo> is already enabled"
> > > (e.g., line 606) is not necessarily correct? What happens if I try to
> > > disable or enable while an enable/disable is still ongoing?
> > >
> > Another enable/disable action is enqueued.
>
> So the processing of each transition is asynchronous but will wait for all
> previous transitions to complete before starting?
>
Nope, the second event (let's say enable event is enqueued by the second
command (disable will be the same)) will not wait until the first one
finishes. The first enable event will start the state machine. When
transitioning through the different state, more events are enqueued.
The second enable event may be processed before the states have
stabilized. However, it doesn't cause any change because the first step
in processing these enable event is to see what object is enabled.
Since these are the same, the second enable event will be ignored.
> > > > > meem-035 nwamadm.c'871: Would seem preferable to return a error code
> > > > > indicating whether the function succeeded rather than overloading
> > > > > *_UNINITALIZED for the job.
> > > > If we can't get the state, then it is UNINITIALIZED. No need to check if
> > > > function succeeded or not.
> > >
> > > Then how about making the state the return value?
> > >
> > The function actually returns the state and auxilliary state. I changed
> > the function to return the nwam_error_t from nwam_*_get_state().
>
> ... and callers pay attention to that return value now?
>
No, not in the way it is being used. The state is set to UNINITIALIZED
before its pointer is passed to determine_object_state(). If the
nwam_*_get_state() fails, then it doesn't change the state and the
calling function will use the UNINITIALIZED state value. I could check
for the return value and set the state to UNINITIALIZED, but that's just
more code to achieve the same thing.
Anurag
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
3,083
From:
US
Registered:
3/9/05
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 13, 2009 5:52 PM
in response to: anurag_m
|
|
> >
> > > > > > meem-032 nwamadm.c'719: By the time we get here isn't the enable
> > > > > > complete? That is, seems like this should be "Enabled", not "Enabling".
> > > > > not really. nwamadm is asynchronous (like svcadm), so the enabling is
> > > > > going on in the background.
> > > >
> > > > Then does this mean that the error message "<foo> is already enabled"
> > > > (e.g., line 606) is not necessarily correct? What happens if I try to
> > > > disable or enable while an enable/disable is still ongoing?
> > > >
> > > Another enable/disable action is enqueued.
> >
> > So the processing of each transition is asynchronous but will wait for all
> > previous transitions to complete before starting?
> >
> Nope, the second event (let's say enable event is enqueued by the second
> command (disable will be the same)) will not wait until the first one
> finishes. The first enable event will start the state machine. When
> transitioning through the different state, more events are enqueued.
> The second enable event may be processed before the states have
> stabilized. However, it doesn't cause any change because the first step
> in processing these enable event is to see what object is enabled.
> Since these are the same, the second enable event will be ignored.
It seems like this could still cause problems -- e.g. a disable followed
immediately by an enable may not reenable everything since its work is
interspersed with the earlier disable.
> > > The function actually returns the state and auxilliary state. I changed
> > > the function to return the nwam_error_t from nwam_*_get_state().
> >
> > ... and callers pay attention to that return value now?
> >
> No, not in the way it is being used. The state is set to UNINITIALIZED
> before its pointer is passed to determine_object_state(). If the
> nwam_*_get_state() fails, then it doesn't change the state and the
> calling function will use the UNINITIALIZED state value. I could check
> for the return value and set the state to UNINITIALIZED, but that's just
> more code to achieve the same thing.
Sorry, I don't follow. What is the point of having a return value that is
always ignored?
--
meem
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 14, 2009 8:35 AM
in response to: meem
|
|
Peter Memishian wrote:
> > >
> > > > > > > meem-032 nwamadm.c'719: By the time we get here isn't the enable
> > > > > > > complete? That is, seems like this should be "Enabled", not "Enabling".
> > > > > > not really. nwamadm is asynchronous (like svcadm), so the enabling is
> > > > > > going on in the background.
> > > > >
> > > > > Then does this mean that the error message "<foo> is already enabled"
> > > > > (e.g., line 606) is not necessarily correct? What happens if I try to
> > > > > disable or enable while an enable/disable is still ongoing?
> > > > >
> > > > Another enable/disable action is enqueued.
> > >
> > > So the processing of each transition is asynchronous but will wait for all
> > > previous transitions to complete before starting?
> > >
> > Nope, the second event (let's say enable event is enqueued by the second
> > command (disable will be the same)) will not wait until the first one
> > finishes. The first enable event will start the state machine. When
> > transitioning through the different state, more events are enqueued.
> > The second enable event may be processed before the states have
> > stabilized. However, it doesn't cause any change because the first step
> > in processing these enable event is to see what object is enabled.
> > Since these are the same, the second enable event will be ignored.
>
> It seems like this could still cause problems -- e.g. a disable followed
> immediately by an enable may not reenable everything since its work is
> interspersed with the earlier disable.
>
Two events of the same type are handled correctly -- the second one
being ignored. Checking thoroughly into the two contradictory events
had some interesting findings. The final state of the object reflected
the second action. However, the state changes for the first action
continued. That shouldn't happen. Bug 10683 should fix this behavior
(http://defect.opensolaris.org/bz/show_bug.cgi?id=10683).
> > > > The function actually returns the state and auxilliary state. I changed
> > > > the function to return the nwam_error_t from nwam_*_get_state().
> > >
> > > ... and callers pay attention to that return value now?
> > >
> > No, not in the way it is being used. The state is set to UNINITIALIZED
> > before its pointer is passed to determine_object_state(). If the
> > nwam_*_get_state() fails, then it doesn't change the state and the
> > calling function will use the UNINITIALIZED state value. I could check
> > for the return value and set the state to UNINITIALIZED, but that's just
> > more code to achieve the same thing.
>
> Sorry, I don't follow. What is the point of having a return value that is
> always ignored?
>
The way I wrote the code, the return value doesn't matter because I know
what the state and aux_state are when the determine_object_state()
function returns. I know the library doesn't change the value of *state
and *aux_state if the function doesn't succeed. I used that knowledge
to design the consumer the same way. Some other code/developer without
prior knowledge could and should use the return value from the library
call. In the topic of return values being ignored, I wonder why they
are ignored all over the source with "(void) function(...)"? Seems like
a waste of ....
Anyway, in the interest of ending this discussion, I have changed the
code to set the state and aux_state to uninitialized if the return value
is not NWAM_SUCCESS. The behavior is the same no matter which way it is
written.
/* returns the state and auxilliary state of the object */
static void
determine_object_state(nwam_object_type_t type, const char *name,
nwam_ncu_class_t ncu_class, nwam_state_t *state,
nwam_aux_state_t *aux_state)
{
nwam_error_t ret;
switch (type) {
case NWAM_OBJECT_TYPE_ENM:
{
nwam_enm_handle_t enmh;
if (nwam_enm_read(name, 0, &enmh) == NWAM_SUCCESS) {
ret = nwam_enm_get_state(enmh, state, aux_state);
nwam_enm_free(enmh);
}
break;
}
case NWAM_OBJECT_TYPE_LOC:
{
nwam_loc_handle_t loch;
if (nwam_loc_read(name, 0, &loch) == NWAM_SUCCESS) {
ret = nwam_loc_get_state(loch, state, aux_state);
nwam_loc_free(loch);
}
break;
}
case NWAM_OBJECT_TYPE_NCP:
{
nwam_ncp_handle_t ncph;
if (nwam_ncp_read(name, 0, &ncph) == NWAM_SUCCESS) {
ret = nwam_ncp_get_state(ncph, state, aux_state);
nwam_ncp_free(ncph);
}
break;
}
case NWAM_OBJECT_TYPE_NCU:
{
nwam_ncp_handle_t ncph;
nwam_ncu_handle_t ncuh;
nwam_ncu_type_t ncu_type;
if ((ncph = determine_active_ncp()) == NULL)
break;
ncu_type = nwam_ncu_class_to_type(ncu_class);
if ((ret = nwam_ncu_read(ncph, name, ncu_type, 0, &ncuh))
== NWAM_SUCCESS) {
ret = nwam_ncu_get_state(ncuh, state, aux_state);
nwam_ncu_free(ncuh);
}
nwam_ncp_free(ncph);
break;
}
default:
/* NOTREACHED */
break;
}
if (ret != NWAM_SUCCESS) {
state = NWAM_STATE_UNINITIALIZED;
aux_state = NWAM_AUX_STATE_UNINITIALIZED;
}
}
And the function that calls determine_object_state() looks like:
/* prepends given information to the p_entries linked-list */
static int
add_to_profile_entry(nwam_object_type_t type, const char *name,
nwam_ncu_class_t ncu_class)
{
profile_entry_t *pent;
nwam_state_t state;
nwam_aux_state_t aux_state;
if ((pent = malloc(sizeof (struct profile_entry))) == NULL)
return (NWAM_NO_MEMORY);
determine_object_state(type, name, ncu_class, &state, &aux_state);
pent->p_type = type;
pent->p_ncu_class = ncu_class;
(void) strlcpy(pent->p_name, name, sizeof (pent->p_name));
if (strlen(name) > pname_max_width)
pname_max_width = strlen(name);
pent->p_state = state;
pent->p_aux_state = aux_state;
/* prepend to p_entries */
pent->p_next = p_entries;
p_entries = pent;
return (0);
}
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
3,083
From:
US
Registered:
3/9/05
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 14, 2009 6:28 PM
in response to: anurag_m
|
|
> Two events of the same type are handled correctly -- the second one
> being ignored. Checking thoroughly into the two contradictory events
> had some interesting findings. The final state of the object reflected
> the second action. However, the state changes for the first action
> continued. That shouldn't happen. Bug 10683 should fix this behavior
> (http://defect.opensolaris.org/bz/show_bug.cgi?id=10683).
OK.
> The way I wrote the code, the return value doesn't matter because I know
> what the state and aux_state are when the determine_object_state()
> function returns. I know the library doesn't change the value of *state
> and *aux_state if the function doesn't succeed. I used that knowledge
> to design the consumer the same way. Some other code/developer without
> prior knowledge could and should use the return value from the library
> call. In the topic of return values being ignored, I wonder why they
> are ignored all over the source with "(void) function(...)"? Seems like
> a waste of ....
Huh? The return value from a given function call may or may not be of
interest to a caller. My point is that it makes no sense to return a
value if it is *never* of interest to a caller. Further, having a
function "return" two values through pointer arguments and return no
values through its return value is simply bad interface design.
--
meem
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 16, 2009 7:54 PM
in response to: meem
|
|
Peter Memishian wrote:
> Huh? The return value from a given function call may or may not be of
> interest to a caller. My point is that it makes no sense to return a
> value if it is *never* of interest to a caller. Further, having a
> function "return" two values through pointer arguments and return no
> values through its return value is simply bad interface design.
>
I've changed determine_object_state() to return the state directly and
aux_state via pointer:
static nwam_state_t
determine_object_state(nwam_object_type_t type, const char *name,
nwam_ncu_class_t ncu_class, nwam_aux_state_t *aux_statep)
Also, the webrev at
http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
has been updated.
Thanks,
Anurag
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
3,083
From:
US
Registered:
3/9/05
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 16, 2009 10:57 PM
in response to: anurag_m
|
|
> > Huh? The return value from a given function call may or may not be of
> > interest to a caller. My point is that it makes no sense to return a
> > value if it is *never* of interest to a caller. Further, having a
> > function "return" two values through pointer arguments and return no
> > values through its return value is simply bad interface design.
> >
> I've changed determine_object_state() to return the state directly and
> aux_state via pointer:
>
> static nwam_state_t
> determine_object_state(nwam_object_type_t type, const char *name,
> nwam_ncu_class_t ncu_class, nwam_aux_state_t *aux_statep)
>
> Also, the webrev at
> http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
> has been updated.
Looks good. A final tweak that would make the interface simpler is to
have determine_object_state() only fill in the aux_state pointer if the
caller passes it as non-NULL. This would e.g. allow list_ncp_callback()
to simply pass NULL for that argument.
--
meem
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 17, 2009 5:49 AM
in response to: meem
|
|
Peter Memishian wrote:
> > nwam_ncu_class_t ncu_class, nwam_aux_state_t *aux_statep)
> >
> > Also, the webrev at
> > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
> > has been updated.
>
> Looks good. A final tweak that would make the interface simpler is to
> have determine_object_state() only fill in the aux_state pointer if the
> caller passes it as non-NULL. This would e.g. allow list_ncp_callback()
> to simply pass NULL for that argument.
>
done and updated webrev.
Anurag
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
Posts:
3,083
From:
US
Registered:
3/9/05
|
|
|
|
Re: [nwam-discuss] nwamadm code review comments: meem-004...063
Posted:
Aug 17, 2009 11:05 AM
in response to: anurag_m
|
|
> > > nwam_ncu_class_t ncu_class, nwam_aux_state_t *aux_statep)
> > >
> > > Also, the webrev at
> > > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
> > > has been updated.
> >
> > Looks good. A final tweak that would make the interface simpler is to
> > have determine_object_state() only fill in the aux_state pointer if the
> > caller passes it as non-NULL. This would e.g. allow list_ncp_callback()
> > to simply pass NULL for that argument.
> >
> done and updated webrev.
Looks good.
--
meem
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
