GNOME Bugzilla – Bug 710970
Crash when deploying in a booted ostree
Last modified: 2013-10-28 13:37:48 UTC
In a booted ostree you can ostree admin deploy something without specifying an os explicitly. That causes a crash though. ot_admin_complete_deploy_one is called with NULL osname and it tries to strcmp it. Patch coming up.
Created attachment 258243 [details] [review] Fix crash when deploying with implicit os name When booted into an ostree you can deploy without passing an --os option. That was crashing though, because ot_admin_complete_deploy_one is called with NULL osname but it was not handling it properly.
Review of attachment 258243 [details] [review]: This patch looks correct and fixes a real bug, but if you look at ot-builtin-admin-upgrade.c, it does: if (!opt_osname) opt_osname = (char*)ostree_deployment_get_osname (ostree_sysroot_get_booted_deployment (sysroot)); The function ostree_sysroot_get_merge_deployment() also does this. So probably the best fix for consistency is to treat osname == NULL inside libostree as "booted osname".
Created attachment 258247 [details] [review] main: Treat default osname more consistently The libostree already treats passing NULL for osname as "booted osname, if any". We should do the same inside the tools. The upgrade builtin had this logic duplicated there; we should be able to safely remove it.
Ok, looking at this more, your patch is definitely right. What do you think about this patch in addition to yours?
Review of attachment 258247 [details] [review]: ::: src/ostree/ot-admin-builtin-upgrade.c @@ -72,3 @@ goto out; - if (!opt_osname) - opt_osname = (char*)ostree_deployment_get_osname (ostree_sysroot_get_booted_deployment (sysroot)); Make sense to me. ::: src/ostree/ot-admin-functions.c @@ +68,3 @@ booted_deployment = ostree_sysroot_get_booted_deployment (sysroot); + if (osname == NULL && booted_deployment) I'm not sure this is strictly necessary because in the if we are also checking "ostree_deployment_equal (deployment, booted_deployment)". Though perhaps it's good to do it anyway, in case someone uses osname somewhere else in the function in the future.
(In reply to comment #5) > I'm not sure this is strictly necessary because in the if we are also checking > "ostree_deployment_equal (deployment, booted_deployment)". We also retain all deployments not for the current OS, which is a pretty important property when parallel installing. Though honestly a lot of this logic is just too hardcoded; I've been thinking about having a config file that says e.g. "Retain 20 installs or up to 10GB". But that's for later... Thanks for the patch and review of my followup!
Attachment 258243 [details] pushed as 7ecfbff - Fix crash when deploying with implicit os name Attachment 258247 [details] pushed as affccb3 - main: Treat default osname more consistently