Re: setup

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

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

Attachment: pgpb1rRbzKA3H.pgp
Description: PGP signature