[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH 10/12] hivexsh: Add 'setval' and 'commit' commands.



On Thu, Feb 04, 2010 at 05:34:36PM +0000, Matthew Booth wrote:
> > +  int prompt = isatty (0) ? 2 : 0;
> 
> You've already got this in a global. In fact, I can't see it used
> anywhere. Doesn't this generate a warning?

Yup, this is a bug -- removed -- thanks.

> > +    buf = rl_gets ("  key> ");
> > +    buf = rl_gets ("value> ");
> 
> Why no indentation on this prompt, as for key?

So they line up ^^^

> > +    if (!buf) {
> > +      fprintf (stderr, _("hivexsh: setval: unexpected end of input\n"));
> > +      quit = 1;
> > +      goto error;
> > +    }
> > +
> > +    if (STREQ (buf, "none")) {
> > +      values[i].t = hive_t_none;
> > +      values[i].len = 0;
> > +    }
> > +    else if (STRPREFIX (buf, "string:")) {
> > +      buf += 7;
> > +      values[i].t = hive_t_string;
> > +      int nr_chars = strlen (buf);
> > +      values[i].len = 2 * (nr_chars + 1);
> > +      values[i].value = malloc (values[i].len);
> > +      if (!values[i].value) {
> > +        perror ("malloc");
> > +        exit (EXIT_FAILURE);
> > +      }
> > +      for (j = 0; j <= /* sic */ nr_chars; ++j) {
> > +        if (buf[j] & 0x80) {
> > +          fprintf (stderr, _("hivexsh: string(utf16le): only 7 bit ASCII strings are supported for input\n"));
> > +          goto error;
> > +        }
> > +        values[i].value[2*j] = buf[j];
> > +        values[i].value[2*j+1] = '\0';
> 
> There must be a library function to do the above. Where does the 7 bit
> ASCII restriction come from?

There's iconv, but that's even crazier than doing it by hand.  This is
fine for 7 bit ASCII, but would break if you pass in UTF-8 (hence the
check that no high bits are set).

> This doesn't look like regedit's expandstring format. What's the purpose
> of it?
>
> > +    else if (STRPREFIX (buf, "expandstring:")) {

Not sure what you mean -- expandstring is a separate type in the
hive.  We don't care about what regedit may or may not do.

> > +      xerr = xstrtol (buf, NULL, 0, &n, "");
> 
> Does xstrtol support 0x notation?

Yes.

> > +#if 0
> > +      if (n < 0 || n > UINT64_MAX) {
> > +        fprintf (stderr, _("%s: %s: integer out of range\n"),
> > +                 "setval", "dword");
> > +        goto error;
> > +      }
> > +#endif
> 
> Why have you commented this out?

You can't test a 64 bit int against UINT64_MAX.  In any case it's just
documentation of what the limits are.

> > +        for (j = 0; *buf && j < 2; buf++) {
> > +          if (c_isxdigit (*buf)) { /* NB: ignore non-hex digits. */
> 
> The documentation defines the limiter to be a comma. I'd stick to this
> strictly, making it more likely to catch typos.

The documentation just says that non-hex digits are ignored, and I
can't be bothered to code more complex parsing here.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]