Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove need to truncate mach IPC results #162
Conversation
|
Awesome! Just had a couple of nits and this is good to go. |
| @@ -371,7 +371,7 @@ fn embedded_bytes_receivers() { | |||
| let (super_tx, super_rx) = ipc::channel().unwrap(); | |||
| super_tx.send(sub_tx).unwrap(); | |||
| let sub_tx = super_rx.recv().unwrap(); | |||
| let bytes = [1, 2, 3, 4, 5, 6, 7, 8]; | |||
| let bytes = [1, 2, 3, 4, 5, 6, 7]; | |||
This comment has been minimized.
This comment has been minimized.
pcwalton
May 10, 2017
Collaborator
Add a comment linking to this PR saying that the odd number of bytes is intentional.
| @@ -836,123 +877,213 @@ impl From<MachError> for bincode::Error { | |||
| } | |||
| } | |||
|
|
|||
| impl From<kern_return_t> for MachError { | |||
| fn from(code: kern_return_t) -> MachError { | |||
| if code == MACH_MSG_SUCCESS { | |||
This comment has been minimized.
This comment has been minimized.
|
I suggest putting the commit that changes the test case after the actual fix, to improve bisectability. (Might actually want to add this as a separate test case, instead of changing the existing one... Not sure though it's really worth the effort.) Have you considered sending the size in a separate data item? It might be considered more elegant... Though to be honest I'm not quite sure it's really a good idea, since it would actually complicate the code I think? I wonder whether putting the size header before the actual data causes a performance hit on i386, because it breaks alignment... (Unless the alignment is already bad because of other things -- haven't checked that.) To avoid that, it might be preferable to put it after the actual data. BTW, the error handling commit seems like a completely orthogonal thing, that could be committed independently? Personally, I'd submit it as a separate PR... (Not really a problem, though.) |
| ptr = ptr.offset(1); | ||
| } | ||
| data_dest = data_dest.offset(mem::size_of::<usize>() as isize); | ||
| ptr::copy_nonoverlapping(data.as_ptr(), data_dest, data_size); |
This comment has been minimized.
This comment has been minimized.
antrik
May 10, 2017
Contributor
I can't see the message size being adjusted anywhere for the extra space taken up by the size header -- am I missing something?...
(This kind of pointer manipulation is so damn fragile -- it's actually worse than in C :-( Can't really blame you for that though I guess: probably not much point trying to use slices for this particular bit, without rewriting the rest of function in a more defensive way too...)
This comment has been minimized.
This comment has been minimized.
jdm
May 10, 2017
Author
Member
I definitely had code which adjusted the header size. I'm confused by how it disappeared!
This comment has been minimized.
This comment has been minimized.
antrik
May 10, 2017
Contributor
To clarify, I do see code doing the adjustment in the receiver -- but not in the sender...
This comment has been minimized.
This comment has been minimized.
jdm
May 10, 2017
Author
Member
I understood your original question. I remain confused about how code that I wrote and committed in Message::size_of somehow did not make it to github both times I added it. I have verified that it is now present in the commits in this PR.
|
@antrik Github is showing the commits in the wrong order; they are actually ordered in the way you recommend. |
|
I am not going to make a separate PR for the error changes. They were useful for diagnosing the issue here, and it's not worth separating them out. |
|
@bors-servo: r+ |
|
|
|
@jdm the reason I prefer to submit independently useful changes as separate PRs is that in the (hopefully unlikely) case that the main PR doesn't get accepted for some reason, or turns out problematic and needs to be reverted later, it would be good to have the independent changes in nevertheless. But as I said, it's not really a problem... |
|
|
|
@bors-servo: r=pcwalton |
|
|
|
|
|
omg, thanks for fixing this, @jdm! |
jdm commentedMay 10, 2017
•
edited
Mach's IPC requires 4 byte alignment. The code that receives data does not account for that right now; these changes ensure that the returned buffer is the same size as the original buffer that was sent. Fixes #83. Fixes #103.