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

Add writev syscall #75

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

Add writev syscall #75

wants to merge 1 commit into from

Conversation

m-matth
Copy link

@m-matth m-matth commented Jun 13, 2020

writev syscall with details of iov struct array


This change is Reviewable

Copy link
Collaborator

@qrilka qrilka left a 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

@m-matth
Copy link
Author

m-matth commented Jun 14, 2020


src/System/Hatrace.hs, line 2922 at r1 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

this looks to be almost a copy of peekArray below and it looks to me that we could use that function instead.

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 ?

Copy link
Collaborator

@qrilka qrilka left a 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 shorts).

@m-matth m-matth force-pushed the writev branch 3 times, most recently from 600d3e6 to 79e940d Compare June 14, 2020 18:25
Copy link
Author

@m-matth m-matth left a 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 in bufContents but also intermediate iovs (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 have void* iov_base as a field in struct 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 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 shorts).

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, not iovec

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 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 :) )

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 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

Done.

Copy link
Collaborator

@qrilka qrilka left a 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

@m-matth m-matth requested a review from qrilka June 14, 2020 19:52
@qrilka
Copy link
Collaborator

qrilka commented Jun 14, 2020

@matth- see my last couple of comments, thanks

@m-matth
Copy link
Author

m-matth commented Jun 15, 2020


src/System/Hatrace.hs, line 1828 at r1 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

but you didn't remove the placeholder and this will output 5 arguments for writev when it has only 3

my bad, didn't catch placeholder was for syscall return value 😅 , 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)

@m-matth
Copy link
Author

m-matth commented Jun 15, 2020


src/System/Hatrace.hs, line 1821 at r1 (raw file):

Previously, qrilka (Kirill Zaborsky) wrote…

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

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

Copy link
Collaborator

@qrilka qrilka left a 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 -->

@qrilka
Copy link
Collaborator

qrilka commented Jun 22, 2020

@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?

@m-matth
Copy link
Author

m-matth commented Jun 25, 2020

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

@qrilka
Copy link
Collaborator

qrilka commented Jun 25, 2020

Thanks @matth-
There is no rush for sure so take your time if you need to. Just let me know if you need some help.

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.

2 participants