-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add writev syscall #75
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution, it looks good but please see some comments on minor details of the PR
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @matth-)
Makefile, line 37 at r1 (raw file):
EXAMPLE_PROGRAMS += $(EXAMPLE_DST)/mkdir EXAMPLE_PROGRAMS += $(EXAMPLE_DST)/rmdir EXAMPLE_PROGRAMS += $(EXAMPLE_DST)/iovec
Normally we name test programs after syscalls their usage they show. So I think a better name would be writev
example-programs/iovec.c, line 18 at r1 (raw file):
iov[1].iov_len = strlen(str1); nwritten = writev(1, iov, 2);
This looks to be taken from the writev
man page - it would be good to add this as a comment in the head of the file. But my question here is about 1
on this line - why is it not STDOUT_FILENO
as in the man page?
And also it's a good practice to have a proper error handling, in this case we need to check if nwritten
gets a value of -1
src/System/Hatrace.hs, line 1821 at r1 (raw file):
{ fd :: CInt , count :: CSize , iovs :: [IovecStruct]
this appears to be a peeked value, in syscalls details we keep the original value (a pointer in this case) and add a peeked value as a convenience, see e.g. SyscallExitDetails_stat
(but in this case we peek a value on exit). Here it looks like we have peeked bytes in bufContents
but also intermediate iovs
(as it's not a pointer)
src/System/Hatrace.hs, line 1828 at r1 (raw file):
instance SyscallEnterFormatting SyscallEnterDetails_writev where syscallEnterToFormatted SyscallEnterDetails_writev{ fd, bufContents, count } = FormattedSyscall "writev" [formatArg fd, argPlaceholder "*iovec", formatArg bufContents, formatArg count]
we have a value of iovec
- it doesn't get filled by the kernel so we could show the value (or the pointer)
src/System/Hatrace.hs, line 1840 at r1 (raw file):
syscallExitToFormatted SyscallExitDetails_writev{ enterDetail, writtenCount, iovsData } = ( FormattedSyscall "writev" [formatArg fd, formatArg writtenCount, formatArg iovsData] , NoReturn
writtenCount
is the return of this syscall
src/System/Hatrace.hs, line 2077 at r1 (raw file):
} Syscall_writev -> do let SyscallArgs{ arg0 = fd, arg1 = iovBasePtr, arg2 = iovcnt } = syscallArgs
the name iovBasePtr
is a bit confusing as we have void* iov_base
as a field in struct iovec
src/System/Hatrace.hs, line 2922 at r1 (raw file):
peekStructArray :: Storable a => TracedProcess -> Int -> Int -> Ptr a -> IO [a] peekStructArray pid size count ptr
this looks to be almost a copy of peekArray
below and it looks to me that we could use that function instead.
src/System/Hatrace/Format.hs, line 120 at r1 (raw file):
data FormattedArg = IntegerArg Integer -- using Integer to accept both Int64 and Word64 at the same time | HexadecimalArg Integer
This looks to be almost an arbitrary addition, I suppose you wanted to show pointers as hexadecimals and that makes perfect sense - I'd propose to make that as a separate PR
src/System/Hatrace/Types.hsc, line 1737 at r1 (raw file):
iovecStructSize :: Int iovecStructSize = #{size struct iovec}
we have it as a part of instance Storable IovecStruct
no need to have another definition.
src/System/Hatrace/Types.hsc, line 1740 at r1 (raw file):
data IovecStruct = IovecStruct { iov_base :: CUIntPtr -- ^ Starting address
lets keep definitions close to C ones, this field should be a Ptr Void
test/HatraceSpec.hs, line 73 at r1 (raw file):
callProcess "make" ["--quiet", "example-programs-build/atomic-write"] makeIovecExample :: IO ()
makeIovecExample
is used only in 1 test - no need to have a separate definition of it, it's better to inline it, it's just 1 line of code
test/HatraceSpec.hs, line 124 at r1 (raw file):
x `elem` [ExitFailure 11, ExitFailure (128+11)] describe "iovec" $ do
it's writev
, not iovec
test/HatraceSpec.hs, line 125 at r1 (raw file):
describe "iovec" $ do it "iovec" $ do
could you write a proper comment instead of "iovec"
?
test/HatraceSpec.hs, line 144 at r1 (raw file):
) <- events ] let (count:vect1:vect2:_) = [ bufContents |
This looks to be relying on printf
resulting in write
getting called and also on stdout using line-buffering. It's worth to have a comment about this. Otherwise it looks like 2 comprehensions filter out the same events (writev
looks quite like write
:) )
test/HatraceSpec.hs, line 155 at r1 (raw file):
fromIntegral (iov_base iov1) `shouldBe` (read (B8.unpack vect1) :: Int) fromIntegral (iov_base iov2) `shouldBe` (read (B8.unpack vect2) :: Int) exitCode `shouldBe` ExitSuccess
I'd propose to move this check closer to the line where exitCode
appears
src/System/Hatrace.hs, line 2922 at r1 (raw file): Previously, qrilka (Kirill Zaborsky) wrote…
can't use peekArray because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @matth-)
src/System/Hatrace.hs, line 2922 at r1 (raw file):
Previously, matth- wrote…
can't use peekArray because
elemSize = sizeOf ptr
always returns a sizeof Ptr not sure if it was intended for the this function.
i hesitate to change peekArray to use the sizeof(a) (like does the tricks in https://hackage.haskell.org/package/linux-ptrace-0.1.2/docs/src/System-Linux-Ptrace.html#peek https://hackage.haskell.org/package/linux-ptrace-0.1.2/docs/src/System-Linux-Ptrace.html#peek), wdyt ?
Actually @matth- it looks like you've found a bug :)
Thanks for that.
So the proper way here is to fix it.
We haven't seen it before as for PollFdStruct
both the struct itself and a pointer to it take the same amount of bytes, namely 8 bytes (pointer size is equal to 1 int
+ 2 short
s).
600d3e6
to
79e940d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 6 files reviewed, 15 unresolved discussions (waiting on @qrilka)
Makefile, line 37 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
Normally we name test programs after syscalls their usage they show. So I think a better name would be
writev
Done.
src/System/Hatrace.hs, line 1821 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
this appears to be a peeked value, in syscalls details we keep the original value (a pointer in this case) and add a peeked value as a convenience, see e.g.
SyscallExitDetails_stat
(but in this case we peek a value on exit). Here it looks like we have peeked bytes inbufContents
but also intermediateiovs
(as it's not a pointer)
Done.
src/System/Hatrace.hs, line 1828 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
we have a value of
iovec
- it doesn't get filled by the kernel so we could show the value (or the pointer)
Done.
src/System/Hatrace.hs, line 1840 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
writtenCount
is the return of this syscall
Done.
src/System/Hatrace.hs, line 2077 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
the name
iovBasePtr
is a bit confusing as we havevoid* iov_base
as a field instruct iovec
Done.
src/System/Hatrace.hs, line 2922 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
Actually @matth- it looks like you've found a bug :)
Thanks for that.
So the proper way here is to fix it.
We haven't seen it before as forPollFdStruct
both the struct itself and a pointer to it take the same amount of bytes, namely 8 bytes (pointer size is equal to 1int
+ 2short
s).
Done.
src/System/Hatrace/Format.hs, line 120 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
This looks to be almost an arbitrary addition, I suppose you wanted to show pointers as hexadecimals and that makes perfect sense - I'd propose to make that as a separate PR
Done.
src/System/Hatrace/Types.hsc, line 1737 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
we have it as a part of
instance Storable IovecStruct
no need to have another definition.
Done.
src/System/Hatrace/Types.hsc, line 1740 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
lets keep definitions close to C ones, this field should be a
Ptr Void
Done.
test/HatraceSpec.hs, line 73 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
makeIovecExample
is used only in 1 test - no need to have a separate definition of it, it's better to inline it, it's just 1 line of code
Done.
test/HatraceSpec.hs, line 124 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
it's
writev
, notiovec
Done.
test/HatraceSpec.hs, line 125 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
could you write a proper comment instead of
"iovec"
?
Done.
test/HatraceSpec.hs, line 144 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
This looks to be relying on
printf
resulting inwrite
getting called and also on stdout using line-buffering. It's worth to have a comment about this. Otherwise it looks like 2 comprehensions filter out the same events (writev
looks quite likewrite
:) )
Done.
test/HatraceSpec.hs, line 155 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
I'd propose to move this check closer to the line where
exitCode
appears
Done.
example-programs/iovec.c, line 18 at r1 (raw file):
Previously, qrilka (Kirill Zaborsky) wrote…
This looks to be taken from the
writev
man page - it would be good to add this as a comment in the head of the file. But my question here is about1
on this line - why is it notSTDOUT_FILENO
as in the man page?
And also it's a good practice to have a proper error handling, in this case we need to check ifnwritten
gets a value of-1
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matth-)
src/System/Hatrace.hs, line 1821 at r1 (raw file):
Previously, matth- wrote…
Done.
What I was proposing is something like
, iovs :: Ptr IovecStruct
i.e. have iovec
as a "raw" argument mimicking (as far as possible) the C definition and thus it shouldn't be in the "peeked" section
src/System/Hatrace.hs, line 1828 at r1 (raw file):
Previously, matth- wrote…
Done.
but you didn't remove the placeholder and this will output 5 arguments for writev
when it has only 3
src/System/Hatrace/Types.hsc, line 1741 at r1 (raw file):
data IovecStruct = IovecStruct { iov_base :: CUIntPtr -- ^ Starting address , iov_len :: CULong -- ^ Number of bytes to transfer
I think CSize
should be slightly closer to C here
@matth- see my last couple of comments, thanks |
src/System/Hatrace.hs, line 1828 at r1 (raw file): Previously, qrilka (Kirill Zaborsky) wrote…
my bad, didn't catch placeholder was for syscall return value 😅 , i'm removing it .
or maybe |
src/System/Hatrace.hs, line 1821 at r1 (raw file): Previously, qrilka (Kirill Zaborsky) wrote…
not sure to fully get it
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matth-)
src/System/Hatrace.hs, line 1821 at r1 (raw file):
Previously, matth- wrote…
not sure to fully get it
you mean :, iovs : Ptr IovecStruct (or ultimately Ptr [IovecStruc] to fit the function signature) -- Peeked detail , iovsContent :: [IovecStruct] , bufContents :: [ByteString] ``` ? i still need to peek the iovs content to be able to display it </blockquote></details> what I mean is
, iovs : Ptr IovecStruct
-- Peeked detail
, bufContents :: [ByteString]___ *[src/System/Hatrace.hs, line 1828 at r1](https://reviewable.io/reviews/nh2/hatrace/75#-M9lqlix2lIpy5XeVtzC:-M9tGETa5ZYWq60bKPJR:b-tinxi6) ([raw file](https://github.com/nh2/hatrace/blob/6da04094dcf5dc6bd6422c244780ad7a1b7b5870/src/System/Hatrace.hs#L1828)):* <details><summary><i>Previously, matth- wrote…</i></summary><blockquote> my bad, didn't catch placeholder was for syscall return value :sweat_smile: , i'm removing it . still need to find a way to merge formatters to display both iovec struct and content into something like this : ```writev(1, [{iov_base=4714500, iov_len=6}, {iov_base=4714507, iov_len=6}] ["hello ", "world\n"], 2)``` or maybe ```writev(1, [{iov_base=4714500, iov_len=6} "hello", {iov_base=4714507, iov_len=6} "world\n"], 2)``` </blockquote></details> Outputting multiple argument representations isn't supported yet, feel free to open a ticket about it (and mention that we could need "inner" pointers). For now I think `writev(1, ["hello ", "world\n"], 2)` should be enough. Let's use the simple version and possible improvements n later tickets/PRs. <!-- Sent from Reviewable.io -->
@matth- have you seen my last 2 comments in Reviewable? Also recent merges brought some conflicts. Will you have tome to resolve this or maybe you need my help finishing this PR? |
Hi @qrilka . Sorry don't have much time currently, the last comments lead to remove some code and writev point is to deal with pointer indirection, so i would rather implement formatting to handle structure with nested pointers in this pr or in another one as a dependency to this one |
Thanks @matth- |
writev syscall with details of iov struct array
This change is