Add writev syscall

[m-matth]

writev syscall with details of iov struct array

---

This change is 

[qrilka]

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]

---

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 ?

[qrilka]

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]

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.

[qrilka]

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

[qrilka]

@matth- see my last couple of comments, thanks

[m-matth]

---

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]

---

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

[qrilka]

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]

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

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]

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.