[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [PATCH 2/2] Reimplement dpkg-divert in C



Twas brillig at 02:05:47 17.10.2009 UTC+02 when guillem@debian.org did gyre and gimble:

 GJ> Hi!

 GJ> For recurring issues I've commented on the first occurence only. And
 GJ> I've stopped reviewing in detail at some point as major parts of the
 GJ> code need to be rewritten anyway.

Wow. Thanks for really detailed review. I'll comment on questions below
and going to prepare a new patch.

 >> +static int
 >> +rename_mv(const char *src, const char *dest)
 >> +{
 >> +        pid_t pid;
 >> +
 >> +        if (rename(src, dest) == 0)
 >> +                return 0;
 >> +
 >> +        pid = fork();
 >> +        if(pid == -1)
 >> +                return -1;
 >> +
 >> +        if (!pid) {
 >> +                /* child */
 >> +                execlp("mv", "mv", src, dest, NULL);
 >> +                exit(EXIT_FAILURE);
 >> +        }
 >> +
 >> +        /* parent */
 >> +        int status;
 >> +        if(waitpid(pid, &status, 0) == -1)
 >> +                return -1;
 >> +
 >> +        return !(WIFEXITED(status) && WEXITSTATUS(status) == 0);
 >> +}

 GJ> Use the stuff from subproc.h. Or actually just copy it yourself, use
 GJ> fd_fd_copy and file_copy_perms.

Ah, did not know about it. One codepath less to check.

 >> +static diversion *
 >> +invert_diversions_list(diversion *diversions)
 >> +{
 >> +        diversion *n = NULL;
 >> +        while (diversions)
 >> +        {
 >> +                diversion *newhead = diversions;
 >> +                diversions = newhead->next;
 >> +                newhead->next = n;
 >> +                n = newhead;
 >> +        }
 >> +        return n;
 >> +}

 GJ> Instead of inverting the list after the fact, just add to its tail
 GJ> instead. OTOH I don't think this should be needed at all.

I have tried to replicate dpkg-divert.pl behaviour exactly, including
sorting of diversions file. If it is not necessary, I could make a
testsuite a bit smarter instead.

 >> +static int
 >> +match_diversion(diversion *diversion, char *const *patterns)
 >> +{
 >> +        for (; *patterns; patterns++) {
 >> +                if(!fnmatch(*patterns, diversion->contest, FNM_NOESCAPE))
 >> +                        return 0;
 >> +                if(!fnmatch(*patterns, diversion->altname, FNM_NOESCAPE))
 >> +                        return 0;
 >> +                if(!fnmatch(*patterns, diversion->package, FNM_NOESCAPE))
 >> +                        return 0;

 GJ> Why the FNM_NOESCAPE?

To match Perl code - '\' is escaped there. Not that it really matters.

-- 
  http://fossarchy.blogspot.com/

Attachment: pgpkfxea0KprU.pgp
Description: PGP signature


Reply to: