Sarah,
Sarah Jelinek wrote:
> 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.
OK
> ...
> ai_manifest.rng:
>
> -Seems like if we need the tag "partition_is_logical" it should not be
> optional.
I don't follow the logic here.
> What is the default in this case?
The default is 'false', that it is not logical.
> I assume we create a primary partition?
yes, or extended
> Is there a better way to have this indicated?
Please explain what is wrong with the way it is 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?
This function checks for logical partitions. Assuming there is no error
in the way the struct is populated, an extended partition can never be a
a logical 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.
Since disk_parts_t *committed_disk_target->dparts already dimensions the
pinfo array, the correct size is defined for partition_info_t array:
typedef struct {
char *disk_name; /* Disk Name for look up */
partition_info_t pinfo[FD_NUMPART + MAX_EXT_PARTS]; /* fdisk */
} disk_parts_t;
>
>
>> 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?
>
Yes, that seems cleaner and more consistent.
>
>> 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.
This is in om_validate_and_resize_disk_partitions(). The particular
adjustment here is to check for the 1st logical partition of the
extended partition. It, as all logical partitions, should have 63
unused blocks before it. It occurs to me that this is perhaps
redundant, since om_create_partition() also performs this padding, but
it may still be useful for other users that perhaps do not use
om_create_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?
We are making sure the 1st logical partition in starting offset order
has appropriate padding (63 unused blocks), so we compare the starting
offset with the starting offset of the extended partition. If we don't
pad logicals, fdisk fails. To make the check more thorough, it could
check all logical partitions for the padding, but since
om_create_partition() does that already, there is no need at the moment.
>
>> /* 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?
Yes, it will be documented that if the user specifies a logical
partition and that the maximum size is requested, that maximum size will
be from the extended partition.
(<partition_is_logical>true</partition_is_logical> and
<partition_size>max_size</partition_size>)
>
>
> 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?
The partition array is indexed by partition number as in ctdpN, where N
is 1-4 for primary/extended and 5-36 for logicals, and the index is
offset from zero. To delete a partition in this scheme, you mark
partition table index N-1 as unused. Jan pointed out that this indexing
change, which I made to simplify the code, was not adequately
documented. I will do this.
>
>> 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().
Indeed. Will change.
>
>> 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>
Right, but we did this only for testing the new libdiskmgt.h - this will
be as you wish for the final update.
Thank you,
William
>
> 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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
