From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8216CC433EF for ; Tue, 14 Sep 2021 11:35:28 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B9DA160C40 for ; Tue, 14 Sep 2021 11:35:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B9DA160C40 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:53530 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mQ6ik-0001jk-MU for qemu-devel@archiver.kernel.org; Tue, 14 Sep 2021 07:35:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:39246) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mQ6hi-00014f-31 for qemu-devel@nongnu.org; Tue, 14 Sep 2021 07:34:23 -0400 Received: from mail-wr1-x435.google.com ([2a00:1450:4864:20::435]:33358) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mQ6hf-0002Jl-ED for qemu-devel@nongnu.org; Tue, 14 Sep 2021 07:34:21 -0400 Received: by mail-wr1-x435.google.com with SMTP id t18so19696507wrb.0 for ; Tue, 14 Sep 2021 04:34:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=H4GPX1iBOmWdxzU5LD1yxEAm65GacjBeoCzREMjKXlQ=; b=EGxxKoKEOHl/GCO6vq/QZM2O2mngkqDtpFJUS2ynJMiQ33tQiVInBz3P9c/JzZLAY9 lZkEs87aKbkuka84XrSLNVLT9/15OKaB0d1tFUBJBbH5UFAD6lxIQX9TMg8cjOLswiZw erLlytj+NTyxP+VLhI/EZ9gY3kbki0jP6yuABIWQQ30NtkAiJ1JsxgeKJ7rTWAmf5G/A xk3CJ1KvfC2gPhq9oqrBT0gEy1u7zGMMhlXLlZXsOU40p164cVCGBw7hOobQo1uprDIN oZuslinG4heCTMs3EKgBKrX6ryz+yVYHTen/UYk3lSUZ+4sn2YgcmTC1tJainvD+YyEW Z42g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=H4GPX1iBOmWdxzU5LD1yxEAm65GacjBeoCzREMjKXlQ=; b=Bku5LjUfPhqk3i857iPe+9qNTPVYVtTECeW8O4OFJGrNkPJsQgXVfWj8X7IyDe6U4G PosFBvb1Hecpd7He6RFklbJmkzOUUaXumkt+sdDimfYwOtSQCIXwijIZY8vzZMMaZ5Zj 02aTVhjOd91ljZthGVdfvUWMR/w3+5jjq+wT+Z7iKyNFnDr3OlFMrDePmcyv4CJRwf7g y1IUDGZgLUcy8wMqqEFN4ycpzl9g1DpDXIbmrYwHrL1muCxFvCXQs3VdMQgp2KhEdJb3 22XB/Buw5AygzdWxCuO+1IGu1YfKksByEFHxItEl+W1PqMjaCCCZErKNGvKoK98zXvQS HWxg== X-Gm-Message-State: AOAM531A6F11ZW0oWKBbHLmtBDSEj1dqWw/5z8UIM6i8h/KCq1zI4G+i EfKDTUC+8uEkPE2snyxic9hbWiFV74JdBMN/MVE= X-Google-Smtp-Source: ABdhPJwvTal53sonks3Fl6eR22uKi6jMchEA95IiBEBxhcXIk/o2RVgiI8YisaaAEUNJCxyplojBQqyohtNopRl14EQ= X-Received: by 2002:adf:e806:: with SMTP id o6mr17813645wrm.239.1631619258013; Tue, 14 Sep 2021 04:34:18 -0700 (PDT) MIME-Version: 1.0 References: <20210907121943.3498701-1-marcandre.lureau@redhat.com> <20210907121943.3498701-13-marcandre.lureau@redhat.com> In-Reply-To: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Tue, 14 Sep 2021 15:34:06 +0400 Message-ID: Subject: Re: [RFC v3 12/32] rust: provide a common crate for QEMU To: Paolo Bonzini Content-Type: multipart/alternative; boundary="000000000000485b2505cbf2f6fb" Received-SPF: pass client-ip=2a00:1450:4864:20::435; envelope-from=marcandre.lureau@gmail.com; helo=mail-wr1-x435.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Daniel P. Berrange" , QEMU , Stefan Hajnoczi , Markus Armbruster Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000485b2505cbf2f6fb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Paolo On Mon, Sep 13, 2021 at 9:18 PM Paolo Bonzini wrote: > On 07/09/21 14:19, marcandre.lureau@redhat.com wrote: > > From: Marc-Andr=C3=A9 Lureau > > > > This crates provides common bindings and facilities for QEMU C API > > shared by various projects. > > > > Most importantly, it defines the conversion traits used to convert from > > C to Rust types. Those traits are largely adapted from glib-rs, since > > those have proved to be very flexible, and should guide us to bind > > further QEMU types such as QOM. If glib-rs becomes a dependency, we > > should consider adopting glib translate traits. For QAPI, we need a > > smaller subset. > > > > Cargo.lock is checked-in, as QEMU produces end-of-chain binaries, and i= t > > is desirable to track the exact set of packages that are involved in > > managed builds. > > > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > As in my previous review, the main issue I have here is with the > complexity of this code. > > I understand that this has to be manually written, but right now I find > it really hard to understand what is going on here. The patch needs to > be expanded in several parts: > > 1) generic traits (including implementations for Option/Box) > > 2) implementation of the generic traits > > 3) io/nix errors > > and these parts should be moved around to the place where they become > necessary. > > The common crate can be split in many pieces. It is easier to have it as a single commit during PoC/RFC (the series is already large enough). Is it really valuable to introduce a new crate/library piece by piece? Instead, better documentation and tests are more valuable imho. But I will certainly split it and clean it more as we iterate. > Also regarding the code itself: > > 1) Stash should not be a tuple. Accesses to it should use standard Rust > methods, such as borrow()/borrow_mut(), and it should also support > standard Rust idioms such as map(): > > I thought we already discussed that. The stash is an implementation detail to handle FFI resource release. The only thing the user should care about the stash is the pointer, which is shortly retrieved with ".0" (cannot be more succinct). Ideally, we would like to provide "foo.to_qemu_none()", but to keep the associated resource allocated, the shortest we can offer is "foo.to_qemu_none().0". Using "as_ptr()" instead would not only be longer to type but possibly misleading: there is nothing else you should do with the stash, no other method should exist. I don't understand what "map()" would provide here either. The storage part of the stash is an internal detail (the FFI data), not meant to be manipulated at all (it would invalidate the stash pointer). > pub struct BorrowedMutPointer<'a, P, T: 'a> { > native: *mut P, > storage: T, > _marker: PhantomData<&'a P>, > } > > #[allow(dead_code)] > impl<'a, P: Copy, T: 'a> BorrowedMutPointer<'a, P, T> { > fn as_ptr(&self) -> *const P { > self.native > } > > fn as_mut_ptr(&mut self) -> *mut P { > self.native > } > > fn map U>(self, f: F) -> > BorrowedMutPointer<'a, P, U> { > BorrowedMutPointer { > native: self.native, > storage: f(self.storage), > _marker: PhantomData, > } > } > } > > impl<'a, P, T> Borrow for BorrowedMutPointer<'a, P, T> { > fn borrow(&self) -> &T { > &self.storage > } > } > > impl<'a, P, T> BorrowMut for BorrowedMutPointer<'a, P, T> { > fn borrow_mut(&mut self) -> &mut T { > &mut self.storage > } > } > > 2) Does ToQemuPtr need to allow multiple implementations? Can the type > parameter in ToQemuPtr<'a, P> be an associated type (for example > "Native")? Type parameters really add a lot of complexity. > > ToQemuPtr is a trait, implemented for the various Rust types that we can translate to FFI pointers. For example: impl<'a> ToQemuPtr<'a, *mut c_char> for String { type Storage =3D CString; ... } The pointer type parameter is to return the correct pointer. It is not necessary to specify it as a user, since Rust infers it from the expected type. > 3) I would rather not have "qemu" in the names. The Rust parts *are* > QEMU. So "foreign" or "c" would be better. > That's kind of cosmetic. The main reason for "qemu" in the name is to avoid potential clashes with other methods and traits. Also "to_none" could associate in the reader's mind (wrongly) with the ubiquitous Option::None. But yes, we could drop "qemu" here. > > 4) full/none is still really confusing to me. I have finally understood > that it's because the pair that borrows is from_qemu_full/to_qemu_none, > and the pair that copies is from_qemu_none/to_qemu_full. I'd really > like to use names following the Rust naming conventions. A possible > improvement of my proposal from the previous review: > > "full" and "none" are inherited from the glib-introspection ownership transfer annotations. We discussed and weighed pros and cons last year. I haven't yet found or agreed on better options. And you can be certain that the glib-rs folks have already pondered a lot about the naming and design. And, I don't like to reinvent what others have done (creating various limitations and incompatibilities). Following the glib traits brings a lot of experience and could help us reuse glib-rs crates more easily (because they would use the same design), but also to port and interoperate. Imagine we need to bind GHashTable someday, we will be in a better position if we have translation traits that follow glib-rs already. The Rust conventions can be inferred from CString/CStr design and methods, but they have to be adjusted, because they don't solve the same constraints (no ownership transfer, and CString/CStr intermediary representations). They are "CStr::from_ptr(*const)" and "CString::as_ptr(&self) -> *const". - from_qemu_full -> from_foreign (or from_c, same below) > + possibly a dual method into_native or into_rust > > There is no standard naming for such FFI pointer ownership transfer. Rust doesn't know how to release FFI resources, in general. from_raw(*mut) is the closest, but "raw" is reserved for Rust data pointer types, not FFI (except for slices, probably because the linear layout is the same as FFI) We can consider "from_mut_ptr(*mut)", but this doesn't convey the ownership transfer as explicitly. > - from_qemu_none -> cloned_from_foreign > You are being creative! This is like "CStr::from_ptr", except we go directly to higher Rust types. > - to_qemu_none -> as_foreign or as_foreign_mut > > Similarity with "CString::as_ptr", except that it handles the temporary stash (CString is the internal storage for String), so we use the "to_qemu_none().0" described earler. If we follow your considerations it could be "to_stash().as_ptr()", which is less user friendly, exposes more internal details, etc. > - to_qemu_full -> clone_to_foreign > There is no equivalent in Rust standard. An implementation may want to store in an existing location, or allocated in different ways etc. Closest would be to_ptr() In summary: from_qemu_full() vs from_mut_ptr() from_qemu_none() from_ptr() to_qemu_full() to_ptr() to_qemu_none() to_stash().as_ptr() I argue there is more consistency and readability by following the glib-rs traits and method names (among other advantages by staying close to glib-rs design for the future). > > I will see if I have some time to do some of this work. > > thanks --=20 Marc-Andr=C3=A9 Lureau --000000000000485b2505cbf2f6fb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Paolo

