On 09/21/10 02:33 AM, Jan Damborsky wrote:
Hi Keith,
thank you very much for code review.
Please see my response in line.
Jan
On 09/20/10 06:40 PM, Keith Mitchell wrote:
Hi Jan,
The comments in the manifest file indicate that only "dcfs" and
"none" are valid, but the RNG file indicates that 'gzip' is still ok.
'gzip' and 'none' are applicable to x86 platform while 'dcfs' and
'none' are applicable to Sparc.
I have put additional comment into RNG file to clarify that.
Ah, right. Thanks for the reminder.
boot_archive_archive.py:
The checks at 136-137 seem redundant with the checks at 173.
I did not introduce that code, so I can just guess why it is there.
It seems to me that check at 136-137 serves slightly different purpose -
I guess its intent is to validate user input and abort if
'fiocompress' entries
specified in DC manifest are invalid.
On the other hand, check at 173 serves for skipping entries not
eligible for compression.
But this is just my guess and if you think that validating content
of DC manifest is not needed, I am fine with removing that check.
Ah, yes, I now see that 136-137 is a manifest validation block. This
code is fine as is then.
Out of curiosity, what's the reasoning behind prepending everything
with "./" and then stripping out that same "./" on line 168?
By prepending paths with './' we are making sure that following
exclusion algorithm works as expected:
compress_fset = set(ba_flist) - set(exp_uc_flist)
Since find(["."]) at 95 (creates list containing all boot archive
entries)
generates paths with leading './', we need to assure that
list of entries not eligible for compression also start with './'.
That makes sense. For new code in the future, I believe use of os.path
functions (rather than manually modifying strings) will allow better
manipulation of file path strings to ensure that code such as the set
operations used here work properly. Since such a change would need to
occur across the whole module at once to be truly effective, I don't see
a need to modify this code at this time.
With respect to later stripping that "./", as I have verified that
fiocompress
works fine with paths containing './', I removed line 168.
168: This lstrip will cause problems with files that start with ".":
>>> "./.hidden_file".lstrip("./")
'hidden_file'
Good catch. I haven't noticed that, since it affected only non-zero
dot files in root directory of boot archive, in particular it caused
that /.volsetid remained uncompressed.
That said, it is no longer an issue, as I removed that code.
I have created updated webrev containing those changes:
http://cr.opensolaris.org/~dambi/bug-6984618-cr
Everything looks good now. Thanks!
- Keith
- Keith
On 09/20/10 08:53 AM, Jan Damborsky wrote:
Hello,
could I please get a code review for fix for following bug:
6984618 Not preserving hard links in boot archive during
fiocompression affects Sparc text media
webrev is available at:
http://cr.opensolaris.org/~dambi/bug-6984618/
Thank you,
Jan
tests accomplished:
[1] Sparc AI
* AI Sparc image based on 148 was built with DC containing the fix
* using that image, build 148 was installed
[2] Sparc text installer
* Text Sparc image based on 147 was built with DC containing the fix
* using that image, build 147 was installed
* resulting system was updated via pkg image-update to build 148
[3] regression tests
* x86 LiveCD image based on 148 was built with DC containing the fix
* using that image, build 148 was installed in Virtual Box guest
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
