[[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

Ken Brown-6
This can be empty if no setup.ini files are found.  Removing it causes
setup to hang.
---
 package_db.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package_db.cc b/package_db.cc
index ac9387c..b104073 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -596,7 +596,7 @@ packagedb::removeEmptyCategories()
 {
   for (packagedb::categoriesType::iterator n = packagedb::categories.begin();
        n != packagedb::categories.end(); ++n)
-    if (!n->second.size())
+    if (!n->second.size() && n->first != "Base")
       {
         Log (LOG_BABBLE) << "Removing empty category " << n->first << endLog;
         packagedb::categories.erase (n++);
--
2.14.2

Reply | Threaded
Open this post in threaded view
|

[[PATCH setup topic/libsolv] 2/2] Avoid clobbering installed.db when no setup.ini is found

Ken Brown-6
Move the calls to packagedb::read and other packagedb functions from
do_ini_thread to ChooserPage::OnInit.  If no setup.ini is found,
do_ini_thread is never called.  But we need to ensure that
packagedb::read is called, or else installed.db gets emptied.
---
 choose.cc | 5 +++++
 ini.cc    | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/choose.cc b/choose.cc
index 619d7db..013a30a 100644
--- a/choose.cc
+++ b/choose.cc
@@ -268,6 +268,11 @@ ChooserPage::OnInit ()
     packagemeta::ScanDownloadedFiles (MirrorOption);
 
   packagedb db;
+  db.makeBase();
+  db.read();
+  db.upgrade();
+  db.fixup_source_package_ids();
+  db.removeEmptyCategories();
   db.setExistence ();
   db.fillMissingCategory ();
 
diff --git a/ini.cc b/ini.cc
index 5089e8b..0f8b927 100644
--- a/ini.cc
+++ b/ini.cc
@@ -352,13 +352,6 @@ do_ini_thread (HINSTANCE h, HWND owner)
   else
     ini_count = do_remote_ini (owner);
 
-  packagedb db;
-  db.makeBase();
-  db.read();
-  db.upgrade();
-  db.fixup_source_package_ids();
-  db.removeEmptyCategories();
-
   if (ini_count == 0)
     return false;
 
--
2.14.2

Reply | Threaded
Open this post in threaded view
|

Re: [[PATCH setup topic/libsolv] 2/2] Avoid clobbering installed.db when no setup.ini is found

Ken Brown-6
On 10/28/2017 8:29 AM, Ken Brown wrote:

> Move the calls to packagedb::read and other packagedb functions from
> do_ini_thread to ChooserPage::OnInit.  If no setup.ini is found,
> do_ini_thread is never called.  But we need to ensure that
> packagedb::read is called, or else installed.db gets emptied.
> ---
>   choose.cc | 5 +++++
>   ini.cc    | 7 -------
>   2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/choose.cc b/choose.cc
> index 619d7db..013a30a 100644
> --- a/choose.cc
> +++ b/choose.cc
> @@ -268,6 +268,11 @@ ChooserPage::OnInit ()
>       packagemeta::ScanDownloadedFiles (MirrorOption);
>  
>     packagedb db;
> +  db.makeBase();
> +  db.read();
> +  db.upgrade();
> +  db.fixup_source_package_ids();
> +  db.removeEmptyCategories();
>     db.setExistence ();
>     db.fillMissingCategory ();

Sorry, this isn't quite right.  The new calls need happen before
ScanDownLoadedFiles is called, to avoid a crash when the latter looks
for the installation tarballs.

I'll send a revised patch shortly.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

Jon TURNEY
In reply to this post by Ken Brown-6
On 28/10/2017 13:29, Ken Brown wrote:

> This can be empty if no setup.ini files are found.  Removing it causes
> setup to hang.
> ---
>   package_db.cc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/package_db.cc b/package_db.cc
> index ac9387c..b104073 100644
> --- a/package_db.cc
> +++ b/package_db.cc
> @@ -596,7 +596,7 @@ packagedb::removeEmptyCategories()
>   {
>     for (packagedb::categoriesType::iterator n = packagedb::categories.begin();
>          n != packagedb::categories.end(); ++n)
> -    if (!n->second.size())
> +    if (!n->second.size() && n->first != "Base")
>         {
>           Log (LOG_BABBLE) << "Removing empty category " << n->first << endLog;
>           packagedb::categories.erase (n++);
>
Hmm... now I remember my other concerns about this piece of code: as
written, it's just wrong.

1. Applying erase to packagedb:categories invalidates the iterator
2. We're incrementing the iterator after doing an erase, so even if the
iterator was still valid, we skip checking if the following category is
empty

So maybe the right way to fix this is as attached:

I need to stare as this a bit more to understand where the 'base'
category is coming from when we have no setup.ini...

0001-Fix-invalid-iterator-use-in-packagedb-removeEmptyCat.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

Ken Brown-6
On 10/29/2017 1:24 PM, Jon Turney wrote:

> On 28/10/2017 13:29, Ken Brown wrote:
>> This can be empty if no setup.ini files are found.  Removing it causes
>> setup to hang.
>> ---
>>   package_db.cc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/package_db.cc b/package_db.cc
>> index ac9387c..b104073 100644
>> --- a/package_db.cc
>> +++ b/package_db.cc
>> @@ -596,7 +596,7 @@ packagedb::removeEmptyCategories()
>>   {
>>     for (packagedb::categoriesType::iterator n =
>> packagedb::categories.begin();
>>          n != packagedb::categories.end(); ++n)
>> -    if (!n->second.size())
>> +    if (!n->second.size() && n->first != "Base")
>>         {
>>           Log (LOG_BABBLE) << "Removing empty category " << n->first
>> << endLog;
>>           packagedb::categories.erase (n++);
>>
>
> Hmm... now I remember my other concerns about this piece of code: as
> written, it's just wrong.
>
> 1. Applying erase to packagedb:categories invalidates the iterator
> 2. We're incrementing the iterator after doing an erase, so even if the
> iterator was still valid, we skip checking if the following category is
> empty
>
> So maybe the right way to fix this is as attached:

Yes.  I wrongly jumped to the conclusion that removing Base was the issue.

> I need to stare as this a bit more to understand where the 'base'
> category is coming from when we have no setup.ini...

I guess it's created implicitly in the 'for' statement in
packagedb::makeBase().  That's probably a mistake.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

Jon TURNEY
On 29/10/2017 21:18, Ken Brown wrote:

> On 10/29/2017 1:24 PM, Jon Turney wrote:
>> Hmm... now I remember my other concerns about this piece of code: as
>> written, it's just wrong.
>>
>> 1. Applying erase to packagedb:categories invalidates the iterator
>> 2. We're incrementing the iterator after doing an erase, so even if
>> the iterator was still valid, we skip checking if the following
>> category is empty
>>
>> So maybe the right way to fix this is as attached:
>
> Yes.  I wrongly jumped to the conclusion that removing Base was the issue.
>
>> I need to stare as this a bit more to understand where the 'base'
>> category is coming from when we have no setup.ini...
>
> I guess it's created implicitly in the 'for' statement in
> packagedb::makeBase().  That's probably a mistake.

Ah yes, I guess this could check if the 'base' category exists first.

This is all a bit broken though.  Because we've forgotten the categories
of installed packages, if we're run without a setup.ini, we'll merrily
let packages in the base category get uninstalled without complaint...
Reply | Threaded
Open this post in threaded view
|

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

Ken Brown-6
On 10/30/2017 11:59 AM, Jon Turney wrote:
> This is all a bit broken though.  Because we've forgotten the categories
> of installed packages, if we're run without a setup.ini, we'll merrily
> let packages in the base category get uninstalled without complaint...

Maybe we need to add something to the existing warning.  It currently
says, "You can still use setup-<arch>.exe to remove installed packages,
but there will be nothing to install.  Press OK if that's what you
wanted or Cancel to choose a different directory."  We could add that
they risk uninstalling required packages if they proceed.

Of course, the same thing applies even if one or more setup.ini files
are found but don't include the base packages or the dependencies of
installed packages.  In this case, however, it means that the user has a
homemade setup.ini, so we probably have to assume that they know what
they're doing.

Ken