On Mon, Sep 13, 2021 at 9:18 = PM Paolo Bonzini <pbonzini@redhat= .com> wrote:
On 07/09/21 14:19, marcandre.lureau@redhat.com wrote:
> From: Marc-Andr=C3=A9 Lureau<marcandre.lureau@redhat.com>
>
> This crates provides common bindings and facilities for QEMU C API
> shared by various projects.
>
> Most importantly, it defines the conversion traits used to convert fro= m
> C to Rust types. Those traits are largely adapted from glib-rs, since<= br> > those have proved to be very flexible, and should guide us to bind
> further QEMU types such as QOM. If glib-rs becomes a dependency, we > should consider adopting glib translate traits. For QAPI, we need a > smaller subset.
>
> Cargo.lock is checked-in, as QEMU produces end-of-chain binaries, and = it
> is desirable to track the exact set of packages that are involved in > managed builds.
>
> Signed-off-by: Marc-Andr=C3=A9 Lureau<marcandre.lureau@redhat.com>

As in my previous review, the main issue I have here is with the
complexity of this code.

I understand that this has to be manually written, but right now I find it really hard to understand what is going on here.=C2=A0 The patch needs t= o
be expanded in several parts:

1) generic traits (including implementations for Option/Box)

2) implementation of the generic traits

