Joe, Joseph J VLcek wrote: > William Schumann wrote: >> New Automated Installer code allows creating partitions of different >> types. >> >> Place partition type value in element "partition_type". >> >> Allows "FAT32", "DOS16", "SOLARIS", "OPENSOLARIS" or any numeric type >> specified in fdisk.h >> >> http://cr.opensolaris.org/~wmsch/bug-6584/ >> http://defect.opensolaris.org/bz/show_bug.cgi?id=6584 >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > Looks good William. > > Please confirm my question, which is a possible confusion on my part > My suggestion may just be a style issue but it seemed clearer to me... > > Hope this helps! > > Joe > > Question: > --------- > File: usr/src/cmd/auto-install/ai_manifest.rng > > Please confirm line 195 correct. > > 195 <text/> Yes, this is the way that a text field is defined in an RNG file. The / at the end means that there is no closing </text> tag in XML. > > Just a suggestion: > ------------------ > File: usr/src/cmd/auto-install/auto_parse.c > > Would it seem clearer to negate the logic on line 362 and remove the > continue on line 362? > > From: > > 362 if (errno == 0 && endptr != p[i]) > 363 continue; > 364 auto_debug_print(AUTO_DBGLVL_ERR, > 365 "Partition type in manifest > (%s) is " > 366 "not a valid number or disk > format.\n", > 367 p[i]); > 368 *pstatus = 1; > 369 free(api); > 370 errno = 0; > 371 return (NULL); > > > To: > > if (!(errno == 0 && endptr != p[i])) { > auto_debug_print(AUTO_DBGLVL_ERR, > "Partition type in manifest (%s) is " > "not a valid number or disk format.\n", > p[i]); > *pstatus = 1; > free(api); > errno = 0; > return (NULL); > } BTW, I changed the wording when I revised the webrev in response to Jan's comments.
If I were to negate the conditional expression, I would use !(A && B) = (!A || !B), or (errno != 0 || endptr == p[i]). At this point in the code, we are at the 4th indentation level, so the maximum line length in Solaris coding standard of 80 characters will result in very short lines, which are increasingly messy and harder to read. I use the continue; to avoid yet another level of indentation, allowing the lines of the logging statement to be slightly longer. Otherwise the logging statement would look like this. > auto_debug_print(AUTO_DBGLVL_ERR, > "Partition type in manifest (%s) " > "is not a valid number or " > "partition type.\n", This use of continue; to skip the error handler might not be a very common practice, but it should still be easily understandable. William
