Re: [Qemu-devel] [PATCH 01/12] chardev: fix validation of options for QMP created chardevs

[Markus Armbruster]

Eric, there's a QAPI code generation idea at the end.

Daniel P. Berrangé <address@hidden> writes:

> On Wed, Jan 16, 2019 at 06:07:41AM +0100, Thomas Huth wrote:
>> On 2019-01-15 15:52, Daniel P. Berrangé wrote:
>> > The TLS creds option is not valid with certain address types. The user
>> > config was only checked for errors when parsing legacy QemuOpts, thus
>> > the user could pass unsupported values via QMP.
>> > 
>> > Pull all code for validating options out into a new method
>> > qmp_chardev_validate_socket, that is called from the main
>> > qmp_chardev_open_socket method. This adds a missing check for rejecting
>> > TLS creds with the vsock address type.
>> > 
>> > Signed-off-by: Daniel P. Berrangé <address@hidden>
>> > ---
>> >  chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
>> >  1 file changed, 66 insertions(+), 26 deletions(-)
>> > 
>> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> > index eaa8e8b68f..6669acb35f 100644
>> > --- a/chardev/char-socket.c
>> > +++ b/chardev/char-socket.c
>> > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer 
>> > opaque)
>> >      return false;
>> >  }
>> >  
>> > +
>> 
>> Please remove the additional empty line.
>
> Having two blanks lines between functions is intentional to
> give visual separation.
>
>> > +static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>> > +                                        SocketAddress *addr,
>> > +                                        Error **errp)
>> > +{
>> > +    /* Validate any options which have a dependancy on address type */
>> 
>> I'd maybe rather write "dependency" which is AFAIK the more common
>> spelling - but I'm not a native speaker, so feel free to ignore me here.

For what it's worth, my dictionary wants dependency.

>> > +    switch (addr->type) {
>> > +    case SOCKET_ADDRESS_TYPE_FD:
>> > +        if (sock->has_reconnect) {
>> > +            error_setg(errp,
>> > +                       "'reconnect' option is incompatible with "
>> > +                       "'fd' address type");
>> > +            return false;
>> > +        }
>> > +        if (sock->has_tls_creds &&
>> > +            !(sock->has_server && sock->server)) {
>> > +            error_setg(errp,
>> > +                       "'tls_creds' option is incompatible with "
>> > +                       "'fd' address type as client");
>> > +            return false;
>> > +        }
>> > +        break;
>> > +
>> > +    case SOCKET_ADDRESS_TYPE_UNIX:
>> > +        if (sock->has_tls_creds) {
>> > +            error_setg(errp,
>> > +                       "'tls_creds' option is incompatible with "
>> > +                       "'unix' address type");
>> > +            return false;
>> > +        }
>> > +        break;
>> > +
>> > +    case SOCKET_ADDRESS_TYPE_INET:
>> > +        break;
>> 
>> You could drop the empty case.
>
> I preferred to explicitly list all cases, so it is clear what
> needs to be handled here when further checks are added later.

Matter of taste, your choice unless maintainer overrules.

>> 
>> > +    case SOCKET_ADDRESS_TYPE_VSOCK:
>> > +        if (sock->has_tls_creds) {
>> > +            error_setg(errp,
>> > +                       "'tls_creds' option is incompatible with "
>> > +                       "'vsock' address type");
>> > +            return false;
>> > +        }
>> > +
>
> Opps, missing default.

I guess you mean break.

>> > +    default:
>> > +        break;
>> 
>> You could drop the empty default case.
>
> If that is not there, then the compiler forces the
> listing of SOCKET_ADDRESS_TYPE__MAX instead due
> to -Wswitch

I wonder whether generating something like

    typedef enum SocketAddressType {
        SOCKET_ADDRESS_TYPE_INET,
        SOCKET_ADDRESS_TYPE_UNIX,
        SOCKET_ADDRESS_TYPE_VSOCK,
        SOCKET_ADDRESS_TYPE_FD,
    } SocketAddressType;

    #define SOCKET_ADDRESS_TYPE__MAX (SOCKET_ADDRESS_TYPE_FD + 1)

would be better.