Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TOOLS-2077 port modern mdata tools to Windows #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joyent-automation
Copy link

TOOLS-2077 port modern mdata tools to Windows

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/5019/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@mgerdts commented at 2018-12-04T18:01:53

Patch Set 3:

(1 comment)

@marsell commented at 2018-12-04T18:52:37

Patch Set 3:

Sadly it's not. I'll recheck, but towards the beginning I also tried an older byte-at-a-time version. :(

@mgerdts commented at 2018-12-04T19:38:29

Patch Set 3: Code-Review+1

@marsell commented at 2018-12-06T05:13:28

Patch Set 3:

Confirmed the 1-byte version has the same 3K issue.

@pfmooney commented at 2018-12-11T15:17:26

Patch Set 3:

(3 comments)

Patch Set 3 code comments
README.md#41 @pfmooney

Include instructions (and/or bits in the makefile) for the proper cygwin resources needed?

plat/cygwin.c#32 @pfmooney

Should be 'size_t', not 'int'

plat/cygwin.c#65 @mgerdts

All of the other platforms read 1 character at a time. I understand the motivation for reading more, but perhaps this is involved in 3K bug.

plat/cygwin.c#142 @pfmooney

All of the other platform drivers seem to store the file descriptor in its own field, and later configure it in the event poller (epoll/kqueue/etc). Perhaps follow their lead?

@pfmooney commented at 2019-01-08T14:21:14

Patch Set 4:

(2 comments)

@mgerdts commented at 2019-01-08T22:24:36

Patch Set 4:

(1 comment)

Patch Set 4 code comments
README.md#44 @pfmooney

style nit: This should probably be a sub header (with license bumped up to the top level)

README.md#49 @pfmooney

In order to use the binaries on a windows machine, are there cygwin libraries that need to be copied with them?

plat/cygwin.c#2 @mgerdts

2019

plat/cygwin.c#66 @jclulow

It looks like this code only deals with a newline character that comes at the very end of a read(). I don't think there's any structural guarantee that this will be true. If you're going to read more than one character at a time, I think you'll need to inspect the entire string for newlines and you'll need to accumulate anything that comes after a newline in some kind of buffer for later.

If I recall correctly, that's why the other platforms read one character at a time -- a subsequent line can remain in the device buffer until we're ready for it.

plat/cygwin.c#67 @jclulow

Please put spaces around mathematical operators; i.e.,

buf[sz - 1]

There are a few other instances of this in this file.

plat/cygwin.c#68 @jclulow

This is meant to be Sun C style, so please don't use C++ style comments. I'd move this up above the line and flesh it out as a full sentence; e.g.,

/*
 * Remove the trailing newline:
 */
@jclulow commented at 2019-01-13T04:48:03

Patch Set 4:

(3 comments)

Some cstyle nits, but also a question about the way the line splitting is working in the serial port read loop.

As an aside: does cygwin not provide epoll emulation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant