PERFORCE change 178984 for review

[Garrett Cooper]

On Sun, May 30, 2010 at 4:35 PM, Ivan Voras <ivoras at freebsd.org> wrote:
> http://p4web.freebsd.org/@@178984?ac=10
>
> Change 178984 by ivoras at betelgeuse on 2010/05/30 23:34:24
>
>        Step 4: (not finished): Start creating the actual patch binary
>
> Affected files ...
>
> .. //depot/projects/soc2010/pkg_patch/src/patch/Makefile#8 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/hashjob.c#7 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/hashjob.h#7 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/main.c#8 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/mkpatch.c#6 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/mkpatch.h#6 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/pkg_patch.h#6 edit
> .. //depot/projects/soc2010/pkg_patch/src/patch/support.c#5 edit
>

[...]

> +
> +       /*
> +        * XXX: Possibly reimplement with libarchive. If I finally get how it
> +        * stores directories.
> +        */
> +       sprintf(tmp, "%s/%s", dpatch, PKGPATCH_FNAME);

snprintf ? This will still be an issue because you have to use
archive_entry_copy_pathname anyhow for just files anyways.

> +       fp = fopen(tmp, "w");
> +       if (fp == NULL)
> +               err(1, "Cannot open file for writing: %s", tmp);
> +       time(&tm);

from time(3):

DESCRIPTION
     The time() function returns the value of time in seconds since 0 hours, 0
     minutes, 0 seconds, January 1, 1970, Coordinated Universal Time.  If an
     error occurs, time() returns the value (time_t)-1.

     The return value is also stored in *tloc, provided that tloc is non-null.

ERRORS
     The time() function may fail for any of the reasons described in
     gettimeofday(2).

