Skip to content

Conversation

mseri
Copy link
Contributor

@mseri mseri commented Apr 18, 2018

This will not solve all the warnings, but it moves slowly in the direction that we need to make xapi safe-string compliant and warning free

@coveralls
Copy link

coveralls commented Apr 18, 2018

Coverage Status

Coverage increased (+0.05%) to 20.581% when pulling 41fe0f4 on mseri:kill-warning into e95b96e on xapi-project:master.

@mseri mseri requested a review from gaborigloi April 18, 2018 19:03
@minli1
Copy link
Member

minli1 commented Apr 19, 2018

Very nice to see much fewer warnings when compiling 👍

String.iteri (fun i _ ->
if str.[i]=c1 then Bytes.set buf i c2 else ()
) str;
Bytes.unsafe_to_string buf
Copy link
Contributor

@gaborigloi gaborigloi Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could replace this with Astring.String.map or something similar - performance probably doesn't matter in the autogenerator:
http://erratique.ch/software/astring/doc/Astring.String.html


let write_string (fd: Unix.file_descr) buf =
Unixext.really_write fd buf 0 (String.length buf)
Unix.write fd buf 0 (String.length buf) |> ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return the number of bytes actually written. write repeats the writing operation until all bytes have been written or an error occurs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that the really_write function comes from https://ocaml.github.io/ocamlunix/ocamlunix.html (Section 5.7). Has anything changed in Unix.write such that really_write isn't useful anymore?

Copy link
Contributor

@gaborigloi gaborigloi Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, we discussed with @mseri a while ago that Unix.write already does what we need, it writes everything according to the docs, unlike the write Linux command. So the it was assumed that Unix.write behaves like write(2), and maybe that's why really_write was created?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See that link I posted. Unix.write does repeated write() system calls, but pipes and sockets are a bit special:

The function write of the Unix module iterates the system call write until all the requested bytes are effectively written.
val write : file_descr -> string -> int -> int -> int
However, when the descriptor is a pipe (or a socket, see chapter 6), writes may block and the system call write may be interrupted by a signal. In this case the OCaml call to Unix.write is interrupted and the error EINTR is raised. The problem is that some of the data may already have been written by a previous system call to write but the actual size that was transferred is unknown and lost. This renders the function write of the Unix module useless in the presence of signals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced, however, that Unixext.really_write does this the right way... 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think it's fine to replace really_write with Unix.write though, because their behaviour is the same IMO :) - they both behave incorrectly in case of EINTR

Copy link
Member

@robhoes robhoes Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As on that website:

let rec restart_on_EINTR f x =
  try f x with Unix_error (EINTR, _, _) -> restart_on_EINTR f x

let rec really_write fd buffer offset len =
  let n = restart_on_EINTR (single_write fd buffer offset) len in
  if n < len then really_write fd buffer (offset + n) (len - n)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if our Unixext.really_write was incorrect, than why don't we just fix it rather than replacing all its users with Unix.write?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've done a really_write -> Unix.write replacement in a past PR already, so we should undo that too, not sure which one, maybe @mseri remembers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robhoes thanks for the rationale behind really_write! When I did deprecate it in stdext nobody could tell me why we had that broken implementation, I'm happy to fix it as you suggest and revert the deprecation and changes.

I think we can figure out where we did the change and revert it once we fix the really_write, it's likely one of the handful PR on moving to safe-strings, and in any case it was equivalent to what it had done for the last 10 years so it should not be a problem if we miss one.

Let's not merge this yet, I'll revert the Unixext changes and then send a PR to fix Stdext

@minishrink
Copy link
Contributor

Is this good to merge?

@gaborigloi
Copy link
Contributor

Yes, IMO. But the more reviews, the better.

Copy link
Contributor

@minishrink minishrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much cleaner from what I can comment on, just one suggestion.

and b = (x >>> 8) &&& 0xffl
and c = (x >>> 16) &&& 0xffl
and d = (x >>> 24) &&& 0xffl in
let result = String.make 4 '\000' in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're ANDing them all with the same value, why not include that in the definition of >>>?

Copy link
Contributor

@gaborigloi gaborigloi Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would find that confusing, because >> is usually the right-shift operator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a different operator of course

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think it's better to keep the old code and stick to operators that look like the usual ones

Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't replace the uses of Unixext.really_write with Unix.write. It seems that they currently are actually equivalent, but I believe that this was a mistake, and we should instead fix really_write (see line comments).

@mseri
Copy link
Contributor Author

mseri commented Apr 25, 2018

Yes, I agree. Thanks for digging out the rationale behind the old function

@mseri
Copy link
Contributor Author

mseri commented Apr 25, 2018

Unix.really_write changes reverted. I can squash the reverts once this PR is approved

Marcello Seri added 12 commits April 25, 2018 13:42
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
@mseri mseri merged commit c048b71 into xapi-project:master Apr 25, 2018
@mseri mseri deleted the kill-warning branch April 25, 2018 12:42
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.

6 participants