Created attachment 252680 [details] [review]
admin: Write out correct version fields in boot/loader/entries files

Hi Colin,

It looks good to me. I've just one small comment below:

>From 73c8d5e8700982cb7146f453b1acb9e7af919f1c Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@verbum.org>
>Date: Wed, 21 Aug 2013 18:48:00 -0400
>Subject: [PATCH] admin: Write out correct version fields in boot/loader/entries files
>
>Before, we were writing the "bootversion", which is either 0 or 1, for
>all entries.  This is completely wrong; the idea of the "version"
>field is to compare between entries.
>
>Fix this by writing out the inverted index - internally, index 0 is
>the *first* boot entry, so we give it the highest version number, and
>index N is the last, so give it version 0.
>
>Then fix the deployment sorting code to correctly reverse the version
>number comparison, so we read back the right order.
>
>In practice before this bug didn't matter because "normally" you only
>have at most two deployments.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=706546
>---
> src/ostree/ot-admin-deploy.c    |   11 +++++++++--
> src/ostree/ot-admin-functions.c |   18 ++++++++++++------
> tests/test-admin-deploy-1.sh    |   10 ++++++++--
> 3 files changed, 29 insertions(+), 10 deletions(-)
>
>diff --git a/src/ostree/ot-admin-deploy.c b/src/ostree/ot-admin-deploy.c
>index 21db483..deda4eb 100644
>--- a/src/ostree/ot-admin-deploy.c
>+++ b/src/ostree/ot-admin-deploy.c
>@@ -791,10 +791,16 @@ parse_os_release (const char *contents,
>   return ret;
> }
> 
>+/*
>+ * install_deployment_kernel:
>+ * 
>+ * Write out an entry in /boot/loader/entries for @deployment.
>+ */
> static gboolean
> install_deployment_kernel (GFile          *sysroot,
>                            int             new_bootversion,
>                            OtDeployment   *deployment,
>+                           guint           n_deployments,
>                            GCancellable   *cancellable,
>                            GError        **error)
> 
>@@ -901,7 +907,7 @@ install_deployment_kernel (GFile          *sysroot,
>                                val);
>   ot_config_parser_set (bootconfig, "title", title_key);
> 
>-  version_key = g_strdup_printf ("%d", ot_deployment_get_bootserial (deployment));
>+  version_key = g_strdup_printf ("%d", n_deployments - ot_deployment_get_index (deployment));
>   ot_config_parser_set (bootconfig, "version", version_key);
> 
>   linux_relpath = g_file_get_relative_path (bootdir, dest_kernel_path);
>@@ -1000,7 +1006,8 @@ ot_admin_write_deployments (GFile             *sysroot,
>       for (i = 0; i < new_deployments->len; i++)
>         {
>           OtDeployment *deployment = new_deployments->pdata[i];
>-          if (!install_deployment_kernel (sysroot, new_bootversion, deployment,
>+          if (!install_deployment_kernel (sysroot, new_bootversion,
>+                                          deployment, new_deployments->len,
>                                           cancellable, error))
>             {
>               g_prefix_error (error, "Installing kernel: ");
>diff --git a/src/ostree/ot-admin-functions.c b/src/ostree/ot-admin-functions.c
>index 7dfc30f..e8662af 100644
>--- a/src/ostree/ot-admin-functions.c
>+++ b/src/ostree/ot-admin-functions.c
>@@ -717,8 +717,8 @@ list_deployments_process_one_boot_entry (GFile               *sysroot,
> }
> 
> static gint
>-compare_deployments_by_boot_loader_version (gconstpointer     a_pp,
>-                                            gconstpointer     b_pp)
>+compare_deployments_by_boot_loader_version_reversed (gconstpointer     a_pp,
>+                                                     gconstpointer     b_pp)
> {
>   OtDeployment *a = *((OtDeployment**)a_pp);
>   OtDeployment *b = *((OtDeployment**)b_pp);
>@@ -728,11 +728,15 @@ compare_deployments_by_boot_loader_version (gconstpointer     a_pp,
>   const char *b_version = ot_config_parser_get (b_bootconfig, "version");
>   
>   if (a_version && b_version)
>-    return strverscmp (a_version, b_version);
>+    {
>+      int r = strverscmp (a_version, b_version);
>+      /* Reverse */
>+      return -r;
>+    }
>   else if (a_version)
>-    return 1;
>-  else
>     return -1;
>+  else
>+    return 1;
> }
> 
> gboolean
>@@ -766,10 +770,12 @@ ot_admin_list_deployments (GFile               *sysroot,
>         goto out;
>     }
> 
>-  g_ptr_array_sort (ret_deployments, compare_deployments_by_boot_loader_version);
>+  g_ptr_array_sort (ret_deployments, compare_deployments_by_boot_loader_version_reversed);
>   for (i = 0; i < ret_deployments->len; i++)
>     {
>       OtDeployment *deployment = ret_deployments->pdata[i];
>+      OtConfigParser *bootconfig = ot_deployment_get_bootconfig (deployment);
>+      const char *version = ot_config_parser_get (bootconfig, "version");

The "version" variable is not used. It is a left over?

>       ot_deployment_set_index (deployment, i);
>     }
> 
>diff --git a/tests/test-admin-deploy-1.sh b/tests/test-admin-deploy-1.sh
>index 0119586..fbf9fc2 100755
>--- a/tests/test-admin-deploy-1.sh
>+++ b/tests/test-admin-deploy-1.sh
>@@ -62,6 +62,8 @@ assert_file_has_content sysroot/boot/loader/entries/ostree-testos-${rev}-0.conf
> assert_file_has_content sysroot/ostree/deploy/testos/deploy/${rev}.1/etc/os-release 'NAME=TestOS'
> assert_file_has_content sysroot/ostree/boot.0/testos/${bootcsum}/0/etc/os-release 'NAME=TestOS'
> 
>+ostree admin --sysroot=sysroot status
>+
> echo "ok second deploy"
> 
> ostree admin --sysroot=sysroot deploy --os=testos testos:testos/buildmaster/x86_64-runtime
>@@ -123,6 +125,8 @@ assert_not_has_file sysroot/ostree/deploy/testos/deploy/${rev}.3/etc/aconfigfile
> 
> echo "ok upgrade bare"
> 
>+ostree admin --sysroot=sysroot status
>+
> os_repository_new_commit
> ostree --repo=sysroot/ostree/repo remote add testos file://$(pwd)/testos-repo testos/buildmaster/x86_64-runtime
> ostree admin --sysroot=sysroot upgrade --os=testos
>@@ -134,14 +138,16 @@ assert_file_has_content sysroot/ostree/deploy/testos/deploy/${newrev}.0/etc/os-r
> 
> echo "ok upgrade"
> 
>+ostree admin --sysroot=sysroot status
>+
> assert_file_has_content sysroot/ostree/deploy/testos/deploy/${newrev}.0/etc/os-release 'NAME=TestOS'
> assert_file_has_content sysroot/ostree/deploy/testos/deploy/${origrev}.4/etc/os-release 'NAME=TestOS'
>-ostree admin --sysroot=sysroot undeploy 1
>+ostree admin --sysroot=sysroot undeploy 2
> assert_file_has_content sysroot/ostree/deploy/testos/deploy/${newrev}.0/etc/os-release 'NAME=TestOS'
> assert_not_has_dir sysroot/ostree/deploy/testos/deploy/${origrev}.4
> 
> assert_file_has_content sysroot/ostree/deploy/testos/deploy/${origrev}.3/etc/os-release 'NAME=TestOS'
>-ostree admin --sysroot=sysroot undeploy 3
>+ostree admin --sysroot=sysroot undeploy 2
> assert_not_has_dir sysroot/ostree/deploy/testos/deploy/${origrev}.3
> 
> assert_file_has_content sysroot/ostree/deploy/testos/deploy/${newrev}.0/etc/os-release 'NAME=TestOS'
>-- 
>1.7.1

(In reply to comment #2)
>
> The "version" variable is not used. It is a left over?

It is indeed a leftover from debugging, thanks for spotting.  Removed and pushed, thanks!