Non-technical users will always be confused because disk sizes reported by a system *never* matches what the disk manufacture's spec. Take a new disk and format it and the "available" space is always smaller than the advertised capacity because some of the disk use is hidden from the user, e.g. boot blocks, file system directory blocks, hidden files, etc. Admittedly the difference becomes more noticeable as disk sizes get larger.
Since disks continue to be laid out in binary-based block sizes, and because RAM sizes continue to be binary-based, I feel we should continue to use binary-based disk sizeso. We could change the units indicators from "GB" to "GiB" which could serve as a clue to non-tech users that there is something going on. However, since Nautilus and other file system utilities on the GNOME desktop continue to use "GB" it might be more confusing than helpful. So I would prefer to leave the units as "GB" for now. Frank On 12/10/2009 12:08 AM, Darren Kenny wrote: > This issue of "what is a Gb" is also an issue for the text installer - which > is > also using 1024 (from what we saw on Tuesday) - they also raised this as a > possible issue. > > Frank, I think this is something you could provide some opinions on too, > especially from the perspective of what a non-technical User could expect. > > Thanks, > > Darren. > > On 12/ 9/09 05:11 PM, 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 >> >> >> 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 >>>> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
