Sarah,
Thanks a million for the review.
I've updated webrev with changes based on comments from code review.
http://cr.opensolaris.org/~mattman/gui_install_ep
One more point which I'd like to raise for discussion, this was initially
raised
by Darren, and was probably discussed at large when gui-installer was first
review but just wanted to make sure.
Issue is around value being used to convert MegaBytes to GigaBytes. Currently
the GUI uses 1024, there are arguments that this should be 1000 :
ref: http://en.wikipedia.org/wiki/Gigabyte
http://www.kernel.org/doc/man-pages/online/pages/man7/units.7.html
Thoughts ?
cheers
Matt
Sarah Jelinek wrote:
> 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).
Will do, the EP GUI enhancement bug has been created, and this also fixes one
other bug, will update comments fo next round of webrev.
>
>
> 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.
I've moved these functions into error-logging.[ch] makes sense to have this
type
of logging contained in here.
> -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.
To help here, I've moved all the blkorder functions out of
installation-disk-screen.c and into new files disk-block-order.[ch], coupled
with the print functions reduces installation-disk-screen.c by over a grand in
lines of 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?
sizechange is used to indicate that the partition size has been changed by the
user, and is therefore used to display the size change warning.
initialsizechange (not used for logicals, I've removed it), for primaries it is
used to indicate that this partition is being changed for the first time. This
is pertinent to extended partition types only, as on an initial size change for
an extended partition, all existing logicals are destroyed and shrunk down to
just one unused block. Subsequent size changes to this extended are
added/removed to unused space within the extended.
>
> 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.
Agreed in principle, but I'll let Darren comment for certain as he wrote this.
One point, we do not create these two partition_info_t structures that are
being
compared, they are passed to use from the orchestrator, therefore the GUI
cannot
guarantee a memset has been carried out on the originals. If the GUI can assume
orchestrator (or TD), do this, then a memcmp could be used.
>
>
> 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?
>
We are actually shifting them by num_rows, partlogicals is used to indicate the
current logicalpartition being processed. The calculation for which table row
this logical is being attached to is calculated using :
MainWindow.InstallationDiskWindow.partrow[i] + partlogicals;
MainWindw.InstallationDiskWindow.partrow[i] at this point has been adjusted by
num_rows (could be positive or negative). The comment could be adjusted to
reflect this information.
Changing to :
/*
* Loop through all children and shift down by num_rows
* The row to which this logical is attached is calculated
* MainWindow.InstallationDiskWindow.partrow[i], which has
* been adjusted already by num_rows.
*/
> 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 ++?
>
Removed 1242 idx = -1,
WRT why setting -1 and then ++, just left over loop logic, and looking at it
now uncessary.
I've changed :
initialize idx to 0 when declared.
Move idx++ into for statement itself :
e.g. for (; curlogical != NULL; curlogical = curlogical->next, idx++) {
Should be clearer now.
> 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 :-).
OM_NUMPART is now defined as (or will be once William commits) :
#define OM_NUMPART (FD_NUMPART + MAX_EXT_PARTS)
>
> 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.
Not sure what you mean here, If can never be a gap in the linked list,
if the item is NULL then it can't point to the next item, so therefore we
are at the end of the singly linked list. There can never be a gap in the list.
The pointer to the start of the list here, will always contain at least one
item, so therefore we will always be appending at the end.
>
> 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?
Initial call at 1499 to set lidx is only called if lidx is == -1 when passed
into this routine. at 1512, we cannot guarantee that it was actually called
therefore we need to call it again.
>
> line:
>> * Ensure we have an entry in modifiedblkorder[acivedisk]
>
> nit: should be [activedisk].
Done
>
> 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.
g_new0() would only ever return NULL of the 2nd paramater was <= 0. If it
failed
it would indicate machine is out of memory, which would be pretty serious. I've
not seen such checks done before.
>
>
> 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.
Wrote two new utility functions which are now used extensively throughout to set
spin button values/ranges and avail label text :
set_size_widgets_from_value()
set_range_avail_from_value()
>
> line:
>> if (parttype == UNUSED &&
>> 2528 +
>> ((curblkorder->partinfo.partition_size > 0 && diffgb > 0) ||
>> 2529 + (diffgb < 0))) {
> Why can't we just check diffgb !=0 ?
Specifically, if diffgb > 0, we only want to enter this function if the
partition size is also > 0 (and unused), as we would have some unused space to
consume.
If diffgb < 0, means we are returning some unused space to the pool so we are
not bothered what is currently in the unused partition_size, as we will simply
be adding this value to it e.g.
curblkorder->partinfo.partition_size -= orchestrator_om_gbtomb(diffgb);
We could change te > and the < to both be !=, both would still be required.
Leaving the > and < in place provides some reasoning (I thought) as to why one
is coupled with partition_size check.
Make sense ?
I've placed following comment above if :
/*
* If partition_size > 0 and diffgb > 0, means we
* are consuming used space, and there is some in
* this unused partition to consume from.
* If diffgb < 0, we are returning some space to
* the unused pool, there fore we are not bothered
* what partition_size is, as we just add to it.
*/
> installation-disk-screen.h:
> line:
>> 34 +#define GUI_INSTALL_FDISK_TABLE_ROWS 6
>
> Why is this value now 6?
The default number of rows in the GtkTable as defined in installatoindisk.glade
is 6, this contains by default, 4 rows for primaries, 1 row for column labels
and 1 row for Reset Button at the end.
Again thanks for the review.
Matt
>
>
> 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
