[pacman-dev] [PATCH 2/2] ensure package download sizes are invalidated when an error occurs

Jonathan Conder

16 May 2010 16 May '10

7:34 a.m.

Signed-off-by: Jonathan Conder --- lib/libalpm/sync.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index d9f4c28..9e140dc 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -855,9 +855,7 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) if(files) { EVENT(trans, PM_TRANS_EVT_RETRIEVE_START, current->treename, NULL); - errors = _alpm_download_files(files, current->servers, cachedir); - - if (errors) { + if(_alpm_download_files(files, current->servers, cachedir)) { _alpm_log(PM_LOG_WARNING, _("failed to retrieve some files from %s\n"), current->treename); if(pm_errno == 0) { @@ -869,12 +867,6 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) } } - for(j = trans->add; j; j = j->next) { - pmpkg_t *pkg = j->data; - pkg->infolevel &= ~INFRQ_DSIZE; - pkg->download_size = 0; - } - /* clear out value to let callback know we are done */ if(handle->totaldlcb) { handle->totaldlcb(0); @@ -1003,6 +995,12 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) ret = 0; error: + for(j = trans->add; j; j = j->next) { + pmpkg_t *pkg = j->data; + pkg->infolevel &= ~INFRQ_DSIZE; + pkg->download_size = 0; + } + FREELIST(files); alpm_list_free(deltas); return(ret); -- 1.7.1

Show replies by date

Nagy Gabor

16 May 16 May

2:54 p.m.

...

Signed-off-by: Jonathan Conder --- lib/libalpm/sync.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index d9f4c28..9e140dc 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -855,9 +855,7 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data)

if(files) { EVENT(trans, PM_TRANS_EVT_RETRIEVE_START, current->treename, NULL); - errors = _alpm_download_files(files, current->servers, cachedir); - - if (errors) { + if(_alpm_download_files(files, current->servers, cachedir)) { _alpm_log(PM_LOG_WARNING, _("failed to retrieve some files from %s\n"), current->treename); if(pm_errno == 0) { @@ -869,12 +867,6 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) } }

- for(j = trans->add; j; j = j->next) { - pmpkg_t *pkg = j->data; - pkg->infolevel &= ~INFRQ_DSIZE; - pkg->download_size = 0; - } - /* clear out value to let callback know we are done */ if(handle->totaldlcb) { handle->totaldlcb(0); @@ -1003,6 +995,12 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) ret = 0;

error: + for(j = trans->add; j; j = j->next) { + pmpkg_t *pkg = j->data; + pkg->infolevel &= ~INFRQ_DSIZE; + pkg->download_size = 0; + } + FREELIST(files); alpm_list_free(deltas); return(ret);

Khm, lol, when I wrote the last patch, I was too lazy and I simply overlooked this part. I "thought" that this code-part sets download_size to 0, but it _unsets_ it instead (see also my commit message). The spirit of this patch is better, but it is still not perfect, because of the -Sw (PM_TRANS_FLAG_DOWNLOADONLY) case. That return(0) can also be changed to goto error, and I can see one more RET_ERR in the function. I just note that in case of error-free -S commit operation, this code-part is needless/overkill, because the target pkgcache packages are replaced in the integrity-check part by the pmpkg_t of pkg.tar.xz file (line 949), and then every download_size is either 0 or unset. An other side note: I have some bad feelings when I see many foo->download_size in this codepart. We have a good access function for this. I know that atm all download_sizes are filled in correctly at the end of sync_prepare, but that may be changed later. But this direct access causes some nanosec speedup, so I dunno. :-) NG

Jonathan Conder

10:22 p.m.

Hi On Sun, 2010-05-16 at 14:54 +0200, Nagy Gabor wrote:

...

The spirit of this patch is better, but it is still not perfect, because of the -Sw (PM_TRANS_FLAG_DOWNLOADONLY) case. That return(0) can also be changed to goto error, and I can see one more RET_ERR in the function.

See my other patch for the -Sw case. I seem to have a tendency to overlook the RET_ERR ones though...

...

