On 06/28/10 12:16 PM, Matt Keenan wrote:
Thanks *Joe*, and John for your review.
I've updated webrev relating to both sets of comments.
cheers
Matt
On 06/28/10 04:58 PM, John Fischer wrote:
Opps I meant Joe.
John
On 06/28/10 08:53 AM, John Fischer wrote:
Matt,
In addition to what Jack said about doing the nvlist_free() and
assigning
NULL right after, something else doesn't seem right in the code.
We have:
1205 if (nvlist_alloc(&ti_ex_attrs, TI_TARGET_NVLIST_TYPE,
0) != 0) {
Then we do some stuff with it and then free it at:
1228 nvlist_free(ti_ex_attrs);
Then latter on:
1246 if (prepare_zfs_root_pool_attrs(&ti_ex_attrs, disk_name,
1247 install_slice_id) != OM_SUCCESS) {
1248 om_log_print("Could not prepare ZFS root pool
attribute set\n");
1249 nvlist_free(ti_ex_attrs);
1250 status = -1;
1251 goto ti_error;
1252 }
prepare_zfs_root_pool_attrs() calls nvlist_alloc() and can return
something
other then OM_SUCCESS if nvlist_alloc() fails:
2542 if (nvlist_alloc(attrs, TI_TARGET_NVLIST_TYPE, 0) != 0) {
2543 om_log_print("Could not create target nvlist.\n");
2544
2545 return (OM_FAILURE);
2546 }
Thus ti_ex_attrs has not been allocated and free-ing it at line 1249
is in
error as well.
prepare_volume_attrs() has a similar situation happening which is what
you have fixed with the NULL assignment and the checks for NULL before
free-ing.
prepare_be_attrs() ditto:
1424 if (prepare_be_attrs(&ti_ex_attrs) != OM_SUCCESS) {
1425 om_log_print("Could not prepare BE attribute
set\n");
1426 nvlist_free(ti_ex_attrs);
1427 status = -1;
1428 goto ti_error;
1429 }
So there appears to be other places within this code that the NULL
assignment and
checks need to be done.
Thanks,
John
On 06/28/10 05:56 AM, Matt Keenan wrote:
Quick Code review request for :
Bug 16192 - auto-install core dumped when creating invalid size
swap or dump
http://defect.opensolaris.org/bz/show_bug.cgi?id=16192
Webrev :
http://cr.opensolaris.org/~mattman/bug-16192/
Whilst auto-install does core, it does not have any adverse affect
on the install as the install is in the process of failing/quitting
either way.
Problem occurs because nvlist_free() is being called on an nvlist
variable that was never allocated.
Solution is to assign NULL to nvlist variable prior to possible
allocation. If it gets allocated it will be non-null and thus can
be freed.
cheers
Matt
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hey Matt,
Question/Suggestion:
Do you want to set it to NULL after these invocations to
nvlist_free(ti_ex_attrs) ?
1213 nvlist_free(ti_ex_attrs);
1250 nvlist_free(ti_ex_attrs);
1349 nvlist_free(ti_ex_attrs);
1386 nvlist_free(ti_ex_attrs);
1430 nvlist_free(ti_ex_attrs);
I realize all of these are followed with a call to goto ti_error; but
future code changes could be problematic if ti_ex_attrs is not set to
NULL after freeing.
Your call.
Joe
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
