Hi Matt, Matt Keenan wrote: > 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
I will take another look this morning and get back to you shortly. > > > 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 > We had this discussion when we did Dwarf. First, we need to be consistent between our installers, whatever we do. I believe we used 1000 for disk storage because that is and was the recommendation. I still think it is valid for disk storage. sarah **** > 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 >