I just note that in case of error-free -S commit operation, this code-part is needless/overkill, because the target pkgcache packages are replaced in the integrity-check part by the pmpkg_t of pkg.tar.xz file (line 949), and then every download_size is either 0 or unset.

Ok, good point, I didn't notice that. In that case I think we should move this back to where it was originally, but make sure only packages for the current repo get unset. This might be a good time to introduce a pmpkg_t parameter to the download callback as well.

...

An other side note: I have some bad feelings when I see many foo->download_size in this codepart. We have a good access function for this. I know that atm all download_sizes are filled in correctly at the end of sync_prepare, but that may be changed later. But this direct access causes some nanosec speedup, so I dunno. :-)

I agree, but I don't mind either way. Since PM_ERR_PKG_INVALID_NAME is checked in the commit I'm not sure we even need the pre-computation any more. I'll have a closer look when I get the chance. Jonathan

Nagy Gabor

17 May 17 May

1:17 a.m.

...

Hi

On Sun, 2010-05-16 at 14:54 +0200, Nagy Gabor wrote:

...
The spirit of this patch is better, but it is still not perfect, because of the -Sw (PM_TRANS_FLAG_DOWNLOADONLY) case. That return(0) can also be changed to goto error, and I can see one more RET_ERR in the function.

See my other patch for the -Sw case. I seem to have a tendency to overlook the RET_ERR ones though...

Oh, I didn't see your other patch, sorry. :-)

...

...
I just note that in case of error-free -S commit operation, this code-part is needless/overkill, because the target pkgcache packages are replaced in the integrity-check part by the pmpkg_t of pkg.tar.xz file (line 949), and then every download_size is either 0 or unset.

Ok, good point, I didn't notice that. In that case I think we should move this back to where it was originally, but make sure only packages for the current repo get unset. This might be a good time to introduce a pmpkg_t parameter to the download callback as well.

Yes, I agree, it would be "nice" to refactor our download code. Then we could do this unset stuff in a more clever way. In addition, we could finally drop this ugly PM_TRANS_FLAG_DOWNLOADONLY flag, and allow the front-end to do the downloading manually, if needed. (Fake -Sw trans_commit has some other side effects, like stupid run_ldconfig implementation etc.) But this is not at the top of my TODO list... But patches are welcome. :-)

...

...
An other side note: I have some bad feelings when I see many foo->download_size in this codepart. We have a good access function for this. I know that atm all download_sizes are filled in correctly at the end of sync_prepare, but that may be changed later. But this direct access causes some nanosec speedup, so I dunno. :-)

I agree, but I don't mind either way. Since PM_ERR_PKG_INVALID_NAME is checked in the commit I'm not sure we even need the pre-computation any more. I'll have a closer look when I get the chance.

Personally I preferred your "original" patch, but now I am fine with this way. We have more important open bugs (I should not have mentioned this at all). :-) NG

Jonathan Conder

6:20 a.m.

On Mon, 2010-05-17 at 01:17 +0200, Nagy Gabor wrote:

...

Yes, I agree, it would be "nice" to refactor our download code. Then we could do this unset stuff in a more clever way. In addition, we could finally drop this ugly PM_TRANS_FLAG_DOWNLOADONLY flag, and allow the front-end to do the downloading manually, if needed. (Fake -Sw trans_commit has some other side effects, like stupid run_ldconfig implementation etc.) But this is not at the top of my TODO list... But patches are welcome. :-)

Ok, well I'm happy to leave it as is for now. I may write a patch in the future, although I don't think it's really my place to make big changes like that. So if you could just apply the first patch that would be good, although again it isn't urgent. Jonathan

Dan McGee

18 May 18 May

7:26 p.m.

New subject: [pacman-dev] [PATCH 2/2] ensure package download sizes are invalidated when an error occurs

On Sun, May 16, 2010 at 11:20 PM, Jonathan Conder wrote:

...

On Mon, 2010-05-17 at 01:17 +0200, Nagy Gabor wrote:

