PERFORCE change 179817 for review

[Garrett Cooper]

On Fri, Jun 18, 2010 at 5:31 PM, Julien Laffaye <jlaffaye at freebsd.org> wrote:
> http://p4web.freebsd.org/@@179817?ac=10
>
> Change 179817 by jlaffaye at jlaffaye-chulak on 2010/06/19 00:30:38
>
>        Errors checking, comments, ...
>
> Affected files ...
>
> .. //depot/projects/soc2010/pkg_complete/usr.sbin/pkg_install/complete/main.c#3 edit
> .. //depot/projects/soc2010/pkg_complete/usr.sbin/pkg_install/complete/perform.c#4 edit
>
> Differences ...
>
> ==== //depot/projects/soc2010/pkg_complete/usr.sbin/pkg_install/complete/main.c#3 (text+ko) ====
>
> @@ -28,6 +28,10 @@
>        if ((dir = dirname(argv[1])) == NULL)
>                err(1, "dirname(%s)", argv[1]);
>
> +       /*
> +        * Take the real path of the target file
> +        * because we are going to chdir()
> +        */
>        if ((realpath(argv[2], out)) == NULL)
>                err(1, "realpath(%s)", argv[2]);
>
> @@ -40,6 +44,6 @@
>  static void
>  usage(void)
>  {
> -    fprintf(stderr, "usage: pkg_complete source_package target_package\n");
> -    exit(1);
> +       fprintf(stderr, "usage: pkg_complete source-package target-package\n");
> +       exit(1);
>  }

It might be a good point to ask, what options are and aren't supported
that can be passed to package_complete (from a user perspective)?

This should have been hashed out earlier, but it never hurts to ask now..

> ==== //depot/projects/soc2010/pkg_complete/usr.sbin/pkg_install/complete/perform.c#4 (text+ko) ====
>
> @@ -22,23 +22,31 @@
>        struct stat st;
>        struct list_deps deps;
>        char fname[PATH_MAX], buf[1024], *ext;
> -       size_t r;
> +       ssize_t r;
>        int fd, err = 0;
>
> -       if ((ext = strrchr(pkgname, '.')) == NULL)
> +       if ((ext = strrchr(pkgname, '.')) == NULL) {
>                warn("strrchr()");
> +               return (1);
> +       }
>
>        deps.size = 100;
>        deps.len = 0;
>        deps.pkgs = malloc(sizeof(Package)*(deps.size));

What if this fails?

> +            == -1) {
> +               warnx("error while unpacking %s", CONTENTS_FNAME);
> +               return (1);
> +       }
> +
> +       pkg.head = pkg.tail = NULL;
> +       read_plist_from_buffer(&pkg, plist_buf, plist_size);
> +       free(plist_buf);

What if this fails?

> -               /* Register the current package */
> -               if (deps->size <= deps->len) {
> -                       deps->size *= 2;
> -                       deps->pkgs = realloc(deps->pkgs,
> -                                            sizeof(Package)*(deps->size));
> -               }
> -               deps->pkgs[deps->len] = pkg;
> -               deps->len++;
...

> +       /*
> +       * Get the dependencies of pkgname's dependencies,
> +       * if not already done.
> +       */
> +       for (p = pkg.head; p; p = p->next) {
> +               if (p->type == PLIST_PKGDEP) {
> +                       found = 0;

It'd be nice if found this was TRUE/FALSE.

Thanks!
-Garrett