[Rpm-maint] [PATCH 2/2] Add a call for changing package header information.

[Jeff Johnson]

> On Apr 2, 2018, at 5:35 AM, Aleksei Nikiforov <darktemplar at altlinux.org> wrote:
> 
> Hi
> 
> 30.03.2018 18:35, Jeff Johnson пишет:
>>> On Mar 30, 2018, at 10:11 AM, Aleksei Nikiforov <darktemplar at altlinux.org> wrote:
>>> 
>>> Hi
>>> 
>>> 29.03.2018 15:55, Aleksei Nikiforov пишет:
>>>> Hi
>>>> 29.03.2018 15:51, Jeff Johnson пишет:
>>>>> 
>>>>> 
>>>>>> On Mar 29, 2018, at 4:46 AM, Aleksei Nikiforov <darktemplar at altlinux.org>
>>>>> 
>>>>> ...
>>>>> 
>>>>>>>>> Adding AUTOINSTALLED through a transaction like this is far more complex than necessary, particularly since rpm itself does not need nor use that tag for any purpose
>>>>>>>> I've looked at librpm API documentation a few moments ago and found rpmdbSetIteratorRewrite + rpmdbSetIteratorModified. Did you mean these interfaces or something else I missed which could be used for changing headers in rpm db?
>>>>>>> Yes, those interfaces. Note that those interfaces are traversed only in the case of a replaced file, which only rarely happens (multilib and sloppy packaging are the two common cases that come to mind).
>>>>>> 
>>>>> 
>>>>> Apologies for not being precise.
>>>>> 
>>>>> Within rpm code, those interfaces are traversed only in the case of a replaced file, and
>>>>> hence might be considered "lightly tested" or buggy.
>>>>> 
>>>>>> Patch 2 is used to provide interface which can be used for implementation of utilities like 'apt-mark auto' or 'apt-mark manual'. In that case no files are changed, no packages are actually reinstalled, only 1 header tag value is changed (or added, if tag AUTOINSTALLED was missing).
>>>>>> 
>>>>>>>> As for doing it within transaction, why not? As far as I can see almost every action on RPM db is done on open transaction, including rpmtsRebuildDB and rpmdbSetIteratorRewrite + rpmdbSetIteratorModified. Atomicity and consistency of transaction wouldn't hurt either.
>>>>>>> Here are the reasons not to implement as you have done:
>>>>>>> 0) rpm transactions are atomic/consistent only within very narrow meanings that have little to do with traditional meanings of ACID database transactions. E.g. The underlying Berkeley DB uses a CDB, not a transactional model. There are no logs nor rollbacks nor commit/discard events. Atomicity is only within an exclusive fcntl write lock, and consistency is only de facto, as in there is exactly one add/del operation per transaction element, with no well defined way to hide, say, an add/del operation until the rpm transaction is committed.
>>>>>>> 1) the rpmte interfaces aren't all that useful or compelling as an API. I can't think of any application that has used/needed access to rpmte transaction elements, because applications already have the headers from which the transaction was composed.
>>>>>>> 2) There isn't any known usage case within rpm for AUTOINSTALL, nor is there any means to compute the value within rpm which has no depsolver.
>>>>>>> Why should rpm start managing apt+rpm (my guess at what alt is doing) data? If apt wishes AUTOINSTALL, then apt, not rpm, should manage that data, and there are already sufficient API calls to perform that management task.
>>>>>> 
>>>>>> Yes, depsolver is external, outside of librpm, but it is desired to keep all information in one database, including AUTOINSTALL tag, and this tag may be needed to be changed after package installation.
>>>>>> 
>>>>>> How can it be implemented other way? If rpmdbSetIteratorRewrite + rpmdbSetIteratorModified only works when files are changed, then this wouldn't work for this use-case.
>>>>>> 
>>>>> 
>>>>> Just do what markReplacedFile() does, but in apt-mark, outside of rpm code.
>>>>> 
>>>> Thanks for clarification, I'll try doing it and see if it works for me.
>>> 
>>> I've tried using rpmdbSetIteratorRewrite + rpmdbSetIteratorModified and it didn't work for me.
>>> I've tried calling headerDel + headerPutUint32, headerGet + headerGetUint32 + writing new value to the pointed memory, headerMod. Every time it fails to write modified header, function headerExport hits following condition:
>>> 
>> Hmmm ... here's some context:
>> There are 3 cases for tags in a header, named akin to stalagmite formation in a cave.
>> 1) within the immutable region created when building a package (aka "rock")
>> 2) appended to the immutable region (aka "dribble")
>> 3) newly added tag data (aka "drip").
>> When a header is unloaded, a "drip" is appended, and thereby becomes a "dribble" when next loaded from an rpmdb.
>> Newly added "drips" can be freely added/deleted until a header is unloaded (I.e. saved in the rpmdb), "dribbles" can be modified through the pointer (if careful), and changes
>> to the immutable region "rock" can be done.
>> Similar to changing file states in markReplacedFile(), AUTOINSTALLED is a "dribble" when retrieved from an rpmdb, and you can modify by headerGet(MINMEM) and changing the value directly through the returned pointer.
>> The data type of AUTOINSTALL from int32 to int8, same as file states, to avoid alignment/padding issues with tag data. That's what usually breaks in headerExport()
> 
> I've used rpmtdGetChar instead of headerGetUint32 and it finally worked as expected. Thanks!