> +       fprintf(fp, "# FreeBSD package patch archive created on %s\n",
> +               ctime(&tm));
> +       fprintf(fp, "@version %s\n", PKGPATCH_VERSION);
> +       parse_package_name(fold, tmp, tmp2, NULL);
> +       fprintf(fp, "@source %s-%s\n", tmp, tmp2);
> +       parse_package_name(fnew, tmp, tmp2, NULL);
> +       fprintf(fp, "@target %s-%s\n", tmp, tmp2);
> +       SLIST_FOREACH(fl, &fldiff_new_old, linkage)
> +               fprintf(fp, "@add %s\n", fl->filename);
> +       SLIST_FOREACH(fl, &fldiff_old_new, linkage)
> +               fprintf(fp, "@remove %s\n", fl->filename);
> +       SLIST_FOREACH(fl, &flchanged, linkage)
> +               fprintf(fp, "@patch [method=cp] %s\n", fl->filename);
> +       if (fclose(fp) != 0)
> +               err(1, "Cannot close %s", PKGPATCH_FNAME);
> +
> +       /* Include all metadata files from the new package. */
> +       SLIST_FOREACH(fl, &flnew, linkage) {
> +               if (fl->filename[0] == '+') {

I talked to kientzle about this in the past and while he and I both
agree that + prefixed files are a package metadata, the problem is
that it's not a widely held standard and non-conforming packages could
definitely cause minor performance and functional problems in this
area if someone upstream creates a package with filenames prefixed in
the `+'.

With the current mess trying to integrate archive(5) support into
pkg_create, I have deeply considered creating a directory within each
package called .fpkg-metadata/ where the metadata files will live.
That way it would make the purpose of said files easy, and would make
it easy to extract the files and inspect them.

It would require a trivial amount of rework within pkg_install. My
only concern would be with existing tools outside of pkg_install that
might depend on +CONTENTS existing within the package at a particular
location.

> +                       sprintf(tmp, "%s/%s", dnew, fl->filename);
> +                       sprintf(tmp2, "%s/%s", dpatch, fl->filename);

snprintf (x2) ?

> +                       if (copy_file_absolute(tmp, tmp2) != 0)
> +                               err(1, "Cannot copy file: %s to file: %s",
> +                                  tmp, tmp2);
> +               }
> +       }
> +
> +       /* Simply copy the directory hierarchy of the new package. */
> +       replicate_dirtree(dnew, dpatch);
> +
> +       SLIST_FOREACH(fl, &flchanged, linkage) {
> +               sprintf(tmp, "%s/%s", dnew, fl->filename);
> +               sprintf(tmp2, "%s/%s", dpatch, fl->filename);
> +               if (copy_file_absolute(tmp, tmp2) != 0)
> +                       err(1, "Cannot copy file: %s to file: %s", tmp, tmp2);
> +       }
> +
> +       sprintf(tmp, "%s -c -j -C %s -f %s *", _PATH_TAR, dpatch, fpatch);
> +       fp = popen(tmp, "r+");
> +       if (fp == NULL)
> +               err(1, "Final tar execution failed for: %s", fpatch);
> +       rm_rf(dold);
> +       rm_rf(dnew);

There's existing code in pkg_delete that matches this purpose.

[...]

> +void
> +parse_package_name(char *pkgfile, char *basename, char *version, char *suff)
> +{
> +       char *tmp, *p;
> +
> +       /* Strip directory path, if any */
> +       p = strrchr(pkgfile, '/');
> +       if (p != NULL)
> +               tmp = strdup(p + 1);
> +       else
> +               tmp = strdup(pkgfile);

This could be done using basename(3) (and to that effect you could
drop tmp, maybe...). I would look at the functions implemented through
pkg_version as there might be some code in there that can be leveraged
for what you're doing here.

> +       p = strrchr(tmp, '.');
> +       if (suff != NULL)
> +               strcpy(suff, p);
> +       *p = '\0';
> +       p = strrchr(tmp, '-');
> +       if (version != NULL)
> +               strcpy(version, p + 1);
> +       *p = '\0';
> +       if (basename != NULL)
> +               strcpy(basename, tmp);
> +       free(tmp);
> +}
> +
> +/*
> + * File copy, preserving attributes: ownership, mtime, mode. Knows how to handle
> + * (re-create) symlinks.
> + */

This doesn't handle POSIX extended attributes nor ACLs.

> +int
> +copy_file_absolute(char *from, char *to)
> +{
> +       char *buf;
> +       const ssize_t bufsize = 256*1024;
> +       ssize_t bs;
> +       int fdfrom, fdto;
> +       struct stat st;
> +       struct timeval tv;
> +
> +       if (lstat(from, &st) != 0)
> +               return (errno);
> +
> +       if (S_ISLNK(st.st_mode)) {
> +               char tmp[PATH_MAX];
> +
> +               if (readlink(from, tmp, PATH_MAX) < 0)
> +                       return (errno);
> +               if (symlink(tmp, to) < 0)
> +                       return (errno);
> +               return (0);
> +       }
> +
> +       fdfrom = open(from, O_RDONLY);
> +       if (fdfrom < 0)
> +               return (errno);
> +       fdto = open(to, O_CREAT | O_WRONLY | O_TRUNC);
> +       if (fdto < 0)
> +               return (errno);
> +       buf = malloc(bufsize);
> +       if (buf == NULL)
> +               return (ENOMEM);
> +       while (1) {
> +               bs = read(fdfrom, buf, bufsize);
> +               if (bs < 0)
> +                       err(1, "read() failure");
> +               else if (bs > 0)
> +                       if (write(fdto, buf, bs) != bs)
> +                               err(1, "write() failure");
> +               if (bs == 0)
> +                       break;
> +       }
> +       free(buf);
> +       close(fdto);
> +       close(fdfrom);
> +
> +       if (chown(to, st.st_uid, st.st_gid) < 0)
> +               return (errno);
> +       tv.tv_usec = 0;
> +       tv.tv_sec = st.st_mtime;
> +       if (lutimes(to, &tv) < 0)
> +               return (errno);
> +       if (lchmod(to, st.st_mode) < 0)
> +               return (errno);
> +       return (0);
> +}

Thanks!
-Garrett