[pacman-dev] [PATCH v2] Introduce alpm_dbs_update() function for parallel db updates

[Allan McRae]

On 27/3/20 6:17 am, Anatol Pomozov wrote:
> Hi

Most comments were addressed.  See below:

> On Thu, Mar 12, 2020 at 6:16 PM Allan McRae <allan at archlinux.org> wrote:
>>
>> On 9/3/20 6:59 am, Anatol Pomozov wrote:
<snipping...>

>>> +
>>> +     /* attempt to grab a lock */
>>> +     if(_alpm_handle_lock(handle)) {
>>> +             GOTO_ERR(handle, ALPM_ERR_HANDLE_LOCK, cleanup);
>>> +     }
>>> +
>>
>> This should be done much earlier in the function.
> 
> Sure I can move it above to make it more consistent with the rest of the code.
> 
> Although technically we need this lock only when we access
> concurrently modifiable data on disk (such as database storage). The
> code above iterates over non-modifiable list and reads db properties
> that are also read-only.

We don't have particularly fine grained locking here, so I prefer the
function to bail as early as possible.

<snip>

>>> +     for(i = dbs; i; i = i->next) {
>>> +             alpm_db_t *db = i->data;
>>> +             if(!(db->usage & ALPM_DB_USAGE_SYNC)) {
>>> +                     continue;
>>> +             }
>>> +
>>> +             /* Cache needs to be rebuilt */
>>> +             _alpm_db_free_pkgcache(db);
>>> +
>>> +             /* clear all status flags regarding validity/existence */
>>> +             db->status &= ~DB_STATUS_VALID;
>>> +             db->status &= ~DB_STATUS_INVALID;
>>> +             db->status &= ~DB_STATUS_EXISTS;
>>> +             db->status &= ~DB_STATUS_MISSING;
>>> +
>>> +             /* if the download failed skip validation to preserve the download error */
>>> +             if(sync_db_validate(db) != 0) {
>>> +                     /* pm_errno should be set */
>>> +                     ret = -1;
>>
>> There are some issues here...   If one database fails to download, but
>> the rest succeed, we skip validating the databases that worked.
> 
> Yep, if any file fails to download then we go to cleanup right after
> _alpm_multi_download().
> And then error code -1 returned to the client. Isn't it what we want?
> 
>> Even if all succeeded, we only set the error for the validation failure
>> for the last database in the list.
> 
> return code is set to "-1" if *any* db fails to validate.
> 

My concern was about fine grained errors being lost but...

>>> +             }
>>> +     }
>>> +
>>> +cleanup:
>>> +     _alpm_handle_unlock(handle);
>>> +
>>> +     if(ret == -1) {
>>> +             /* pm_errno was set by the download code */
>>> +             _alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync dbs: %s\n",
>>> +                             alpm_strerror(handle->pm_errno));
>>
>> Following on from above, this needs work.  There could be multiple
>> distinct failures on different databases, and only the last one is
>> reported.
> 
> Per database download events (such as download progress, download
> error, up-to-date) are going to be reported via callback.
> Thus client will have a chance to react to any issues right after the
> download is complete (or failed).
> 
> The idea was proposed here
> https://www.mail-archive.com/pacman-dev@archlinux.org/msg17770.html
> and implemented in my patch set here
> https://github.com/anatol/pacman/commit/ab3412fe0aa2d8bb64f524c6849664738cb95112#diff-38617b063ba9856428120eb6885c1a5dR907.
> 
> The CL for the download callback will follow later.
> 

... this looks like it will do what I want to be able to handle.  I had
been working on a patchset that would download dbs to a temporary
location and only move them to the sync db location once validated.

Seems there is enough individual error handling to handle that, or that
can be alter to handle that.

I will put the patch on my patchqueue branch.

A