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@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/ab3412fe0aa2d8bb64f524c6849664738cb9....
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