On Thu, Nov 18, 2010 at 2:29 AM, Peter Stuge <pe...@stuge.se> wrote: > Andrew Leech wrote:
>>
>> I'll look into that, I've not used snprintf so don't really know the
>> difference. It's obviously only a google away. I'm just used to
>> sprintf from embedded devices when you often don't have a choice of
>> using a better alternative (depending on compiler).
>
> The point of snprintf() is that it will never write beyond the given
> buffer size, yet always terminate the string with a \0. The return
> value can be used to check if the buffer was large enough. This works
> together to avoid possible buffer overrun conditions, which are quite
> frequently exploited to inject code via data, and break security of
> the program or product.
>
Thanks, that's good to know, and makes a lot of sense.
>> Note newer patch attached. This is made directly against current
>> git, use instead of original patch.
>
> (A note that you can send patches easily by first committing and then
> using the git send-email command.)
>
Ok I'm still pretty new to git, so by committing it just adds changes
to the local repository not the mainline one? So far I'm just doing a
git diff > blah.patch
> Good stuff! I just have one very minor request for the help text:
>
> usage: svf [[-]help] [-tap device.tap] <file> [[-]quiet]
>
> If [-]help is common in other commands then it is good to have here,
> but otherwise I think it might be redundant. If I just enter svf
> (without further options) then I will get the same output, right?
yep
>
> Also, I'd like quiet to still be at the end, to signal a little more
> that this is still backwards compatible.
>
> Are you sure about the dash in -quiet?
>
> I would feel best about this:
>
> usage: svf [-tap device.tap] <file> [quiet]
>
> Because quiet can be treated specially, and it is even possible to
> reliably allow a -tap filename by careful evaluation of the
> arguments. I would be happy to make this change if you're OK with
> that.
>
I'm in no way certain about quiet having a dash, it just seems more
consistent to me to have all switches with a dash, more like standard
*nix programs. On the other hand it's also important to keep
consistency within openocd to try to make it easier to use. I'm also
not too concerned whether -help stays in there, as as mentioned yes
usage gets printed if you have an incorrect command line anyway. I'm
not sure if help as a command is common in any other modules. Either
way I'm more than happy for you or anyone to modify the command line
layout / behavior to keep it relevant.
For now I've changed it to usage: svf [-tap device.tap] <file>
[quiet], I do think this looks cleaner.
>
>> >> reuse .. existing handling of head/tail
> ..
>> I dug deep enough to figure out how it works, it uses array's that it
>> dynamically sets the size of, so it's a little convoluted to set them.
>
> I'd like to suggest a function to handle the setting, see below.
>
>
>> It's debatable whether this newer code is neater to understand, but
>> it is certainly far more efficient to run, and avoids all the extra
>> string handling.
>
> I like the new approach very much!
>
>
>> Other than that it seems to run fine, I've tested the command line in
>> pretty much every direction and it works well. The operation of
>> header/trailer padding certainly seems to be completely compatible
>> with my first effort, I don't think I've missed anything of left out
>> any important steps.
>
> Lovely.
>
>
>> +static int svf_adjust_array_length(uint8_t **arr, int orig_bit_len, int
>> new_bit_len, int set);
>
> I would suggest adding a function with a new name that takes the
> extra parameter, and renaming the existing function, and adding the
> parameter to it. Then add a one-line function with the original name
> that calls the new function with the last parameter set to 0. This
> way, each previous call site for the existing function does not need
> to be touched by this patch. The new call sites invoke the renamed
> function and specify values as needed for the extra parameter.
>
Once again this was the way I was initially implementing it and it
gave some minor issues and I then thought it seemed wasteful to add
another function just to modify the functionality of an existing
function. But yeah, going and modifying all calls to an existing
function is pretty messy too.
>
>> + else if ((svf_fd = open(CMD_ARGV[i], O_RDONLY)) < 0)
>> {
>> - LOG_ERROR("unknown variant for svf: %s", CMD_ARGV[i]);
>> -
>> + command_print(CMD_CTX, USAGE);
>> + command_print(CMD_CTX, "file \"%s\" not found",
>> CMD_ARGV[i]);
>
> I know this was already in here before, but open() can fail for many
> reasons. It would be nice to not assume that the file is missing, and
> e.g. call strerror() to get the specific error message:
>
> else if ((svf_fd = open(CMD_ARGV[i], O_RDONLY)) < 0)
> {
> int err = errno;
> LOG..
> print usage ..
> command_print(CMD_CTX, "open(\"%s\"): %s", CMD_ARGV[i], strerror(err));
>
> Also, I'm surprised that open() is used instead of fopen(), but that
> doesn't matter as long as this works. Are you testing on a Windows or
> Linux system?
>
Yeah that error reporting makes a lot of sense, added. Now if you
specify a folder rather than a file you get "permission denied", much
better.
I'm testing on Windows 7 (cygwin) x64 (x32 openocd binaries) and
haven't had any issues with the file open. I've got a linux vm I could
have a got at compiling it on, probably should give it a test.
>
>> + // HDR %d TDI (0)
>> + error |= svf_adjust_array_length(&svf_para.hdr_para.tdi,
>> svf_para.hdr_para.len, header_dr_len, 0);
>> + error |= svf_adjust_array_length(&svf_para.hdr_para.tdo,
>> svf_para.hdr_para.len, header_dr_len, 0);
>> + error |= svf_adjust_array_length(&svf_para.hdr_para.mask,
>> svf_para.hdr_para.len, header_dr_len, 0);
>> + svf_para.hdr_para.len = header_dr_len;
>> + svf_para.hdr_para.data_mask = XXR_TDI;
>
> Watch out for trailing whitespace, in the last line quoted above.
>
Ah now I see what you mean about the trailing whitespace, I never
recognized this before. I think I've fixed it up, haven't noticed any
in the new patch.
> These blocks of code are somewhat repetitive, maybe they could be
> refactored into a nice function:
>
> unsigned int set_xxr_para(struct svf_xxr_para *p, unsigned int len, unsigned
> char tdi) {
> int ret = 0;
> ret |= svf_adjust_array_length(&p->tdi, p->len, len, tdi);
> ret |= svf_adjust_array_length(&p->tdo, p->len, len, 0);
> ret |= svf_adjust_array_length(&p->mask, p->len, len, 0);
> p->len = len;
> p->data_mask = XXR_TDI;
> return ret;
> }
>
> The blocks would then become:
>
> error |= set_xxr_para(&svf_para.hdr_para, header_dr_len, 0);
> error |= set_xxr_para(&svf_para.hir_para, header_ir_len, 0xff);
> error |= set_xxr_para(&svf_para.tdr_para, trailer_dr_len, 0);
> error |= set_xxr_para(&svf_para.tir_para, trailer_ir_len, 0xff);
>
I also agree with this suggestion, and have rolled them into a neat
function that absorbs the "modifying an existing function" without
looking at all wasteful and I'm pretty happy with it.
Much easier to read and more target error messages now.
>
>> -static int svf_adjust_array_length(uint8_t **arr, int orig_bit_len, int
>> new_bit_len)
>> +static int svf_adjust_array_length(uint8_t **arr, int orig_bit_len, int
>> new_bit_len, int set)
>
> Right, please do change the name of the function as mentioned above.
> I think it's fairly important that all existing call sites do not
> have to change, when they are not really relevant for this change.
>
Yep reverted,
>
>> case HDR:
>> + if (svf_tap_is_specified)
>> + break;
>> xxr_para_tmp = &svf_para.hdr_para;
>> goto XXR_common;
>
> Maybe add a message when breaking here, so that the user is informed
> why the commands in the file are being ignored? I'm not sure.
>
>
I quite like this suggestion too, added. It now prints (skipped) after
header/trailer commands when not in quiet mode, as such:
STATE
RUNTEST
ENDIR
SIR
ENDIR
HDR (skipped)
HIR (skipped)
TDR (skipped)
TIR (skipped)
3000 kHz
FREQUENCY
STATE
RUNTEST
ENDIR
ENDDR
Thanks again for the suggestions, it's helping me be a much neater programmer!
Andrew
svf_chain_3.patch
Description: Binary data
_______________________________________________ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
