Re: [Qemu-devel] [PATCH v2 02/15] crypto: introduce crypto queue handler

[Gonglei (Arei)]

Hi,

All comments are accepted :)


Regards,
-Gonglei


> -----Original Message-----
> From: Daniel P. Berrange [mailto:address@hidden
> Sent: Tuesday, September 13, 2016 5:21 PM
> To: Gonglei (Arei)
> Cc: address@hidden; address@hidden; Huangpeng
> (Peter); Luonengjun; address@hidden; address@hidden;
> address@hidden; Huangweidong (C); address@hidden;
> address@hidden; address@hidden; Claudio Fontana; address@hidden;
> address@hidden
> Subject: Re: [PATCH v2 02/15] crypto: introduce crypto queue handler
> 
> On Tue, Sep 13, 2016 at 11:52:08AM +0800, Gonglei wrote:
> > crypto queue is a gallery used for executing crypto
> > operation, which supports both synchronization and
> > asynchronization. The thoughts stolen from net/queue.c
> >
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> >  crypto/Makefile.objs          |   1 +
> >  crypto/crypto-queue.c         | 206
> ++++++++++++++++++++++++++++++++++++++++++
> >  crypto/crypto.c               |  28 ++++++
> >  include/crypto/crypto-queue.h |  69 ++++++++++++++
> >  include/crypto/crypto.h       |  12 +++
> >  5 files changed, 316 insertions(+)
> >  create mode 100644 crypto/crypto-queue.c
> >  create mode 100644 include/crypto/crypto-queue.h
> >
> 
> > diff --git a/include/crypto/crypto-queue.h b/include/crypto/crypto-queue.h
> > new file mode 100644
> > index 0000000..6fba64d
> > --- /dev/null
> > +++ b/include/crypto/crypto-queue.h
> > @@ -0,0 +1,69 @@
> > +/*
> > + * Copyright (c) 2003-2008 Fabrice Bellard
> > + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * Authors:
> > + *    Gonglei <address@hidden>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> > + * of this software and associated documentation files (the "Software"), to
> deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> 
> Again, wrong license header.
> 
> > +#ifndef QEMU_CRYPTO_QUEUE_H
> > +#define QEMU_CRYPTO_QUEUE_H
> > +
> > +#include "qemu-common.h"
> > +
> > +typedef struct CryptoPacket CryptoPacket;
> > +typedef struct CryptoQueue CryptoQueue;
> > +typedef struct CryptoPacketBuf CryptoPacketBuf;
> > +
> > +typedef void (CryptoPacketSent) (CryptoClientState *, int);
> 
> As previously, I'd expect naming of
> 
>  QCryptoCryptodevPacket
>  QCryptoCryptodevPacketBuf
>  QCryptoCryptodevQueue
> 
> > +
> > +
> > +/* Returns:
> > + *   >0 - success
> > + *    0 - queue packet for future redelivery
> > + *   <0 - failure (discard packet)
> > + */
> > +typedef int (CryptoQueueDeliverFunc)(CryptoClientState *sender,
> > +                                     unsigned flags,
> > +                                     void *header_opaque,
> > +                                     void *opaque);
> > +
> > +CryptoQueue *
> > +qemu_new_crypto_queue(CryptoQueueDeliverFunc *deliver, void *opaque);
> > +
> > +void qemu_crypto_queue_cache(CryptoQueue *queue,
> > +                               unsigned flags,
> > +                               CryptoClientState *sender,
> > +                               void *opaque,
> > +                               CryptoPacketSent *sent_cb);
> > +
> > +void qemu_del_crypto_queue(CryptoQueue *queue);
> > +
> > +int qemu_crypto_queue_send(CryptoQueue *queue,
> > +                                unsigned flags,
> > +                                CryptoClientState *sender,
> > +                                void *opaque,
> > +                                CryptoPacketSent *sent_cb);
> > +
> > +void qemu_crypto_queue_purge(CryptoQueue *queue, CryptoClientState
> *from);
> > +bool qemu_crypto_queue_flush(CryptoQueue *queue);
> 
> And naming of
> 
>   qcrypto_cryptodev_queue_purge
>   qcrypto_cryptodev_queue_flush
>   etc.
> 
> Also missing docs for this file
> 
> > +#endif /* QEMU_CRYPTO_QUEUE_H */
> > diff --git a/include/crypto/crypto.h b/include/crypto/crypto.h
> > index f93f6f9..46b3b9e 100644
> > --- a/include/crypto/crypto.h
> > +++ b/include/crypto/crypto.h
> > @@ -29,6 +29,8 @@
> >
> >  #include "qemu/queue.h"
> >  #include "qapi-types.h"
> > +#include "crypto/crypto-queue.h"
> > +
> >
> >  typedef void (CryptoPoll)(CryptoClientState *, bool);
> >  typedef void (CryptoCleanup) (CryptoClientState *);
> > @@ -52,6 +54,8 @@ struct CryptoClientState {
> >      char *model;
> >      char *name;
> >      char info_str[256];
> > +    CryptoQueue *incoming_queue;
> > +    unsigned int queue_index;
> >      CryptoClientDestructor *destructor;
> >  };
> >
> > @@ -62,5 +66,13 @@ CryptoClientState
> *new_crypto_client(CryptoClientInfo *info,
> >                                      CryptoClientState *peer,
> >                                      const char *model,
> >                                      const char *name);
> > +int qemu_deliver_crypto_packet(CryptoClientState *sender,
> > +                              unsigned flags,
> > +                              void *header_opqaue,
> > +                              void *opaque);
> > +int qemu_send_crypto_packet_async(CryptoClientState *sender,
> > +                                unsigned flags,
> > +                                void *opaque,
> > +                                CryptoPacketSent *sent_cb);
> 
> Missing docs for these API additions.
> 
> 
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-
> http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-
> http://virt-manager.org :|
> |: http://autobuild.org       -o-
> http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-
> http://live.gnome.org/gtk-vnc :|