3) io/nix errors

and these parts should be moved around to the place where they become
necessary.


The common crate can be split in many = pieces. It is easier to have it as a single commit during PoC/RFC (the seri= es is already large enough).

Is it really val= uable to introduce a new crate/library piece by piece? Instead, better docu= mentation and tests are more valuable imho. But I will certainly split it a= nd clean it more as we iterate.


Also regarding the code itself:

1) Stash should not be a tuple.=C2=A0 Accesses to it should use standard Ru= st
methods, such as borrow()/borrow_mut(), and it should also support
standard Rust idioms such as map():



I thought we already discussed tha= t. The stash is an implementation detail to handle FFI resource release. Th= e only thing the user should care about the stash is the pointer, which is = shortly retrieved with ".0" (cannot be more succinct).

Ide= ally, we would like to provide "foo.to_qemu_none()", but to keep = the associated resource allocated, the shortest we can offer is "foo.t= o_qemu_none().0". Using "as_ptr()" instead would not only be= longer to type but possibly misleading: there is nothing else you should d= o with the stash, no other method should exist.

I don't understa= nd what "map()" would provide here either. The storage part of th= e stash is an internal detail (the FFI data), not meant to be manipulated a= t all (it would invalidate the stash pointer).

=C2=A0
pub struct BorrowedMutPointer<'a, P, T: 'a> {
=C2=A0 =C2=A0 =C2=A0native: *mut P,
=C2=A0 =C2=A0 =C2=A0storage: T,
=C2=A0 =C2=A0 =C2=A0_marker: PhantomData<&'a P>,
}

