Hi Matt,
General:
-Please include all bugs in the webrev. Some of these changes are as a
result of other bugs than extended partition support(at least it looks
like it to me).
installation-disk-screen.c:
-You have a lot of print methods in this module that seem to be used for
debugging. Seems like you could put these in a file that contains
utility functions.
-This module is huge. Again, I would consider breaking out any utility
functions, putting them in a separate module to help maintainability of
this code.
line:
> 70 gboolean sizechange;
> 71 gboolean initialsizechange;
Why do you need to members for size change? Is there a fundamental
difference in initial size change and size change in terms of a warning?
line:
> 529 #define PARTINFO_NOT_EQ(x, y) \
> 530 ((x)->partition_id != (y)->partition_id || \
> 531 (x)->partition_size != (y)->partition_size || \
> 532 (x)->partition_offset != (y)->partition_offset || \
> 533 (x)->partition_order != (y)->partition_order || \
> 534 (x)->partition_type != (y)->partition_type || \
> 535 (x)->content_type != (y)->content_type || \
> 536 (x)->active != (y)->active || \
> 537 (x)->partition_size_sec != (y)->partition_size_sec || \
> 538 (x)->partition_offset_sec != (y)->partition_offset_sec)
Can you just use memcmp() here? The reason I suggest it is that all the
members in the structures are uint/int or boolean(int), there are no
pointers to other structures and it seems more efficient to use a
memcmp() in this case. You would need to be sure you used a memset() on
the structures before using them to ensure all bits, including padding,
are set to 0.
line:
> /* Has this primary got any logical children */
> 1140 + if
> (MainWindow.InstallationDiskWindow.startlogical[i] != NULL) {
> 1141 + /* Loop through all children and shift
> down by num_rows */
> 1142 +
> 1143 + partlogicals = 0;
> 1144 + curpartlogical =
> 1145 +
> MainWindow.InstallationDiskWindow.startlogical[i];
> 1146 + for (; curpartlogical != NULL;
> 1147 + curpartlogical = curpartlogical->next)
> {
> 1148 + partlogicals = partlogicals + 1;
> 1149 + logical_attach =
> 1150 +
> MainWindow.InstallationDiskWindow.partrow[i] +
> 1151 + partlogicals;
The comment here says you are shifting the extended partitions down by
num_rows. But, we aren't really are we?
It looks to me like we are moving the logical partitions down by 1:
partlogicals = partlogicals + 1
Am I misunderstanding what you are doing here?
line:
> 1323 + /* Cycle through until we get to matching index then free this
> item */
> 1324 + idx = -1;
you have already set idx at line 1309, no need to reset it here.
Actually, why do you set it to -1, then immediately do an idx ++?
line:
> 1387 + partitions = modifiedpartitions[activedisk];
> 1388 + for (partindex = FD_NUMPART; partindex < OM_NUMPART;
> partindex++) {
Since I don't have the orchestrator code in front of me, it is my
assumption that OM_NUMPART is no longer FD_NUMPART, based on Ginnie's
changes for extended partitions, as it is in the current code. I have to
assume this is true, otherwise this code wouldn't work :-).
line:
> +logicalpart_append(LogicalPartition *startlogical,
> 1436 + LogicalPartition *newlogical)
> 1437 +{
> 1438 + LogicalPartition *curlogical;
> 1439 +
> 1440 + for (curlogical = startlogical;
> 1441 + curlogical != NULL;
> 1442 + curlogical = curlogical->next) {
> 1443 + if (curlogical->next == NULL) {
> 1444 + curlogical->next = newlogical;
> 1445 + break;
> 1446 + }
> 1447 + }
> 1448 +}
Is it possible that you have a gap in the logical partitions and that
this would look like a NULL? So you would be inserting rather than
appending a new logical partition to the list? I am asking only because
it isn't clear to me if this will always result in an append.
line:
> 1512 + tidx =
> orchestrator_om_get_last_logical_index(partitions)+1;
> 1513 + for (; tidx > lidx+1; tidx--) {
> 151
Why do we need to call
orchestrator_om_get_last_logical_index(partitions) again, when we have
called it at line: 1499. We haven't modified the 'lidx' value we
obtained at this line. We could use this value and add 1, couldn't we?
line:
> * Ensure we have an entry in modifiedblkorder[acivedisk]
nit: should be [activedisk].
line:
> 1613 + newlogicalpart = g_new0(LogicalPartition, 1);
Is there any way a g_new() can return a NULL? Wondering if this is
something we have to check.
line:
> * If Changing from Unused we need re-calculate the AVAIL space
> 2109 + * And set spin button ranges and value
> 2110 + */
> 2111 + if
> (MainWindow.InstallationDiskWindow.partcombosaved[partindex] ==
> 2112 + UNUSED) {
> 2113 + partsize =
> 2114 +
> orchestrator_om_get_partition_sizegb(partinfo);
> 2115 + if (partsize == 0.0) {
> 2116 + /*
> 2117 + * This partition is going to be
> assigned 0.1
> 2118 + * in size so we need to remove
> 0.1 from the
> 2119 + * first unused item either below
> or above
> 2120 + * this item. There will always be
> unused
> 2121 + * space somewhere at this point
> otherwise
> 2122 + * the combobox would not have
> been active.
> 2123 + */
> 2124 +
> update_primary_unused_partition_size_from_ui(
> 2125 + partitions, partindex, 0.1);
> 2126 + }
Seems like we could combine this into a function since we do this more
than one time.
line:
> if (parttype == UNUSED &&
> 2528 + ((curblkorder->partinfo.partition_size
> > 0 && diffgb > 0) ||
> 2529 + (diffgb < 0))) {
Why can't we just check diffgb !=0 ?
installation-disk-screen.h:
line:
> 34 +#define GUI_INSTALL_FDISK_TABLE_ROWS 6
Why is this value now 6?
thanks,
sarah
******
Matt Keenan wrote:
> Hi,
>
> This is the initial code review request for Extended Partition Support in
> GUI installer.
>
> Webrev :
> http://cr.opensolaris.org/~mattman/gui_install_ep
>
> Thanks
>
> Matt
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