Good: that confirms that there is something screwy with "drip" alignment when unloading headers for re-writing through an iterator.

(aside)
I am not surprised: this was incredibly nasty code to implement way back when, and has never been used for anything but rewriting file states since. But is likely fixable with a little bit of work.

> I think patch 2 may be dropped since it's no longer required. But patch 1 looks still needed. Can patch 1 be merged?
> 
>>>    if (count != (rdlen + entry->info.count + drlen))
>>>        goto errxit;
>>>    }
>>> and returns NULL to function miFreeHeader.
>>> 
>> Can you get some values for those variables? My guess is that the problem is in darken computed across "dribbles" earlier in headerExport().
> 
> It's no longer relevant since it works now, but in case it might be needed here are values I got before using rpmtdGetChar:
> 
> i = 0, count = 2408, rdlen = 3424, entry->info.count = 16, drlen = 156, sum = 3596 (count != sum)
> 

Thanks!

>>> I was hitting similar issue when implementing current patches and worked around it by making a copy of header and working with it:
>>> 
>>>    case TR_CHANGED:
>>>        tmp_h = rpmteDBHeader(te);
>>>        h = headerCopy(tmp_h);
>>>        headerFree(tmp_h);
>>>        break;
>>> But I can't find a way to modify header returned by rpmdbNextIterator() and make rpmdbFreeIterator() successfully save changes, or a way to replace whole header from rpmdbMatchIterator. Am I missing some way to modify or replace header and successfully write changes to rpm db?
>>> 
>> An alternative approach is to retrieve the header, modify AUTOINSTALLED, and replace.
> 
> Is there an API to replace whole header? I didn't find any yet. I thought about copying all tags to temporary header, changing values there as needed, cleaning all tags from original header and copying modified tags back. I didn't try doing it yet though.
> 

There is an API to retrieve the "immutable header region" which is exactly what was in the *.rpm package.

The immutable header region is the plaintext blob on which rpm digests/signatures are based. So changing the header (or changing a "rock" tag) isn't just deleting tags, the immutable header region is wired throughout rpm package security validation.

73 de Jeff
>> hth
>> 73 de Jeff
>>>>> 73
>>>>>>> 73 de Jeff
>>>>>>>> Best regards
>>>>>>>> Aleksei Nikifo
>>>> Best regards
>>>> Aleksei Nikiforov
>>>> _______________________________________________
>>>> Rpm-maint mailing list
>>>> Rpm-maint at lists.rpm.org
>>>> http://lists.rpm.org/mailman/listinfo/rpm-maint
>>> 
>>> Best regards
>>> Aleksei Nikiforov
> 
> Best regards
> Aleksei Nikiforov