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.
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
Move the tmpsect variable to align with other structure members.
> 606 device = malloc(len);
Should check for 'device' being not NULL after this call.
> 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?
> 536 int j;
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
