Hi!
On Fri, 2010-04-09 at 09:02:22 +0200, Raphael Hertzog wrote:
> I want to push this patch in the next update (1.15.6.2).
Ah perfect! I wanted to fix this too for the next release, glad you
got to it while I was away.
> Guillem, a quick ok from you would be nice (feel free to point stylistic
> issues if you have some).
Overall the patch looks good, I had in mind doing it in just two
passes by storing enough information, but this is fine for now, the
rest can be dealt with in future refactoring work. But see below:
> diff --git a/src/archives.h b/src/archives.h
> index 2390bb8..0ee8057 100644
> --- a/src/archives.h
> +++ b/src/archives.h
> @@ -35,6 +35,12 @@ struct pkg_deconf_list {
> struct pkginfo *pkg_removal;
> };
>
> +struct rename_list {
> + struct rename_list *next;
> + char *src;
> + char *dest;
I'd name this dst for consistency with src (the rest of the new code
uses this too).
> +};
> +
And ‘struct rename_list’ does not need to be exposed, it can just placed
in processarc.c.
> extern int fnameidlu;
> extern struct varbuf fnamevb;
> extern struct varbuf fnametmpvb;
> diff --git a/src/processarc.c b/src/processarc.c
> index fe5ca1f..b596c79 100644
> --- a/src/processarc.c
> +++ b/src/processarc.c
> @@ -118,6 +118,7 @@ void process_archive(const char *filename) {
> struct dirent *de;
> struct stat stab, oldfs;
> struct pkg_deconf_list *deconpil, *deconpiltemp;
> + struct rename_list *rename_head = NULL, *rename_temp = NULL;
I'd name rename_temp as rename_node instead.
>
> cleanup_pkg_failed= cleanup_conflictor_failed= 0;
> admindirlen= strlen(admindir);
> @@ -850,19 +851,35 @@ void process_archive(const char *filename) {
> varbufaddstr(&infofnvb,de->d_name);
> varbufaddc(&infofnvb,0);
> strcpy(cidirrest,p);
> - if (!rename(cidir,infofnvb.buf)) {
> + /* We keep files to rename in a list as doing the rename immediately
> + * might influence the current readdir(), the just renamed file might
> + * be returned a second time as it's actually a new file from the
> + * point of view of the filesystem. */
> + rename_temp = m_malloc(sizeof(struct rename_list));
Here use sizeof(*rename_node) so it's resilient against type change.
> + rename_temp->next = rename_head;
> + rename_temp->src = m_strdup(cidir);
> + rename_temp->dest = m_strdup(infofnvb.buf);
> + rename_head = rename_temp;
> + }
> + pop_cleanup(ehflag_normaltidy); /* closedir */
> +
> + for(rename_temp = rename_head; rename_temp; rename_temp = rename_head) {
Missing space after ‘for’, but to avoid the redundancy I'd use instead:
while ((rename_node = rename_head)) {
> + if (!rename(rename_temp->src, rename_temp->dest)) {
> debug(dbg_scripts, "process_archive info installed %s as %s",
> - cidir, infofnvb.buf);
> + rename_temp->src, rename_temp->dest);
> } else if (errno == ENOENT) {
> /* Right, no new version. */
> - if (unlink(infofnvb.buf))
> - ohshite(_("unable to remove obsolete info file
> `%.250s'"),infofnvb.buf);
> - debug(dbg_scripts, "process_archive info unlinked %s",infofnvb.buf);
> + if (unlink(rename_temp->dest))
> + ohshite(_("unable to remove obsolete info file `%.250s'"),
> rename_temp->dest);
> + debug(dbg_scripts, "process_archive info unlinked %s",
> rename_temp->dest);
> } else {
> - ohshite(_("unable to install (supposed) new info file
> `%.250s'"),cidir);
> + ohshite(_("unable to install (supposed) new info file `%.250s'"),
> rename_temp->src);
Both ohshite node arguments could get wrapped into the next line to
avoid exceeding line length.
> }
> + rename_head = rename_temp->next;
> + free(rename_temp->src);
> + free(rename_temp->dest);
> + free(rename_temp);
> }
> - pop_cleanup(ehflag_normaltidy); /* closedir */
>
> *cidirrest = '\0'; /* the directory itself */
> dsd= opendir(cidir);
With those:
Acked-by: Guillem Jover <guil...@debian.org>
thanks,
guillem
--
To UNSUBSCRIBE, email to debian-dpkg-bugs-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
