[{"id":1771037,"web_url":"http://patchwork.ozlabs.org/comment/1771037/","msgid":"<20170919133351.GO9536@redhat.com>","list_archive_url":null,"date":"2017-09-19T13:33:51","subject":"Re: [Qemu-devel] [PATCH 2/4] scsi: build qemu-pr-helper","submitter":{"id":2694,"url":"http://patchwork.ozlabs.org/api/people/2694/","name":"Daniel P. Berrangé","email":"berrange@redhat.com"},"content":"On Tue, Sep 19, 2017 at 12:24:32PM +0200, Paolo Bonzini wrote:\n> Introduce a privileged helper to run persistent reservation commands.\n> This lets virtual machines send persistent reservations without using\n> CAP_SYS_RAWIO or out-of-tree patches. The helper uses Unix permissions\n> and SCM_RIGHTS to restrict access to processes that can access its socket\n> and prove that they have an open file descriptor for a raw SCSI device.\n> \n> The next patch will also correct the usage of persistent reservations\n> with multipath devices.\n> \n> It would also be possible to support for Linux's IOC_PR_* ioctls in\n> the future, to support NVMe devices. For now, however, only SCSI is\n> supported.\n> \n> Signed-off-by: Paolo Bonzini \n> ---\n\n\n> +\n> +#define PR_HELPER_CDB_SIZE 16\n> +#define PR_HELPER_SENSE_SIZE 96\n> +#define PR_HELPER_DATA_SIZE 8192\n> +\n> +typedef struct PRHelperResponse {\n> + int32_t result;\n> + int32_t sz;\n> + uint8_t sense[PR_HELPER_SENSE_SIZE];\n> +} PRHelperResponse;\n\nShould we annotate this with 'packed' to ensure its immune to compiler\npadding ?\n\n\n> +typedef struct PRHelperRequest {\n> + int fd;\n> + size_t sz;\n> + uint8_t cdb[PR_HELPER_CDB_SIZE];\n> +} PRHelperRequest;\n\nSame q here ?\n\n\n> +static int coroutine_fn prh_write_response(PRHelperClient *client,\n> + PRHelperRequest *req,\n> + PRHelperResponse *resp, Error **errp)\n> +{\n> + ssize_t r;\n\nCan just be int\n\n> + size_t sz;\n> +\n> + if (req->cdb[0] == PERSISTENT_RESERVE_IN && resp->result == GOOD) {\n> + assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data));\n> + } else {\n> + assert(resp->sz == 0);\n> + }\n> +\n> + sz = resp->sz;\n> +\n> + resp->result = cpu_to_be32(resp->result);\n> + resp->sz = cpu_to_be32(resp->sz);\n> + r = qio_channel_write_all(QIO_CHANNEL(client->ioc),\n> + (char *) resp, sizeof(*resp), errp);\n> + if (r < 0) {\n> + return r;\n> + }\n> +\n> + r = qio_channel_write_all(QIO_CHANNEL(client->ioc),\n> + (char *) client->data,\n> + sz, errp);\n> + return r < 0 ? r : 0;\n\nJust return 'r' directly, its only ever -1 or 0\n\n\n\nRegards,\nDaniel","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=berrange@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxPPq1nPhz9s7h\n\tfor ;\n\tTue, 19 Sep 2017 23:49:51 +1000 (AEST)","from localhost ([::1]:43015 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duIuJ-000207-DW\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 09:49:47 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:48144)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duIfD-0006z4-1B\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:34:12 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duIf7-0003PC-4T\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:34:11 -0400","from mx1.redhat.com ([209.132.183.28]:46388)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duIey-0003HQ-9S; Tue, 19 Sep 2017 09:33:56 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 58C43C6550;\n\tTue, 19 Sep 2017 13:33:55 +0000 (UTC)","from redhat.com (unknown [10.42.22.189])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 65F305D97C;\n\tTue, 19 Sep 2017 13:33:54 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 58C43C6550","Date":"Tue, 19 Sep 2017 14:33:51 +0100","From":"\"Daniel P. Berrange\" ","To":"Paolo Bonzini ","Message-ID":"<20170919133351.GO9536@redhat.com>","References":"<20170919102434.21147-1-pbonzini@redhat.com>\n\t<20170919102434.21147-3-pbonzini@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20170919102434.21147-3-pbonzini@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tTue, 19 Sep 2017 13:33:55 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH 2/4] scsi: build qemu-pr-helper","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"\"Daniel P. Berrange\" ","Cc":"qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771079,"web_url":"http://patchwork.ozlabs.org/comment/1771079/","msgid":"<0e5ceed7-ca5c-f771-fa58-72766f07373b@redhat.com>","list_archive_url":null,"date":"2017-09-19T14:25:09","subject":"Re: [Qemu-devel] [PATCH 2/4] scsi: build qemu-pr-helper","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 19/09/2017 15:33, Daniel P. Berrange wrote:\n> On Tue, Sep 19, 2017 at 12:24:32PM +0200, Paolo Bonzini wrote:\n>> Introduce a privileged helper to run persistent reservation commands.\n>> This lets virtual machines send persistent reservations without using\n>> CAP_SYS_RAWIO or out-of-tree patches. The helper uses Unix permissions\n>> and SCM_RIGHTS to restrict access to processes that can access its socket\n>> and prove that they have an open file descriptor for a raw SCSI device.\n>>\n>> The next patch will also correct the usage of persistent reservations\n>> with multipath devices.\n>>\n>> It would also be possible to support for Linux's IOC_PR_* ioctls in\n>> the future, to support NVMe devices. For now, however, only SCSI is\n>> supported.\n>>\n>> Signed-off-by: Paolo Bonzini \n>> ---\n> \n> \n>> +\n>> +#define PR_HELPER_CDB_SIZE 16\n>> +#define PR_HELPER_SENSE_SIZE 96\n>> +#define PR_HELPER_DATA_SIZE 8192\n>> +\n>> +typedef struct PRHelperResponse {\n>> + int32_t result;\n>> + int32_t sz;\n>> + uint8_t sense[PR_HELPER_SENSE_SIZE];\n>> +} PRHelperResponse;\n> \n> Should we annotate this with 'packed' to ensure its immune to compiler\n> padding ?\n\nYes...\n\n> \n>> +typedef struct PRHelperRequest {\n>> + int fd;\n>> + size_t sz;\n>> + uint8_t cdb[PR_HELPER_CDB_SIZE];\n>> +} PRHelperRequest;\n> \n> Same q here ?\n\n... and no since this one is not the wire format (which is just a\n16-byte cdb, plus fd in ancillary data).\n\n> \n>> +static int coroutine_fn prh_write_response(PRHelperClient *client,\n>> + PRHelperRequest *req,\n>> + PRHelperResponse *resp, Error **errp)\n>> +{\n>> + ssize_t r;\n> \n> Can just be int\n> \n>> + size_t sz;\n>> +\n>> + if (req->cdb[0] == PERSISTENT_RESERVE_IN && resp->result == GOOD) {\n>> + assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data));\n>> + } else {\n>> + assert(resp->sz == 0);\n>> + }\n>> +\n>> + sz = resp->sz;\n>> +\n>> + resp->result = cpu_to_be32(resp->result);\n>> + resp->sz = cpu_to_be32(resp->sz);\n>> + r = qio_channel_write_all(QIO_CHANNEL(client->ioc),\n>> + (char *) resp, sizeof(*resp), errp);\n>> + if (r < 0) {\n>> + return r;\n>> + }\n>> +\n>> + r = qio_channel_write_all(QIO_CHANNEL(client->ioc),\n>> + (char *) client->data,\n>> + sz, errp);\n>> + return r < 0 ? r : 0;\n> \n> Just return 'r' directly, its only ever -1 or 0\n\nOk, thanks, will adjust all of these.\n\nPaolo","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxQCr4BGfz9s7h\n\tfor ;\n\tWed, 20 Sep 2017 00:26:16 +1000 (AEST)","from localhost ([::1]:43219 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duJTa-0004SQ-KV\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 10:26:14 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:51881)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duJSo-0004MZ-Bd\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:25:32 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duJSk-0003Af-A5\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:25:26 -0400","from mx1.redhat.com ([209.132.183.28]:52068)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duJSb-00033a-4V; Tue, 19 Sep 2017 10:25:13 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 18F76C0587E4;\n\tTue, 19 Sep 2017 14:25:12 +0000 (UTC)","from [10.36.117.61] (ovpn-117-61.ams2.redhat.com [10.36.117.61])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 2614260618;\n\tTue, 19 Sep 2017 14:25:10 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 18F76C0587E4","To":"\"Daniel P. Berrange\" ","References":"<20170919102434.21147-1-pbonzini@redhat.com>\n\t<20170919102434.21147-3-pbonzini@redhat.com>\n\t<20170919133351.GO9536@redhat.com>","From":"Paolo Bonzini ","Message-ID":"<0e5ceed7-ca5c-f771-fa58-72766f07373b@redhat.com>","Date":"Tue, 19 Sep 2017 16:25:09 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170919133351.GO9536@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tTue, 19 Sep 2017 14:25:12 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH 2/4] scsi: build qemu-pr-helper","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]