setup
Corinna Vinschen
corinna-cygwin@cygwin.com
Thu Jul 2 09:10:00 GMT 2015
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_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
> + "localdir", current_ini_sig_name.c_str(), owner);
The existing code is inconsistently formatted, but for new code it would
be nice if we could try to be more consistent. Always prepend a space
to a left parenthesis, please.
> + // did we find a compressed setup?
> + if (ini_file &&
> + !(casecompare (current_ini_ext, "xz") &&
> + casecompare (current_ini_ext, "bz2")))
> + ini_file = decompress_ini(ini_file);
(*) And this could simply use the information from ini_suffixes if
"current_ini_ext" wouldn't be the actual extension, but rather a pointer
into ini_suffixes. Kind of like
if (ini_file && current_ini_ext->needs_decompressing)
ini_file = decompress_ini (ini_file);
Sorry if that's a lot. It just occured to me while reading your code.
I'm not adamant about the structural change I outlined above, but to
me it seems better to do it that way. What do you think?
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-apps/attachments/20150702/e2810376/attachment.sig>
More information about the Cygwin-apps
mailing list