[packagekit] PackageKit: pkgenpack fixes

[Richard Hughes]

On Sat, 2009-01-24 at 19:35 +0530, Utsav Handa wrote:
> While using "pkgenpack", i came across some issues like -
> 1) If i give a file name in "--output" param, it ends with "Segmentation
> fault".
> 2) If "--package=" is passed as parameter, it ends with 
> Failed to find package '': Internal error: Search string zero length

Cool, both bugs, cheers for finding them.

> I decided to check out the source-code for pkgenpack and found that
> these problems can be rectified.

Kudos for checking out the source and fixing things. 

> Please find the patch for these issues and if possible merge it with
> core-code. 

I've added some comments to this mail, could you make the changes and
then please resend?

> (This is my first try with PackageKit code, so please be kind to newbie)

We welcome you with open arms :-) If anything is confusing or unknown
please shout on the mailing list and we'll explain this the best we can.

Comments follow:

On Sat, 2009-01-24 at 19:35 +0530, Utsav Handa wrote:
>  diff --git a/client/pk-generate-pack.c b/client/pk-generate-pack.c
> index df040ee..7a35d0f 100644
> --- a/client/pk-generate-pack.c
> +++ b/client/pk-generate-pack.c
> @@ -37,11 +37,14 @@
>  static guint last_percentage = 0;
>  static PkServicePack *pack = NULL;
>  
> +static gchar os_directory_separator    = '/';

You want to use G_DIR_SEPARATOR, although see below, as g_build_path
might be a better way to join paths.

> +const  gchar *pack_identifier          = ".servicepack";

You probably want to use:

#define PK_GENPACK_FILE_EXTENSION "servicepack"

>  /**
>   * pk_generate_pack_get_filename:
>   **/
>  static gchar *
> -pk_generate_pack_get_filename (const gchar *name, const gchar *directory)
> +pk_generate_pack_get_filename (const gchar *name, const gchar *directory, const gchar *custom_packname)
>  {
>         gchar *filename = NULL;
>         gchar *distro_id;
> @@ -49,12 +52,24 @@ pk_generate_pack_get_filename (const gchar *name, const gchar *directory)
>  
>         distro_id = pk_get_distro_id ();
>         if (name != NULL) {
> -               filename = g_strdup_printf ("%s/%s-%s.servicepack", directory, name, distro_id);
> +               
> +               /* Make user supecified pack */

specified

> +               if (custom_packname != NULL) {
> +                       filename = g_strdup_printf ("%s%c%s", directory, os_directory_separator, custom_packname);

I think you want to use g_build_path() in this case.

> +               } else {
> +                       /* Make default pack */
> +                       filename = g_strdup_printf ("%s%c%s-%s%s", directory, os_directory_separator, name, distro_id, pack_identifier);
> +               }

You can either do this, or use a temporary variable and then g_build_path.

>         } else {
>                 iso_time = pk_iso8601_present ();
>                 /* don't include the time, just use the date prefix */
>                 iso_time[10] = '\0';
> -               filename = g_strdup_printf ("%s/updates-%s-%s.servicepack", directory, iso_time, distro_id);
> +               if (custom_packname != NULL) {
> +                       filename = g_strdup_printf ("%s%c%s", directory, os_directory_separator, custom_packname);
> +               } else {
> +                       filename = g_strdup_printf ("%s%cupdates-%s-%s%s", directory, os_directory_separator, iso_time, distro_id, pack_identifier);
> +               }

Same comments apply.

>         g_free (distro_id);
>         g_free (iso_time);
> @@ -178,6 +193,7 @@ main (int argc, char *argv[])
>         gchar *package_list = NULL;
>         gchar *package = NULL;
>         gboolean updates = FALSE;
> +       gchar *extracted_packname = NULL;
>  
>         const GOptionEntry options[] = {
>                 { "verbose", 'v', 0, G_OPTION_ARG_NONE, &verbose,
> @@ -215,7 +231,7 @@ main (int argc, char *argv[])
>         egg_debug_init (verbose);
>  
>         /* neither options selected */
> -       if (package == NULL && !updates) {
> +       if ( (package == NULL || strlen(package) <= 0) && !updates) {

This probably wants to be egg_strzero() rather than (package == NULL ||
strlen(package) <= 0).

There's no point finding the length of a completely untrusted string
just to check if the first character is \0. Just a simple optimisation
that keeps things secure :-)

>                 /* TRANSLATORS: This is when the user fails to supply the correct arguments */
>                 g_print ("%s\n", _("Neither --package or --updates option selected."));
>                 g_print ("%s", options_help);
> @@ -230,6 +246,25 @@ main (int argc, char *argv[])
>                 return 1;
>         }
>  
> +
> +       /* Check for specified "Output" */
> +       if ( !g_file_test (directory, G_FILE_TEST_IS_DIR) ) {
> +
> +               /* Output specified is 'File", Check for pack_identifier */
> +               if ( !g_str_has_suffix (directory, pack_identifier) ) {
> +                       /* TRANSLATORS: This is when the output specified does not have pack_identifier ext. */
> +                       g_print ("%s %s \n", _("Output should be valid directory or packname must end with"), pack_identifier);

Can we tell which?

> +                       g_print ("%s", options_help);
> +                       return 1;
> +               } else {
> +                       /* Extract "Directory" and filename from "Output" */
> +                       extracted_packname = g_path_get_basename (directory);
> +                       directory              = g_path_get_dirname (directory);

Big space -- no reason to pad.

> +               }
> +
> +       }
> +
> +
>         /* fall back to the system copy */
>         if (package_list == NULL)
>                 package_list = g_strdup (PK_SYSTEM_PACKAGE_LIST_FILENAME);
> @@ -246,8 +281,9 @@ main (int argc, char *argv[])
>                 goto out;
>         }
>  
> +

No need to add extra line.

>         /* get fn */
> -       filename = pk_generate_pack_get_filename (package, directory);
> +       filename = pk_generate_pack_get_filename (package, directory, extracted_packname);
>  
>         /* download packages to a temporary directory */
>         tempdir = g_build_filename (g_get_tmp_dir (), "pack", NULL);
> @@ -258,7 +294,7 @@ main (int argc, char *argv[])
>         /*ask user input*/
>         if (exists) {
>                 /* TRANSLATORS: This is when file already exists */
> -               overwrite = pk_console_get_prompt (_("A pack with the same name already exists, do you want to overwrite it?"), FALSE);
> +               overwrite = pk_console_get_prompt ( g_strdup_printf("A pack with the same name (%s) already exists, do you want to overwrite it?", filename), FALSE );

If you use g_strdup_printf, you have to g_free the return value. Just
use a temporary gchar * variable.

>                 if (!overwrite) {
>                         /* TRANSLATORS: This is when the pack was not overwritten */
>                         g_print ("%s\n", _("The pack was not overwritten."));
> @@ -329,9 +365,10 @@ main (int argc, char *argv[])
>         }
>  
>  out:
> +       g_print("\n");
>         /* get rid of temp directory */
>         g_rmdir (tempdir);
> -
> +       

Don't add whitespace, thanks.

>         if (pack != NULL)
>                 g_object_unref (pack);
>         if (client != NULL)
> @@ -342,6 +379,7 @@ out:
>         g_free (filename);
>         g_free (package_id);
>         g_free (directory);
> +       g_free (extracted_packname);
>         g_free (package_list);
>         g_free (options_help);
>         g_object_unref (control);

Please can you fix up the little things, and then we can test and commit
this. Thanks!

Richard.