On 5/9/12 6:39 AM, Jan Damborsky wrote:
Hi Drew,

please see my comments below. They are all
related to 7094123, other bug fixes look
good to me.

Thank you,
Jan


general comment
---------------
I am not quite sure that suggested fix will correctly work
in all cases without changing related code handling
unconfiguration.

In particular, milestone-unconfig (responsible for removing
old SC profiles) seems to remove only profiles residing directly
in /etc/svc/profile/site/ directory, but doesn't remove
subdirectories, as those may contain user site profiles - those
should not be touched by sysconfig:

...
    #
    # Preserve profiles  in the /etc/svc/profile/site directory
    # which customize configuration process. All other profiles in that
    # directory are removed. However profiles located in sub-directories
    # of /etc/svc/profile/site are not altered. The command
    # "find $SMF_PROFILE_SITE_DIR/* -prune" effectively equates to max
    # depth 1, so only occurs in the $SMF_PROFILE_SITE_DIR.
    #
    find $SMF_PROFILE_SITE_DIR/* -prune -a ! -type d  \
        -a ! -name $CONFIG_XML ! -name $UNCONFIG_XML | xargs rm -f
...

I assume that problem will disappear with SC groupings project,
when it introduces different layout of SC profiles.

system-config/__init__.py
-------------------------

801-804 - this code cleans up temporary profile directory.
I am wondering if this needs some adjustment, as it currently
deletes only regular files. Perhaps we could just remove the
whole dir by os.rmtree(custom_profile_dir), then followed
by os.mkdir(custom_profile_dir).

838 - Looking at doc, os.path.split(options.profile)[1])))
would evaluate to empty string if options.profile contained
trailing slash. Do we need to somehow account for that case ?


Ugh. I had forgotten about that -prune flag in milestone-unconfig. You're right that this fix won't deal with that problem. I think I'm going to revert the change for 7094123 entirely but leave the other three fixes in as changing how milestone-unconfig works (that rm -f call is just terrible) is not something that we're going to fix until the SC groups project goes back.

I've respun the webrev with the fix backed out:

https://cr.opensolaris.org/action/browse/caiman/drewfish/sysconfig_fixes_3/webrev/

Can you take a quick look?

-Drew

_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Reply via email to