Hi William,
Comments inline.
> 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.
ok.
>> 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.
It isn't wrong. That's not my point. It seems to me that the "optional"
part of this tag makes it unclear that the default is false, or that it
is even there. In looking at the code in auto_parse.c, we look for this,
and then search to see if it is set to "true". Maybe this is due to the
fact of the way the default settings work in xml and with our manifest
server. Seems to me, it existence, or not, is the thing that tells us if
it is set. Setting an optional tag with a boolean value seems
counterintuitive to me.
In looking at the way we defined this in the AI schema, we ask the user
to specify if a partition they want to create or use is a logical
partition, correct? Which means we should have an extended partition
that we create this logical partition on, correct? If this is the case,
the extended partition definition in the AI manifest, if this was
required, and then any associated logical partitions would be an
explicit way of indicating that the partition is logical. How do we
handle the creation of a logical partition without the corresponding
extended partition? Perhaps we don't create logical partitions in AI? It
isn't clear to me in the code.
All of that said, it is ok the way it is.
>> 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.
Right.. I get that. My point was that at this point we have the
pinfo->partition_id value populated in the structure. Instead of using a
separate variable, "ipar" can't we return the pinfo->partition_id >
FD_NUMPART?
>> 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;
Ok.
>
>
>>
>>
>>> 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.
>
Ok, I get it now. thanks.
>>> /* 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>)
Ok.
>>
>>
>> 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.
ok, thanks.
>>> 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.
Ok, thanks.
sarah
*****
> 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
