On 2014-05-14 10:49, Jef Driesen wrote:

On 2014-05-13 13:37, Jef Driesen wrote:

...

On 2014-05-13 11:39, Venkatesh Shukla wrote:

...

I have attached a patch to a minute mistake that I noticed in the code.

• if (memcmp (&tty, &active, sizeof (struct termios) != 0)) {

• if (memcmp (&tty, &active, sizeof (struct termios)) !=0) {

Nice catch! I'll apply the patch.

Due to this bug, the memcmp only checked the first byte. I wonder whether fixing this will have any negative side effect, with this discussion in mind:

http://libdivecomputer.org/pipermail/devel/2014-March/000268.html

Anyway, we'll find out soon enough.

Great, I already discovered a major problem. After applying this patch, the memcmp always fails on my system. Not good.

The problem seems to be that the termios struct contains some padding bytes on my system. Because the content of padding bytes is unspecified, they may contain some random data, which causes the memcmp to fail. Comparing the individual termios members, which would be the portable way to compare structs, isn't an option here, because the contents of the termios struct is platform dependent.

Explicitly initializing the termios structure with memset, will also set the padding bytes to zero. This seems to work fine on my system. But the C standard doesn't make any guarantee regarding padding bytes. Thus in theory the compiler is free to change those padding values at will. I doubt this will be a problem in practice, but just in case, please test the attached patch.

The alternative solution is removing the memcmp check completely.

Has anyone tried the patch? On my system it still fails for all backends that require odd or even parity (e.g. vyper and iconhd). I assume this is only a pty limitation (see Linus comments in the link above), but since I don't have one of those devices at my disposal, I can't try on real hardware.

So most likely, either this memcmp will have to be removed completely, or at least conditionally compiled on the ENABLE_PTY macro.

Jef

On 2014-05-28 01:05, Linus Torvalds wrote:

On Tue, May 27, 2014 at 6:16 AM, Jef Driesen jef@libdivecomputer.org wrote:

...

So most likely, either this memcmp will have to be removed completely, or at least conditionally compiled on the ENABLE_PTY macro.

I'd suggest just removing it entirely. The check just doesn't make sense, even ignoring the whole padding issue (and no, initializing it to zero doesn't fix it, since there might be random private members outside the spec) that might get initialized.

I also see no reason to keep the check. It only seem to cause more problems than it solves. So let's just remove it.

BTW, Those private members are actually the reason why I used memcmp for the comparison in the first place. There is basically no other way to compare a struct that may contain "unknown" members. But I don't think those private members would be problematic in this particular case. We don't setup the termios structure from scratch, but initialize it with a call to tcgetattr. If there are private members present, they would get initialized there. But this no longer matters, once the check is gone :-)

Jef