| Message ID | 20200813032537.2888593-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | crypto/cipher: Class hierarchy cleanups | expand |
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 :|
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;
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(-) -- 2.25.1