On Wed, Nov 12, 2008 at 09:23:54PM -0800, Bart Smaalders wrote:
> Danek Duvall wrote:
>> On Wed, Nov 12, 2008 at 09:02:43PM -0800, Bart Smaalders wrote:
>>
>>>> - line 278: I'm not sure I see the point of these two tests. What
>>>> happens if you have two chattrs, the first one changes an attribute
>>>> of the original to something different, then the second one comes in
>>>> and changes it to the original. With this test, the second chattr
>>>> would be ignored, but I can't see why you'd want to ignore it more
>>>> than you'd want to ignore any other chattr.
>>>>
>>> The purpose of these two tests are accumulate only changes from each
>>> chattr* in f.changed_attributes... since each chattr is (confusingly)
>>> applied to the original file specification, unless we note which portions
>>> are changed by each chattr, subsequent changes by other
>>> chattrs will undo our previous modifications.
>>
>> I thought we already were potentially overwriting previous chattrs --
>> that's what the test on line 280 indicates. Can you give an example?
>>
>
> Ah... the changed_attr list before got the entire modified action;
> each chattr was applied to the original action.. So each change would
> completely replace the previous change, which was fine if they affected
> the same attribute but not if they affected different ones, since the
> previous change would be overwritten.
Yes -- that's the essence of the bug.
I guess what I'm expecting the code to look like is this:
each chattr/chattr_glob that applies to a particular pkgmap entry -- "f"
in all these cases adds entries into f.changed_attrs. If those entries
weren't there before, great, if they were, print out a warning message
and overwrite the previous value
once all the chattrs are accumulated into f.changed_attrs, that
dictionary is used to add and overwrite entries in the final action's
attr dict.
And that's what the code does, ignoring this test, right?
As far as I can tell, this test only allows a chattr to change an attribute
from its value in the original set of attributes (orig_action.attrs) if the
new value would be different from the old value -- if it would result in no
change, we ignore the chattr.
But like I said before, if you have multiple chattrs (triggering the
warning on line 281), and the second chattr provides an attribute value
that's identical to the original attributes, then it doesn't get applied,
leaving the first chattr's results to stick. In particular, the warning
never gets emitted, even though there was a second chattr.
A more concrete example:
SUNWfoo/pkgmap:
1 f none usr/bin/foo 0755 root bin <size> <csum> <timestamp>
import file:
1 import SUNWfoo
2 chattr usr/bin/foo mode=0644
3 chattr usr/bin/foo mode=0755
4 end import
Line 1 sets the mode of /usr/bin/foo to 0755 -- that's what's in the
package. Line 2 puts "mode": "0644" into f.changed_attrs. Line 3 will
note that "mode" is already in the original attributes and already has the
value "0755" there, so this chattr is ignored. Thus the final mode is
0644.
I think that the correct behavior is for line 3 to simply go ahead and
overwrite the 0644 in f.changed_attrs that was introduced by line 2 to
0755. Thus the final mode is 0755, and you get the warning telling you
that something may be wrong.
Am I going wrong somewhere in this analysis?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