...
Yes, I agree, it would be "nice" to refactor our download code. Then we could do this unset stuff in a more clever way. In addition, we could finally drop this ugly PM_TRANS_FLAG_DOWNLOADONLY flag, and allow the front-end to do the downloading manually, if needed. (Fake -Sw trans_commit has some other side effects, like stupid run_ldconfig implementation etc.) But this is not at the top of my TODO list... But patches are welcome. :-)

Ok, well I'm happy to leave it as is for now. I may write a patch in the future, although I don't think it's really my place to make big changes like that. So if you could just apply the first patch that would be good, although again it isn't urgent.

Can someone sum up what needs doing here, if anything? I got a bit lost in all the emails (especially given that 25cd6c2e8da already got applied). -Dan

Jonathan Conder

19 May 19 May

6:17 a.m.

On Tue, 2010-05-18 at 12:26 -0500, Dan McGee wrote:

...

Can someone sum up what needs doing here, if anything? I got a bit lost in all the emails (especially given that 25cd6c2e8da already got applied).

I think the consensus was just to leave it for now. Another possible temporary solution I thought of would be to change 862: errors = _alpm_download_files(files, current->servers, cachedir); to 862: errors += _alpm_download_files(files, current->servers, cachedir); and move the error reporting further down. This might waste the user's time though, if it keeps downloading outdated packages when some of them have been removed from the mirror. Sorry for the confusion, and thanks for applying the other patch. Jonathan

Xavier Chantry

21 May 21 May

6:42 p.m.

New subject: [pacman-dev] [PATCH 2/2] ensure package download sizes are invalidated when an error occurs

On Sun, May 16, 2010 at 10:22 PM, Jonathan Conder wrote:

...

Hi

On Sun, 2010-05-16 at 14:54 +0200, Nagy Gabor wrote:

...
The spirit of this patch is better, but it is still not perfect, because of the -Sw (PM_TRANS_FLAG_DOWNLOADONLY) case. That return(0) can also be changed to goto error, and I can see one more RET_ERR in the function.

See my other patch for the -Sw case. I seem to have a tendency to overlook the RET_ERR ones though...

...
I just note that in case of error-free -S commit operation, this code-part is needless/overkill, because the target pkgcache packages are replaced in the integrity-check part by the pmpkg_t of pkg.tar.xz file (line 949), and then every download_size is either 0 or unset.

Ok, good point, I didn't notice that. In that case I think we should move this back to where it was originally, but make sure only packages for the current repo get unset. This might be a good time to introduce a pmpkg_t parameter to the download callback as well.

...
An other side note: I have some bad feelings when I see many foo->download_size in this codepart. We have a good access function for this. I know that atm all download_sizes are filled in correctly at the end of sync_prepare, but that may be changed later. But this direct access causes some nanosec speedup, so I dunno. :-)

I agree, but I don't mind either way. Since PM_ERR_PKG_INVALID_NAME is checked in the commit I'm not sure we even need the pre-computation any more. I'll have a closer look when I get the chance.

I did not really follow the discussion, and still don't have much time, sorry for that. I just wonder if this is related to the following debug message I saw with scriptlet002 : debug: returning error 32 from compute_download_size : package filename is not valid Remove (1): dummy-1.0-1 Total Removed Size: 0.00 MB Do you want to remove these packages? [Y/n] removing dummy... debug: removing package dummy-1.0-1

Jonathan Conder

22 May 22 May

5:40 a.m.

On Fri, 2010-05-21 at 18:42 +0200, Xavier Chantry wrote:

...

I did not really follow the discussion, and still don't have much time, sorry for that.

I just wonder if this is related to the following debug message I saw with scriptlet002 :

debug: returning error 32 from compute_download_size : package filename is not valid Remove (1): dummy-1.0-1

Total Removed Size: 0.00 MB

Do you want to remove these packages? [Y/n] removing dummy... debug: removing package dummy-1.0-1

I don't think it is related, but that error only occurs when the package filename is NULL. Maybe the pactest doesn't set it properly? Jonathan

5092

Age (days ago)

5098

Last active (days ago)

List overview

Download

8 comments

4 participants

participants (4)

Dan McGee
Jonathan Conder
Nagy Gabor
Xavier Chantry