Hi Matt,
A couple of comments inline...It looks ok though.
>>>
>>>
>>> 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.
Thanks! this helps.
>>>
>>>
>>> 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.
>
Fair enough. I don't have major heartburn, and if you are not in control
of this structure it is probably safer to check each member individually.
>>>
>>>
>>> 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.
I was asking if this list could have unused space, represented as a
logical partition in it? Sounds like the answer is no. Just checking.
>>
>>>
>>> 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.
>>
ok, makes sense.
>>>
>>> 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 ?
Yes, thanks.
sarah
*****
>>
>> 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
>>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
