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

> + 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?

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

> + 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?

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

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

> +#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?

> + 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.