This allows header files to declare pointers without pulling
in the entire crypto subsystem.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/crypto/cipher.h | 2 --
 include/qemu/typedefs.h | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/crypto/cipher.h b/include/crypto/cipher.h
index 5928e5ecc7..95a0412911 100644
--- a/include/crypto/cipher.h
+++ b/include/crypto/cipher.h
@@ -23,8 +23,6 @@
 
 #include "qapi/qapi-types-crypto.h"
 
-typedef struct QCryptoCipher QCryptoCipher;
-
 /* See also "QCryptoCipherAlgorithm" and "QCryptoCipherMode"
  * enums defined in qapi/crypto.json */
 
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 427027a970..45b9c57717 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -95,6 +95,7 @@ typedef struct PostcopyDiscardState PostcopyDiscardState;
 typedef struct Property Property;
 typedef struct PropertyInfo PropertyInfo;
 typedef struct QBool QBool;
+typedef struct QCryptoCipher QCryptoCipher;
 typedef struct QDict QDict;
 typedef struct QEMUBH QEMUBH;
 typedef struct QemuConsole QemuConsole;
-- 
2.25.1

On 8/13/20 5:25 AM, Richard Henderson wrote:
> This allows header files to declare pointers without pulling
> in the entire crypto subsystem.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/crypto/cipher.h | 2 --
>  include/qemu/typedefs.h | 1 +
>  2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On Wed, Aug 12, 2020 at 08:25:21PM -0700, Richard Henderson wrote:
> This allows header files to declare pointers without pulling
> in the entire crypto subsystem.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/crypto/cipher.h | 2 --
>  include/qemu/typedefs.h | 1 +
>  2 files changed, 1 insertion(+), 2 deletions(-)

I'm not in favour of this change or the next. Using #include "cipher.h"
is not a burden on the users of the crypto code. Moving typedefs away
from the associated struct is a step backwards IMHO.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 8/17/20 9:48 AM, Daniel P. Berrangé wrote:
> On Wed, Aug 12, 2020 at 08:25:21PM -0700, Richard Henderson wrote:
>> This allows header files to declare pointers without pulling
>> in the entire crypto subsystem.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  include/crypto/cipher.h | 2 --
>>  include/qemu/typedefs.h | 1 +
>>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> I'm not in favour of this change or the next. Using #include "cipher.h"
> is not a burden on the users of the crypto code. Moving typedefs away
> from the associated struct is a step backwards IMHO.

Consider if you put a pointer to QCryptoCipher in a relatively generic header
file (e.g. "target/foo/cpu.h"), restricting "cipher.h" to a portion of the
implementation (e.g. target/foo/helper_crypto.c).

This sort of thing is exactly why "qemu/typedefs.h" exists.


r~

On 8/17/20 1:38 PM, Richard Henderson wrote:
> On 8/17/20 9:48 AM, Daniel P. Berrangé wrote:
>> On Wed, Aug 12, 2020 at 08:25:21PM -0700, Richard Henderson wrote:
>>> This allows header files to declare pointers without pulling
>>> in the entire crypto subsystem.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  include/crypto/cipher.h | 2 --
>>>  include/qemu/typedefs.h | 1 +
>>>  2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> I'm not in favour of this change or the next. Using #include "cipher.h"
>> is not a burden on the users of the crypto code. Moving typedefs away
>> from the associated struct is a step backwards IMHO.
> 
> Consider if you put a pointer to QCryptoCipher in a relatively generic header
> file (e.g. "target/foo/cpu.h"), restricting "cipher.h" to a portion of the
> implementation (e.g. target/foo/helper_crypto.c).
> 
> This sort of thing is exactly why "qemu/typedefs.h" exists.

As for the next patch for QCryptoCipherDriver, I could easily see not moving
the typedef to typedefs.h, but instaed to "crypto.h", where we do in fact want
to declare an incomplete structure.  I think it's a real mistake to be using
void* there at present.


r~

On 8/17/20 1:42 PM, Richard Henderson wrote:
> On 8/17/20 1:38 PM, Richard Henderson wrote:
>> On 8/17/20 9:48 AM, Daniel P. Berrangé wrote:
>>> On Wed, Aug 12, 2020 at 08:25:21PM -0700, Richard Henderson wrote:
>>>> This allows header files to declare pointers without pulling
>>>> in the entire crypto subsystem.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>>  include/crypto/cipher.h | 2 --
>>>>  include/qemu/typedefs.h | 1 +
>>>>  2 files changed, 1 insertion(+), 2 deletions(-)
>>>
>>> I'm not in favour of this change or the next. Using #include "cipher.h"
>>> is not a burden on the users of the crypto code. Moving typedefs away
>>> from the associated struct is a step backwards IMHO.
>>
>> Consider if you put a pointer to QCryptoCipher in a relatively generic header
>> file (e.g. "target/foo/cpu.h"), restricting "cipher.h" to a portion of the
>> implementation (e.g. target/foo/helper_crypto.c).
>>
>> This sort of thing is exactly why "qemu/typedefs.h" exists.
> 
> As for the next patch for QCryptoCipherDriver, I could easily see not moving
> the typedef to typedefs.h, but instaed to "crypto.h", where we do in fact want
> to declare an incomplete structure.  I think it's a real mistake to be using
> void* there at present.

That said, I can drop this first patch because, in the end, I'm *not* going to
put QCryptoCipher in target/arm/cpu.h.


r~

On Mon, Aug 17, 2020 at 01:50:21PM -0700, Richard Henderson wrote:
> On 8/17/20 1:42 PM, Richard Henderson wrote:
> > On 8/17/20 1:38 PM, Richard Henderson wrote:
> >> On 8/17/20 9:48 AM, Daniel P. Berrangé wrote:
> >>> On Wed, Aug 12, 2020 at 08:25:21PM -0700, Richard Henderson wrote:
> >>>> This allows header files to declare pointers without pulling
> >>>> in the entire crypto subsystem.
> >>>>
> >>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >>>> ---
> >>>>  include/crypto/cipher.h | 2 --
> >>>>  include/qemu/typedefs.h | 1 +
> >>>>  2 files changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> I'm not in favour of this change or the next. Using #include "cipher.h"
> >>> is not a burden on the users of the crypto code. Moving typedefs away
> >>> from the associated struct is a step backwards IMHO.
> >>
> >> Consider if you put a pointer to QCryptoCipher in a relatively generic header
> >> file (e.g. "target/foo/cpu.h"), restricting "cipher.h" to a portion of the
> >> implementation (e.g. target/foo/helper_crypto.c).
> >>
> >> This sort of thing is exactly why "qemu/typedefs.h" exists.
> > 
> > As for the next patch for QCryptoCipherDriver, I could easily see not moving
> > the typedef to typedefs.h, but instaed to "crypto.h", where we do in fact want
> > to declare an incomplete structure.  I think it's a real mistake to be using
> > void* there at present.
> 
> That said, I can drop this first patch because, in the end, I'm *not* going to
> put QCryptoCipher in target/arm/cpu.h.

Thanks, I'd appreciate that. We can re-visit the discussion if needs
change again in future.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|