Skip to content
New issue

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

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

Already on GitHub? # 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?

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

Successfully merging this pull request may close these issues.

1 participant