#[allow(dead_code)]
impl<'a, P: Copy, T: 'a> BorrowedMutPointer<'a, P, T&g= t; {
=C2=A0 =C2=A0 =C2=A0fn as_ptr(&self) -> *const P {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self.native
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0fn as_mut_ptr(&mut self) -> *mut P {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self.native
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0fn map<U: 'a, F: FnOnce(T) -> U>(self, f: = F) ->
BorrowedMutPointer<'a, P, U> {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BorrowedMutPointer {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0native: self.native,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0storage: f(self.storage), =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0_marker: PhantomData,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0}
}

impl<'a, P, T> Borrow<T> for BorrowedMutPointer<'a, = P, T> {
=C2=A0 =C2=A0 =C2=A0fn borrow(&self) -> &T {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&self.storage
=C2=A0 =C2=A0 =C2=A0}
}

impl<'a, P, T> BorrowMut<T> for BorrowedMutPointer<'= a, P, T> {
=C2=A0 =C2=A0 =C2=A0fn borrow_mut(&mut self) -> &mut T {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&mut self.storage
=C2=A0 =C2=A0 =C2=A0}
}

2) Does ToQemuPtr need to allow multiple implementations?=C2=A0 Can the typ= e
parameter in ToQemuPtr<'a, P> be an associated type (for example =
"Native")?=C2=A0 Type parameters really add a lot of complexity.<= br>


ToQemuPtr is a trait, implemented = for the various Rust types that we can translate to FFI pointers.

Fo= r example:

impl<'a> ToQemuPtr<'a, *mut c_char> f= or String {
=C2=A0 =C2=A0 type Storage =3D CString;
...
}

T= he pointer type parameter is to return the correct pointer. It is not neces= sary to specify it as a user, since Rust infers it from the expected type.<= br>=C2=A0
3) I would rather not have "qemu" in the names.=C2=A0 The Rust pa= rts *are*
QEMU.=C2=A0 So "foreign" or "c" would be better.


That's kind of cosmetic.

The = main reason for "qemu" in the name is to avoid potential clashes = with other methods and traits.

Also "to_none" could assoc= iate in the reader's mind (wrongly) with the ubiquitous Option::None.
But yes, we could drop "qemu" here.
=C2=A0

4) full/none is still really confusing to me.=C2=A0 I have finally understo= od
that it's because the pair that borrows is from_qemu_full/to_qemu_none,=
and the pair that copies is from_qemu_none/to_qemu_full.=C2=A0 I'd real= ly
like to use names following the Rust naming conventions.=C2=A0 A possible <= br> improvement of my proposal from the previous review:



"full" and "none&qu= ot; are inherited from the glib-introspection ownership transfer annotation= s.

We discussed and weighed pros and cons last year. I haven't y= et found or agreed on better options. And you can be certain that the glib-= rs folks have already pondered a lot about the naming and design. And, I do= n't like to reinvent what others have done (creating various limitation= s and incompatibilities). Following the glib traits brings a lot of experie= nce and could help us reuse glib-rs crates more easily (because they would = use the same design), but also to port and interoperate. Imagine we need to= bind GHashTable<char *, QapiFoo> someday, we will be in a better pos= ition if we have translation traits that follow glib-rs already.

The= Rust conventions can be inferred from CString/CStr design and methods, but= they have to be adjusted, because they don't solve the same constraint= s (no ownership transfer, and CString/CStr intermediary representations).
They are "CStr::from_ptr(*const)" and "CString::as_ptr= (&self) -> *const".


- from_qemu_full -> from_foreign (or from_c, same below)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0+ possibly a dual method into_native or into_rust


There is no standard naming for such FFI pointer = ownership transfer. Rust doesn't know how to release FFI resources, in = general.

from_raw(*mut) is the closest, but "raw" is reser= ved for Rust data pointer types, not FFI (except for slices, probably becau= se the linear layout is the same as FFI)

We can consider "from_= mut_ptr(*mut)", but this doesn't convey the ownership transfer as = explicitly.
=C2=A0

- to_qemu_none -> as_foreign or as_foreign_mut


Similarity with "CString::as_ptr&= quot;, except that it handles the temporary stash (CString is the internal = storage for String), so we use the "to_qemu_none().0" described e= arler.

If we follow your considerations it could be "to_stash()= .as_ptr()", which is less user friendly, exposes more internal details= , etc.
=C2=A0
- to_qemu_full -> clone_to_foreign

<= br>There is no equivalent in Rust standard. An implementation may want to s= tore in an existing location, or allocated in different ways etc.

Cl= osest would be to_ptr()

In summary:

from_qemu_full() =C2=A0vs= =C2=A0from_mut_ptr()
from_qemu_none() =C2=A0 =C2=A0 =C2=A0from_ptr()to_qemu_full() =C2=A0 =C2=A0 =C2=A0 =C2=A0to_ptr()
to_qemu_none() =C2= =A0 =C2=A0 =C2=A0 =C2=A0to_stash().as_ptr()

I argue there is more co= nsistency and readability by following the glib-rs traits and method names = (among other advantages by staying close to glib-rs design for the future).=
=C2=A0

I will see if I have some time to do some of this work.

=C2=A0
thanks


--
=
Marc-Andr=C3=A9 Lureau
= --000000000000485b2505cbf2f6fb--