On Thu, Apr 05, 2018 at 05:16:27PM +0100, Christian Lindig wrote:
> I think this is a good patch as it reduces the amount of copying. I believe
> it is safe as it is. There is one place where I am a little hesitant:
> @@ -291,7 +291,9 @@ let access_logging ~con ~tid ?(data="") ~level
> access_type =
> let date = string_of_date() in
> let tid = string_of_tid ~con tid in
> let access_type = string_of_access_type
> access_type in
> - let data = sanitize_data (Bytes.of_string data)
> + (* we can use unsafe_of_string here as the
> sanitize_data function
> + immediately makes a copy of the data and
> operates on that. *)
> + let data = sanitize_data
> (Bytes.unsafe_of_string data) in
> let prefix = prefix !access_log_destination
> date in
> let msg = Printf.sprintf "%s %s %s %s" prefix
> tid access_type data in
> logger.write ~level msg)
> This relies on the implementation of sanitize_data() and somebody could
> change it in the future
> and invalidate the assumption being made here. However, this is the only
> call site and the
> function is defined above. Anybody making changes in the context of
> String/Byte conversion
> come across the comment here.
> So: I'm happy to take the patch as it is.
> On 05/04/18 11:40, Marcello Seri wrote:
> > When xenstore was ported to the new safe-string interface, it mostly
> > happened by making copyies of string into bytes and back. The ideal
> > fix would be to rewrite all of the relevant interfaces to be uniformly
> > using bytes, but in the meanwhile we can improve the code by using unsafe
> > conversion functions (see
> > https://caml.inria.fr/pub/docs/manual-ocaml/libref/Bytes.html#3_Unsafeconversionsforadvancedusers).
> > In most cases we own the bytes that we are converting to string, or we
> > immediately make copies that we then mutate, or we use them immutably
> > as payloads for writes. In all these cases it is safe to use the unsafe
> > functions and prevent a copy.
> > This patch updates the code to use the unsafe conversions where possible.
> > Signed-off-by: Marcello Seri <marcello.s...@citrix.com>
> Reviewed-by: Christian Lindig <christian.lin...@citrix.com>
Applied with Juergen's release-ack.
Xen-devel mailing list