|
Replies:
10
-
Last Post:
Sep 17, 2009 7:02 PM
by: anurag_m
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
[nwam-discuss] nwamcfg code review comments: meem-109...180
Posted:
Aug 31, 2009 1:42 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.
(All code-review comments are at
http://wikis.sun.com/display/NWAM/Phase+1+Code+Review+Feedback).
The original webrev is at http://cr.opensolaris.org/~mph/nwam1_cr/.
> meem-109 nwamcfg.c: many of the same comments I made for nwamadm apply
> to this file – e.g., zerr(), die_nwamerr(), CMD_* vs command tables,
> etc. In the interest of brevity, I have not repeated previous comments
> that apply directly to nwamcfg (e.g., problematic use of basename()).
Rather than die_*() commands, the following commands exist. Can't use
die_*() as we don't want to exit because of interactive-mode.
* zerr() has been renamed to nerr().
* nwamerr() takes in nwam_error_t and prints the string representations.
I've also added nwamcfg.xcl and XGETFLAGS in the Makefile.
> meem-110 nwamcfg.c: a lot of very repetitious code – I have enumerated
> a number of instances below. FWIW, a lot of these issues are due to
> the libnwam API itself – e.g., sets of parallel functions that differ
> only in the handle they take and their name. It seems like an OO
> design would've simplified the code considerably – e.g., each object
> could've had an ops vector associated with it that the various callers
> could use without repeatedly concerning themselves with the type of
> the object.
I have taken your suggestion on "get_handle_type()". I called it
"active_object_type()" and returns NWAM_OBJECT_TYPE_* depending on which
handle is not NULL. I've added functions that taken in the object type
and call the respective library functions.
get_func(), set_func(), clear_func(), walkprop_func() have been
considerably shortened thanks to your approach. The object type is
consulted when library function calls are to be made. At other times,
the helper functions are passed the object type.
Also, code that duplicated logic of commit_func(), destroy_func(),
end_func() has been factored into do_commit() and do_cancel().
> meem-118 nwamcfg.c'163, 167, 187, 189, 193, 214: What is the
> significance of these comments?
To show where the constants come from.
> meem-120 nwamcfg.c'240, 286, 304, 359: Why are these all different
> structures? They all have identical contents and field names. These
> should be folded into a common nwamcfg_prop_table_entry structure and
> all members should have a consistent prefix for easy cscoping.
These structures have been modified to map the PT_* constants (used by
the parser) to the property names. The other fields in the table has
been removed. Library calls are used to determine the type of the value
and whether it is multi-valued?
> meem-126 nwamcfg.c'1043: prompt[] only accounts for MAX_NWAM_TYPE_LEN
> once, but there can be two types.
fixed this. found that the object name can also appear twice (in the
case of NCUs).
> meem-130 nwamcfg.c'1390-1392: Under what situations do callers pass in
> NULL?
export_func() and list_func() can pass in NULL strings. Checking and
returning NULL here clears up a lot of duplicate code in the respective
functions.
> meem-138 nwamcfg.c'1752-1764: What happens to nwamcfg when this to
> fails part-way through?
Whatever is destroyed in gone. Once an error is hit, the remaining
objects are not destroyed. Returns to the nwamcfg> prompt if in
interactive-mode.
> meem-150 nwamcfg.c'2389-2399: Doesn't strtol() already provide this
> ability by checking if isdigit(*endptr)?
Letters are returned as 0 and thus the need to check if all values are
digits or not.
> meem-153 nwamcfg.c'2543-2544: How will this value be maintained? Seems
> like nothing uses more than two values though.
maintained in nwamcfg. Not likely that the number of values will
increase in the future. Made the constant slightly bigger so that
there's no need to change later.
> meem-154 nwamcfg.c'2777-2780: `return (!prop_found)' seems simpler (if
> the concern is clarity, giving the boolean another name (like
> `show_prop') would help.
Keeping this variable as prop_found. That's because if the code reaches
after the for-loop, then it means that the property was found in the
table but did not match any rule, thus it should not be shown. show_prop
is not the intended meaning at this point in the code.
> meem-158 nwamcfg.c'3181-3258: This is really hard to read. One minor
> improvement would be to have the logic at 3193-3257 just build the
> string arrays, and have a single loop iterate through them at the end
> and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic.
generating the string would be the same with snprintf() replacing fprintf().
> meem-159 nwamcfg.c'3322-3334: Needs to share a common implementation
> with output_propname().
The reason this is different is because the location properties have
longer names than the other objects. If the other objects used the same
tabbing as location, the values would appear spaced out more. The reason
for the difference is just making output prettier.
Using "%-25s" would print:
ENM:foo
activation-mode manual
enabled false
stop "/bar"
start "/bar"
and "%-16s" will print
ENM:foo
activation-mode manual
enabled false
stop "/bar"
start "/bar"
> meem-177 nwamcfg.c'4899-4908: What would go wrong if we attempted to
> use a file that wasn't a "regular file"?
checked with S_ISREG flag a few lines later.
Thanks,
Anurag
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
Posts:
542
From:
Registered:
5/11/05
|
|
|
|
Re: [nwam-discuss] nwamcfg code review comments: meem-109...180
Posted:
Sep 2, 2009 1:53 AM
in response to: anurag_m
|
|
On Mon, 31 Aug 2009 16:42:53 -0400
"Anurag S. Maskey" <Anurag dot Maskey at Sun dot COM> wrote:
> Webrev is at:
>
> http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
This looks great. All the deleted code is wonderful :)
These are some nits I noticed. Do with them as you wish. I'm happy
with the code and as far as I'm concerned you can push when you want.
nwamcfg.c:1247-1254 better as 'while(end >= 0 && (str[end] == '"' ||
str[end] == '\n')) end--; str[end+1] = 0;' instead of trying to
enumerate various combos?
nwamcfg.c:3935 should be putchar()
nwamcfg.h:42 max length of "" to me would be 1 since the string
contains an implied 0 termination
mph
_______________________________________________
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 2, 2009 6:13 AM
in response to: mph
|
|
Michael Hunter wrote:
>> Webrev is at:
>>
>> http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
>>
> nwamcfg.c:1247-1254 better as 'while(end >= 0 && (str[end] == '"' ||
> str[end] == '\n')) end--; str[end+1] = 0;' instead of trying to
> enumerate various combos?
>
> nwamcfg.c:3935 should be putchar()
>
> nwamcfg.h:42 max length of "" to me would be 1 since the string
> contains an implied 0 termination
>
Thanks Michael. All comments accepted. Diffs below:
diff -r 2adbb065f174 usr/src/cmd/cmd-inet/usr.sbin/nwamcfg/nwamcfg.c
--- a/usr/src/cmd/cmd-inet/usr.sbin/nwamcfg/nwamcfg.c Mon Aug 31
16:36:38 2009 -0400
+++ b/usr/src/cmd/cmd-inet/usr.sbin/nwamcfg/nwamcfg.c Wed Sep 02
09:12:17 2009 -0400
@@ -1230,7 +1230,7 @@
trim_quotes(const char *quoted_str)
{
char *str;
- int len;
+ int end;
/* export_func() and list_func() can pass NULL here */
if (quoted_str == NULL)
@@ -1245,13 +1245,10 @@
return (NULL);
/* remove trailing quote and newline */
- len = strlen(str);
- if (str[len - 1] == '\n') {
- str[len - 1] = '\0';
- if (str[len - 2] == '"')
- str[len - 2] = '\0';
- } else if (str[len - 1] == '"')
- str[len - 1] = '\0';
+ end = strlen(str) - 1;
+ while (end >= 0 && (str[end] == '"' || str[end] == '\n'))
+ end--;
+ str[end+1] = 0;
return (str);
}
@@ -3811,7 +3808,7 @@
choices = B_TRUE;
}
if (choices)
- (void) printf("]");
+ (void) putchar(']');
break;
case NWAM_VALUE_TYPE_BOOLEAN:
(void) printf(" [%s|%s]", "true", "false");
@@ -3932,7 +3929,7 @@
if (ret == NWAM_SUCCESS) {
(void) printf(" (");
output_prop_val(props[i], vals, stdout, B_TRUE);
- (void) printf(")");
+ (void) putchar(')');
nwam_value_free(vals);
}
/* print choices, won't print anything if there aren't
any */
diff -r 2adbb065f174 usr/src/cmd/cmd-inet/usr.sbin/nwamcfg/nwamcfg.h
--- a/usr/src/cmd/cmd-inet/usr.sbin/nwamcfg/nwamcfg.h Mon Aug 31
16:36:38 2009 -0400
+++ b/usr/src/cmd/cmd-inet/usr.sbin/nwamcfg/nwamcfg.h Wed Sep 02
09:12:17 2009 -0400
@@ -40,7 +40,7 @@
#define NWAM_REPEAT 2
/* max length of "ncu", "ncp", "loc", "enm", "wlan" */
-#define NWAM_MAX_TYPE_LEN 4
+#define NWAM_MAX_TYPE_LEN 5
#define CMD_CANCEL 0
#define CMD_CLEAR 1
am223141@zhadum:nwam1-fixes$
_______________________________________________
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 8, 2009 6:14 PM
in response to: anurag_m
|
|
Sorry for the delay.
> http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
>
> > meem-109 nwamcfg.c: many of the same comments I made for nwamadm apply
> > to this file e.g., zerr(), die_nwamerr(), CMD_* vs command tables,
> > etc. In the interest of brevity, I have not repeated previous comments
> > that apply directly to nwamcfg (e.g., problematic use of basename()).
>
> Rather than die_*() commands, the following commands exist. Can't use
> die_*() as we don't want to exit because of interactive-mode.
>
> * zerr() has been renamed to nerr().
> * nwamerr() takes in nwam_error_t and prints the string representations.
Sounds good.
> I've also added nwamcfg.xcl and XGETFLAGS in the Makefile.
Thanks.
> > meem-110 nwamcfg.c: a lot of very repetitious code I have enumerated
> > a number of instances below. FWIW, a lot of these issues are due to
> > the libnwam API itself e.g., sets of parallel functions that differ
> > only in the handle they take and their name. It seems like an OO
> > design would've simplified the code considerably e.g., each object
> > could've had an ops vector associated with it that the various callers
> > could use without repeatedly concerning themselves with the type of
> > the object.
> I have taken your suggestion on "get_handle_type()". I called it
> "active_object_type()" and returns NWAM_OBJECT_TYPE_* depending on which
> handle is not NULL. I've added functions that taken in the object type
> and call the respective library functions.
>
> get_func(), set_func(), clear_func(), walkprop_func() have been
> considerably shortened thanks to your approach. The object type is
> consulted when library function calls are to be made. At other times,
> the helper functions are passed the object type.
>
> Also, code that duplicated logic of commit_func(), destroy_func(),
> end_func() has been factored into do_commit() and do_cancel().
Great. To be clear, my broader suggestion above (and this is *not*
something I'm suggesting be done right now) was for each NWAM object to
have an array of function pointers that allowed different operations to be
done on them (e.g., commit, validate, copy, read, ...). Then nwamcfg
would not need to switch on the type of object, but could instead just
invoke the function pointer.
> > meem-118 nwamcfg.c'163, 167, 187, 189, 193, 214: What is the
> > significance of these comments?
> To show where the constants come from.
But to what purpose? How will someone use this information in the future?
> > meem-120 nwamcfg.c'240, 286, 304, 359: Why are these all different
> > structures? They all have identical contents and field names. These
> > should be folded into a common nwamcfg_prop_table_entry structure and
> > all members should have a consistent prefix for easy cscoping.
> These structures have been modified to map the PT_* constants (used by
> the parser) to the property names. The other fields in the table has
> been removed. Library calls are used to determine the type of the value
> and whether it is multi-valued?
Sounds good.
> > meem-126 nwamcfg.c'1043: prompt[] only accounts for MAX_NWAM_TYPE_LEN
> > once, but there can be two types.
> fixed this. found that the object name can also appear twice (in the
> case of NCUs).
Sounds good. Might be able to make the code more self-descriptive with
"+ sizeof("::::> ")" instead of "7".
> > meem-138 nwamcfg.c'1752-1764: What happens to nwamcfg when this to
> > fails part-way through?
> Whatever is destroyed in gone. Once an error is hit, the remaining
> objects are not destroyed. Returns to the nwamcfg> prompt if in
> interactive-mode.
My question was more around the user experience. Do things continue to
work in a reliable fashion? For instance, can the user see/interact with
those objects that were not destroyed?
> > meem-150 nwamcfg.c'2389-2399: Doesn't strtol() already provide this
> > ability by checking if isdigit(*endptr)?
> Letters are returned as 0 and thus the need to check if all values are
> digits or not.
I don't understand. What returns letters as 0? Please explain precisely
what would fail if you used `strtoul(val[i], &endptr, 10)' and verified
that `endptr != val[i]'.
> > meem-153 nwamcfg.c'2543-2544: How will this value be maintained? Seems
> > like nothing uses more than two values though.
> maintained in nwamcfg. Not likely that the number of values will
> increase in the future. Made the constant slightly bigger so that
> there's no need to change later.
It seems like it's not really tracking the enumeration max in libnwam.h,
but rather only concerned with the maximum number of values in
check_prop_values[] in nwamcfg.c. As such, I think it should be recast as
CHECK_PROP_VALMAX or somesuch. (As an aside, it'd be nice to have unique
prefixes for prop_display_entry fields -- e.g., pde_name, pde_checkname
and pde_checkvals.)
> > meem-158 nwamcfg.c'3181-3258: This is really hard to read. One minor
> > improvement would be to have the logic at 3193-3257 just build the
> > string arrays, and have a single loop iterate through them at the end
> > and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic.
> generating the string would be the same with snprintf() replacing fprintf().
No, because you centralize this:
(void) fprintf(wf, "%s", (i == (num - 1)) ? "\0" :
NWAM_VALUE_DELIMITER_STR);
... into just one single `for' loop at the end, instead of having to
inline that into all the type-specific `for' loops.
> > meem-159 nwamcfg.c'3322-3334: Needs to share a common implementation
> > with output_propname().
> The reason this is different is because the location properties have
> longer names than the other objects. If the other objects used the same
> tabbing as location, the values would appear spaced out more. The reason
> for the difference is just making output prettier.
That can be accommodated by renaming output_propname() to
output_propname_common(), adding an additional `width' argument, and
having output_loc_propname() and output_propname() both call through to
output_propname_common() with an appropriate value for `width'.
> > meem-177 nwamcfg.c'4899-4908: What would go wrong if we attempted to
> > use a file that wasn't a "regular file"?
> checked with S_ISREG flag a few lines later.
My comment was intended to reference the S_ISREG() test. That is, why is
this test needed? What would happen if we didn't have it?
--
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 9, 2009 12:41 PM
in response to: meem
|
|
Thanks for your comments. I've updated the webrev.
Peter Memishian wrote:
> > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
>
> > > meem-118 nwamcfg.c'163, 167, 187, 189, 193, 214: What is the
> > > significance of these comments?
> > To show where the constants come from.
>
> But to what purpose? How will someone use this information in the future?
>
I thought it would be helpful to provide the source of the constants. I
removed the comments (both in nwamcfg.c and nwamcfg.h).
> > > meem-126 nwamcfg.c'1043: prompt[] only accounts for MAX_NWAM_TYPE_LEN
> > > once, but there can be two types.
> > fixed this. found that the object name can also appear twice (in the
> > case of NCUs).
>
> Sounds good. Might be able to make the code more self-descriptive with
> "+ sizeof("::::> ")" instead of "7".
>
changed '7' to 'sizeof("::::> ")'.
> > > meem-138 nwamcfg.c'1752-1764: What happens to nwamcfg when this to
> > > fails part-way through?
> > Whatever is destroyed in gone. Once an error is hit, the remaining
> > objects are not destroyed. Returns to the nwamcfg> prompt if in
> > interactive-mode.
>
> My question was more around the user experience. Do things continue to
> work in a reliable fashion? For instance, can the user see/interact with
> those objects that were not destroyed?
>
yes. If the object was not destroyed, the user can still select and
alter it just like before the destroy failed.
> > > meem-150 nwamcfg.c'2389-2399: Doesn't strtol() already provide this
> > > ability by checking if isdigit(*endptr)?
> > Letters are returned as 0 and thus the need to check if all values are
> > digits or not.
>
> I don't understand. What returns letters as 0? Please explain precisely
> what would fail if you used `strtoul(val[i], &endptr, 10)' and verified
> that `endptr != val[i]'.
>
I was shortcutting with atoi() rather than strtoul() which was returning
the 0. I've changed the code to use strtoul()
uint_vals[i] = strtou(val[i], &endptr, 10);
if (endptr == val[i]) {
[...]
return (NULL);
}
> > > meem-153 nwamcfg.c'2543-2544: How will this value be maintained? Seems
> > > like nothing uses more than two values though.
> > maintained in nwamcfg. Not likely that the number of values will
> > increase in the future. Made the constant slightly bigger so that
> > there's no need to change later.
>
> It seems like it's not really tracking the enumeration max in libnwam.h,
> but rather only concerned with the maximum number of values in
> check_prop_values[] in nwamcfg.c. As such, I think it should be recast as
> CHECK_PROP_VALMAX or somesuch. (As an aside, it'd be nice to have unique
> prefixes for prop_display_entry fields -- e.g., pde_name, pde_checkname
> and pde_checkvals.)
>
fixed and renamed the constants and the added prefixes to the
prop_display_entry fields. also added pte_ prefixes to prop_table_entry
fields.
> > > meem-158 nwamcfg.c'3181-3258: This is really hard to read. One minor
> > > improvement would be to have the logic at 3193-3257 just build the
> > > string arrays, and have a single loop iterate through them at the end
> > > and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic.
> > generating the string would be the same with snprintf() replacing fprintf().
>
> No, because you centralize this:
>
> (void) fprintf(wf, "%s", (i == (num - 1)) ? "\0" :
> NWAM_VALUE_DELIMITER_STR);
>
> ... into just one single `for' loop at the end, instead of having to
> inline that into all the type-specific `for' loops.
>
I see. I've fixed these separate fprintf()'s into the same one.
> > > meem-159 nwamcfg.c'3322-3334: Needs to share a common implementation
> > > with output_propname().
> > The reason this is different is because the location properties have
> > longer names than the other objects. If the other objects used the same
> > tabbing as location, the values would appear spaced out more. The reason
> > for the difference is just making output prettier.
>
> That can be accommodated by renaming output_propname() to
> output_propname_common(), adding an additional `width' argument, and
> having output_loc_propname() and output_propname() both call through to
> output_propname_common() with an appropriate value for `width'.
>
got it, thanks.
> > > meem-177 nwamcfg.c'4899-4908: What would go wrong if we attempted to
> > > use a file that wasn't a "regular file"?
> > checked with S_ISREG flag a few lines later.
>
> My comment was intended to reference the S_ISREG() test. That is, why is
> this test needed? What would happen if we didn't have it?
>
It will try to read from that file. I tried with several /dev/* files
without the S_ISREG(). /dev/console, /dev/stdin, /dev/tty brought me to
the nwamcfg> interactive prompt. Others like /dev/ipf, /dev/zpool gave
a syntax error as nwamcfg tried to read in the file. Then, files like
/dev/bge0, /dev/ttyra hung.
I don't know if this S_ISREG() is needed or not. zonecfg does the test.
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 9, 2009 4:17 PM
in response to: anurag_m
|
|
> > > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
>
> > > > meem-158 nwamcfg.c'3181-3258: This is really hard to read. One minor
> > > > improvement would be to have the logic at 3193-3257 just build the
> > > > string arrays, and have a single loop iterate through them at the end
> > > > and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic.
> > > generating the string would be the same with snprintf() replacing fprintf().
> >
> > No, because you centralize this:
> >
> > (void) fprintf(wf, "%s", (i == (num - 1)) ? "\0" :
> > NWAM_VALUE_DELIMITER_STR);
> >
> > ... into just one single `for' loop at the end, instead of having to
> > inline that into all the type-specific `for' loops.
> >
> I see. I've fixed these separate fprintf()'s into the same one.
The webrev doesn't seem to have this change.
> > > > meem-177 nwamcfg.c'4899-4908: What would go wrong if we attempted to
> > > > use a file that wasn't a "regular file"?
> > > checked with S_ISREG flag a few lines later.
> >
> > My comment was intended to reference the S_ISREG() test. That is, why is
> > this test needed? What would happen if we didn't have it?
> >
> It will try to read from that file. I tried with several /dev/* files
> without the S_ISREG(). /dev/console, /dev/stdin, /dev/tty brought me to
> the nwamcfg> interactive prompt. Others like /dev/ipf, /dev/zpool gave
> a syntax error as nwamcfg tried to read in the file. Then, files like
> /dev/bge0, /dev/ttyra hung.
>
> I don't know if this S_ISREG() is needed or not. zonecfg does the test.
>
OK.
--
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 9, 2009 5:39 PM
in response to: meem
|
|
Peter Memishian wrote:
> > > > http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
> >
> > > > > meem-158 nwamcfg.c'3181-3258: This is really hard to read. One minor
> > > > > improvement would be to have the logic at 3193-3257 just build the
> > > > > string arrays, and have a single loop iterate through them at the end
> > > > > and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic.
> > > > generating the string would be the same with snprintf() replacing fprintf().
> > >
> > > No, because you centralize this:
> > >
> > > (void) fprintf(wf, "%s", (i == (num - 1)) ? "\0" :
> > > NWAM_VALUE_DELIMITER_STR);
> > >
> > > ... into just one single `for' loop at the end, instead of having to
> > > inline that into all the type-specific `for' loops.
> > >
> > I see. I've fixed these separate fprintf()'s into the same one.
>
> The webrev doesn't seem to have this change.
>
Meem,
I do not understand what you are suggesting. Do you mean the printing
either the DELIMITER or not after all the if-then-else statements? That
wouldn't work since each value type will have to read the values
separately and then loop through the values and print them (with %s,
%lld, etc). The looping is basically printing the "value," if it is not
the last item and just the "value" if it is the last item in the list.
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 9, 2009 6:00 PM
in response to: anurag_m
|
|
> I do not understand what you are suggesting. Do you mean the printing
> either the DELIMITER or not after all the if-then-else statements? That
> wouldn't work since each value type will have to read the values
> separately and then loop through the values and print them (with %s,
> %lld, etc). The looping is basically printing the "value," if it is not
> the last item and just the "value" if it is the last item in the list.
I mean something like this:
static void
output_prop_val(const char *prop_name, nwam_value_t value, FILE *wf,
boolean_t quoted_strings)
{
int i;
uint_t num;
nwam_value_type_t value_type;
nwam_error_t ret;
void *vals;
tostr_func_t func;
if (nwam_value_get_type(value, &value_type) != NWAM_SUCCESS) {
nerr("Get value type error");
return;
}
switch (value_type) {
case NWAM_VALUE_TYPE_STRING:
if (nwam_value_get_string_array(value, &vals, &num) !=
NWAM_SUCCESS) {
nerr("Get string array error");
return;
}
func = quoted_strings ? str2qstr : str2str;
break;
case NWAM_VALUE_TYPE_INT64:
if (nwam_value_get_int64_array(value, &vals, &num) !=
NWAM_SUCCESS) {
nerr("Get int64 array error");
return;
}
func = int2str;
break;
/* ... */
case NWAM_VALUE_TYPE_BOOLEAN):
if (nwam_value_get_boolean_array(value, &vals, &num) !=
NWAM_SUCCESS) {
nerr("Get boolean array error");
return;
}
func = bool2str;
break;
}
for (i = 0; i < num; i++) {
(void) fprintf(wf, "%s%s", func(vals[i]),
i != num-1 ? NWAM_VALUE_DELIMITER_STR : "");
}
}
But maybe that's too hard to do without defeating type safety.
--
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 10, 2009 2:23 PM
in response to: meem
|
|
Peter Memishian wrote:
> > I do not understand what you are suggesting. Do you mean the printing
> > either the DELIMITER or not after all the if-then-else statements? That
> > wouldn't work since each value type will have to read the values
> > separately and then loop through the values and print them (with %s,
> > %lld, etc). The looping is basically printing the "value," if it is not
> > the last item and just the "value" if it is the last item in the list.
>
> I mean something like this:
>
> static void
> output_prop_val(const char *prop_name, nwam_value_t value, FILE *wf,
> boolean_t quoted_strings)
> {
> int i;
> uint_t num;
> nwam_value_type_t value_type;
> nwam_error_t ret;
> void *vals;
> tostr_func_t func;
>
> if (nwam_value_get_type(value, &value_type) != NWAM_SUCCESS) {
> nerr("Get value type error");
> return;
> }
>
> switch (value_type) {
> case NWAM_VALUE_TYPE_STRING:
> if (nwam_value_get_string_array(value, &vals, &num) !=
> NWAM_SUCCESS) {
> nerr("Get string array error");
> return;
> }
> func = quoted_strings ? str2qstr : str2str;
> break;
> case NWAM_VALUE_TYPE_INT64:
> if (nwam_value_get_int64_array(value, &vals, &num) !=
> NWAM_SUCCESS) {
> nerr("Get int64 array error");
> return;
> }
>
> func = int2str;
> break;
> /* ... */
> case NWAM_VALUE_TYPE_BOOLEAN):
> if (nwam_value_get_boolean_array(value, &vals, &num) !=
> NWAM_SUCCESS) {
> nerr("Get boolean array error");
> return;
> }
>
> func = bool2str;
> break;
> }
>
> for (i = 0; i < num; i++) {
> (void) fprintf(wf, "%s%s", func(vals[i]),
> i != num-1 ? NWAM_VALUE_DELIMITER_STR : "");
> }
> }
>
> But maybe that's too hard to do without defeating type safety.
>
That's great design. Thanks. I'll just have to point out that the
*2str() functions will have to take the prop_name also. That because
uint2str() will have to first run nwam_uint64_get_value_string() to see
if the property is an enum and if so, get a string representation of the
value. The other *2str() functions will ignore the prop_name.
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 10, 2009 2:26 PM
in response to: anurag_m
|
|
> That's great design. Thanks. I'll just have to point out that the
> *2str() functions will have to take the prop_name also. That because
> uint2str() will have to first run nwam_uint64_get_value_string() to see
> if the property is an enum and if so, get a string representation of the
> value. The other *2str() functions will ignore the prop_name.
True, but I think that's only because we've overloaded uint64_t to be two
different things (enumeration and unsigned integer). Unfortunately,
that's quite a bit to disentangle at this point.
--
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] nwamcfg code review comments: meem-109...180
Posted:
Sep 17, 2009 7:02 PM
in response to: meem
|
|
Meem,
I've updated the code and webrev so that output_prop_val() is similar to
what you suggested. The changes involves the signature of the *2str()
function. It takes (void *, const char *, char *). Also, void * arrays
don't work too well. So, the vals[] array and its usages isn't as simple
as you have it.
http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
Thanks,
Anurag
Peter Memishian wrote:
> static void
> output_prop_val(const char *prop_name, nwam_value_t value, FILE *wf,
> boolean_t quoted_strings)
> {
> int i;
> uint_t num;
> nwam_value_type_t value_type;
> nwam_error_t ret;
> void *vals;
> tostr_func_t func;
>
> if (nwam_value_get_type(value, &value_type) != NWAM_SUCCESS) {
> nerr("Get value type error");
> return;
> }
>
> switch (value_type) {
> case NWAM_VALUE_TYPE_STRING:
> if (nwam_value_get_string_array(value, &vals, &num) !=
> NWAM_SUCCESS) {
> nerr("Get string array error");
> return;
> }
> func = quoted_strings ? str2qstr : str2str;
> break;
> case NWAM_VALUE_TYPE_INT64:
> if (nwam_value_get_int64_array(value, &vals, &num) !=
> NWAM_SUCCESS) {
> nerr("Get int64 array error");
> return;
> }
>
> func = int2str;
> break;
> /* ... */
> case NWAM_VALUE_TYPE_BOOLEAN):
> if (nwam_value_get_boolean_array(value, &vals, &num) !=
> NWAM_SUCCESS) {
> nerr("Get boolean array error");
> return;
> }
>
> func = bool2str;
> break;
> }
>
> for (i = 0; i < num; i++) {
> (void) fprintf(wf, "%s%s", func(vals[i]),
> i != num-1 ? NWAM_VALUE_DELIMITER_STR : "");
> }
> }
>
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
