Hi Sarah - Thank you very much for the code review. See my response below.... ginnie
On 10/15/09 12:13, jeanm wrote: > > > ------------------------------------------------------------------------ > > Subject: > Re: [caiman-discuss] Code review for the Install on Extended > Partitions project > From: > Sarah Jelinek <Sarah.Jelinek at Sun.COM> > Date: > Wed, 14 Oct 2009 13:51:43 -0600 > To: > jeanm <Jean.McCormack at Sun.COM> > > To: > jeanm <Jean.McCormack at Sun.COM> > CC: > caiman-discuss <caiman-discuss at opensolaris.org> > > > Hi Jean and team, > > Great work! I have a few comments: > > I tried not to repeat comments, if I have, point me to your answers to > the original submitter. > > -Please include the bug id's in the final webrev, just for completeness. I will be sure to do that. > > > partition.c: > > nit: > >> 535 #if defined(i386) || defined(__amd64) >> 536 int j; >> 537 ext_part_t *epp; /* extended partition >> structure */ >> 538 char *device; /* name of fixed disk >> drive */ >> 539 size_t len; >> 540 logical_drive_t *log_drv; /* logical drive >> structure */ >> 541 uint64_t tmpsect; >> 542 #endif > Fixed. > Move the tmpsect variable to align with other structure members. > >> 606 device = malloc(len); > > Should check for 'device' being not NULL after this call. Sundar also pointed that out. I've fixed it. > >> if ((libfdisk_init(&epp, device, &iparts[i], >> 611 FDISK_READ_DISK)) != FDISK_SUCCESS) >> 612 continue; > > If we cannot init or read this fdisk, should we just continue? Does it > mean there is no fdisk partition at that slot? If that is the case, is > there any other more fine grained status we can use to check against > to ensure better error reporting/handling? Jean and I talked about this. The way the extended partition team wrote their code they didn't centralize the error codes in one location. after each return from libfdisk_init, there is a switch statement that processes the return value. You can see an example of this here http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/boot/installgrub/installgrub.c#312. At the time of our discussion, we decided not to code it this way. I'm interested in your thoughts on this. > >> 536 int j; This is used in the for loop that starts at line 617. > > Don't think this is needed. > > Jan mentioned: >> I assume these changes are addressing bug 6539687. If this is the >> case, I think >> that the same modifications should be done in function >> partition_get_assocs() - >> lines 193-199 > > This does need to be changed in this function. Also, I think it needs > to be modified here as well: > >> 162 if (conv_flag) { >> 163 /* convert part. name (e.g. c0d0p0) */ >> 164 (void) snprintf(part_name, sizeof >> (part_name), "%s%d", >> 165 pname, i); >> 166 } else { >> 167 (void) snprintf(part_name, sizeof >> (part_name), "%d", i); >> 168 } > > > ai_manifest.rng: > > -Seems like if we need the tag "partition_is_logical" it should not be > optional. What is the default in this case? I assume we create a > primary partition? Is there a better way to have this indicated? > > > disk_parts.c: > >> 440 pinfo = committed_disk_target->dparts->pinfo; >> 441 for (ipar = 0; ipar < OM_NUMPART; ipar++, pinfo++) >> 442 if (is_used_partition(pinfo) && >> 443 pinfo->partition_type == SUNIXOS2) >> 444 return (ipar >= FD_NUMPART); > > Can't we check the pinfo->partition_id here for seeing whether this is > an extended partition? Also, how do we know we have OM_NUMPART worth > of pfino committed disk targets? We could hit a bad memory value here, > couldn't we? It isn't clear to me if liborchestrator allocates enough > memory for all possible partitions, or only all discovered partitions > for a disk? It may, I am just checking. > > >> if (p_new->partition_offset_sec == 0) { >> 768 /* move from 1st to 2nd >> cylinder */ >> 769 >> p_new->partition_offset_sec = >> 770 >> dt->dinfo.disk_cyl_size; > > Could call resize_partition() here, can't you? > > >> 828 } else if (extpinfo != NULL && >> 829 IS_LOG_PAR(p_new->partition_id) && >> 830 extpinfo->partition_offset_sec == >> 831 p_new->partition_offset_sec) { >> 832 /* pad for logical partition */ >> 833 resize_partition(p_new, >> LOGICAL_PARTITION_PAD); > > Seems to me that if we get here, we are: > > 1. Not doing a GUI allocation > 2. We know it is not the first partition > 3. Have figured out that the p_new is not a primary partition > 4. And are now adjusting this because we know it is a logical partition. > > I get why we check to see if extpinfo != NULL, but the other checks > seem unnecessary to me. Or, to put it another way, what happens if we > don't fall in to one of these conditionals? > >> /* if size set to OM_MAX_SIZE in manifest, take entire disk */ >> 1179 if (partition_size_sec == OM_MAX_SIZE) >> 1180 if (is_log_part) { >> 1181 partition_info_t *extpinfo; >> 1182 1183 om_debug_print(OM_DBGLVL_INFO, >> 1184 "allocating entire extended >> partition " >> 1185 "for logical partition\n"); >> 1186 extpinfo = >> get_extended_partition_info(); >> 1187 if (extpinfo == NULL) { >> 1188 >> om_debug_print(OM_DBGLVL_ERR, >> 1189 "system error: >> failed to find " >> 1190 "extended partition >> definition\n"); >> 1191 >> om_set_error(OM_NO_PARTITION_FOUND); >> 1192 return (B_FALSE); >> 1193 } >> 1194 partition_size_sec = >> 1195 >> (uint64_t)extpinfo->partition_size * >> 1196 BLOCKS_TO_MB; > > The code above changes the behavior from taking the entire disk to the > entire extended partition. Is this going to be expected by our users? > > > mark_for_deletion_by_index(): > -Do we not need to move the partitions up by one, in the case of > marking for deletion a primary partition? > >> if (is_log_part) { >> 2025 if (sorted_parts[0].partition_offset_sec > >> starting_sec + >> 2026 LOGICAL_PARTITION_PAD) >> 2027 append_free_space_table(starting_sec, >> 2028 sorted_parts[0].partition_offset_sec); >> 2029 } else if (sorted_parts[0].partition_offset_sec > >> starting_sec) { >> 2030 append_free_space_table(starting_sec, >> 2031 sorted_parts[0].partition_offset_sec); >> 2032 } > > Seems like this could be made cleaner by assigning a variable to the > value we are checking against sorted_parts[0].partition_offset_sec. We > do the same thing in both cases, that is append_free_space_table(). > >> if (is_log_part) >> 2116 partition_size += LOGICAL_PARTITION_PAD; > > Using the parameter, partition_size here seems confusing to me. I > realize it doesn't change the value to the caller, but it makes the > code hard to read. I would create a local function variable and have > it set to the correct value, then use that. > > td_dd.c: > >> 49 #include "libdiskmgt.h" > > Should be <libdiskmgt.h> > > thanks, > sarah > > ****** > jeanm wrote: >> >> Please review the following for the installation on extended >> partitions project: >> >> For the ON gate (questions should be directed to Ginnie Wray): >> >> http://cr.opensolaris.org/~ginnie/libdisk3/ >> >> >> For the slim_source gate (questions for libtd to Ginnie, rest of the >> code questions should be directed to William Schumann) >> >> file:///net/indiana-build.central/export/home/ws199450/extp/webrev/index.html >> >> >> >> >> Please respond by COB on Friday October 16. >> >> Thanks, >> >> Jean McCormack >> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091019/18d72f10/attachment.html>
