Re: [Qemu-devel] [PATCH 2/4] scsi: build qemu-pr-helper

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Sep 19, 2017 at 12:24:32PM +0200, Paolo Bonzini wrote:
> Introduce a privileged helper to run persistent reservation commands.
> This lets virtual machines send persistent reservations without using
> CAP_SYS_RAWIO or out-of-tree patches.  The helper uses Unix permissions
> and SCM_RIGHTS to restrict access to processes that can access its socket
> and prove that they have an open file descriptor for a raw SCSI device.
> 
> The next patch will also correct the usage of persistent reservations
> with multipath devices.
> 
> It would also be possible to support for Linux's IOC_PR_* ioctls in
> the future, to support NVMe devices.  For now, however, only SCSI is
> supported.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---


> +
> +#define PR_HELPER_CDB_SIZE     16
> +#define PR_HELPER_SENSE_SIZE   96
> +#define PR_HELPER_DATA_SIZE    8192
> +
> +typedef struct PRHelperResponse {
> +    int32_t result;
> +    int32_t sz;
> +    uint8_t sense[PR_HELPER_SENSE_SIZE];
> +} PRHelperResponse;

Should we annotate this with 'packed' to ensure its immune to compiler
padding ?


> +typedef struct PRHelperRequest {
> +    int fd;
> +    size_t sz;
> +    uint8_t cdb[PR_HELPER_CDB_SIZE];
> +} PRHelperRequest;

Same q here ?


> +static int coroutine_fn prh_write_response(PRHelperClient *client,
> +                                           PRHelperRequest *req,
> +                                           PRHelperResponse *resp, Error **errp)
> +{
> +    ssize_t r;

Can just be int

> +    size_t sz;
> +
> +    if (req->cdb[0] == PERSISTENT_RESERVE_IN && resp->result == GOOD) {
> +        assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data));
> +    } else {
> +        assert(resp->sz == 0);
> +    }
> +
> +    sz = resp->sz;
> +
> +    resp->result = cpu_to_be32(resp->result);
> +    resp->sz = cpu_to_be32(resp->sz);
> +    r = qio_channel_write_all(QIO_CHANNEL(client->ioc),
> +                              (char *) resp, sizeof(*resp), errp);
> +    if (r < 0) {
> +        return r;
> +    }
> +
> +    r = qio_channel_write_all(QIO_CHANNEL(client->ioc),
> +                              (char *) client->data,
> +                              sz, errp);
> +    return r < 0 ? r : 0;

Just return 'r' directly, its only ever -1 or 0



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 :|