Hi Achim,
On Jul 1 22:37, Achim Gratz wrote:
> Corinna Vinschen writes:
> > Ok, for once. But please make sure that you split the commit into
> > functional chunks next time it's so large. And send it to this list, so
> > code snippets can be referenced in the review.
>
> I've split it up into three parts that at least compile cleanly.
Thanks!
> +extern IniList found_ini_list, setup_ext_list;
> +const std::string setup_exts[] = { "xz", "bz2", "ini" };
See (*) below.
> @@ -97,6 +99,8 @@ static BoolOption PackageManagerOption (false, 'M',
> "package-manager", "Semi-att
> static BoolOption NoAdminOption (false, 'B', "no-admin", "Do not check for
> and enforce running as Administrator");
> static BoolOption WaitOption (false, 'W', "wait", "When elevating, wait for
> elevated child process");
> static BoolOption HelpOption (false, 'h', "help", "print help");
> +static StringOption SetupBaseNameOpt ("setup", 'i', "ini-basename", "Use a
> different basename instead of setup", false);
The word setup needs quoting of some sort I think. An example wouldn't
hurt either. What about
"Use a different ini file basename instead of \"setup\", e.g. \"foo\".ini"
?
> + (theFile->nFileSizeLow || theFile->nFileSizeHigh))
> + {
> + if (!casecompare (SetupBaseName + ".xz", theFile->cFileName))
> + found_xz = true;
> + if (!casecompare (SetupBaseName + ".bz2", theFile->cFileName))
> + found_bz2 = true;
> + if (!casecompare (SetupBaseName + ".ini", theFile->cFileName))
> + found_ini = true;
> + }
(*) This puzzles me a bit. You're keeping arrays and lists in terms of
the file suffix (setup_ext, setup_ext_list), but you don't use the
information here and elsewhere. The setup_exts array already contains
the suffixes and constitutes an order. If you extend the array of
strings to a struct, you could just run a loop over it in places like
the above and generalize the suffix handling. For instance...
struct ini_suffix_t
{
char *suffix;
bool needs_decompressing;
[...]
}
ini_suffix_t ini_suffixes[] = {
{ "xz", true },
{ "bz2", true },
{ "ini", false },
{ NULL, false } };
bool found_ini[]; // <- just as example
for (i = 0; ini_suffixes[i].suffix; ++i)
if (!casecompare (SetupBaseName + "." + ini_suffixes[i].suffix, ...)
found_ini[i] = true;
This would work OOTB, without C++11x initializer lists, and you could
drop setup_ext_list entirely.
This would also simplify things if there's a new compression we want to
support.
> +if (found_xz)
> + found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" +
> SetupBaseName + ".xz");
> +else if (found_bz2)
> + found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" +
> SetupBaseName + ".bz2");
> +else if (found_ini)
> + found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" +
> SetupBaseName + ".ini");
Same kind of loop here.
for (i = 0; ini_suffixes[i].suffix; ++i)
if (found_ini[i])
found_ini_list.push_back (... + ini_suffixes[i].suffix);
Ideally found_ini_list keeps a pointer into ini_suffixes as well so
you don't have to extract the suffix again at (**).
> Subject: [PATCH 3/5] Refactor setup search and implement XZ compressed setup
> files
>
> * ini.cc: Construct setup_ext_list from array until we can use
> C++11 aggregate initializers.
> (decompress_ini): Refactored for use from do_local_ini and
> do_remote_ini. Change outdated comment about setup.ini
> uncompressed size.
> (check_ini_sig): Factor out signature check.
> (fetch_remote_ini): Refactored for use from do_remote_ini.
> (do_local_ini): Iterate over search results in found_ini_list.
> Use ini_decompress and check_ini_sig.
^^
missing name change
> + // Unless the NoVerifyOption is set, check the signature for the
> + // current setup and record the result. On a failed signature check
> + // the streams are invalidated so even if we tried to read in the
> + // setup anyway there's be nothing to parse.
I'd prefer /* */ for multiline comments, but that's used pretty
inconsistently anyway, so, never mind.
> + IniDBBuilderPackage aBuilder(myFeedback);
> + io_stream *ini_file, *ini_sig_file;
> + // iterate over all setup files found in do_from_local_dir
> + for (IniList::const_iterator n = found_ini_list.begin();
> + n != found_ini_list.end(); ++n)
> +{
> + bool sig_fail = false;
> + std::string current_ini_ext, current_ini_name, current_ini_sig_name;
> +
> + current_ini_name = *n;
> + current_ini_sig_name = current_ini_name + ".sig";
> + current_ini_ext = current_ini_name.substr(current_ini_name.rfind(".")
> + 1);
(**) See above.
> + ini_sig_file = io_stream::open("file://" + current_ini_sig_name, "rb",
> 0);
> + ini_file = io_stream::open("file://" + current_ini_name, "rb", 0);
> + ini_fil
