From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3BF78A00C4; Fri, 5 Jun 2020 02:16:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 111851D608; Fri, 5 Jun 2020 02:16:09 +0200 (CEST) Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) by dpdk.org (Postfix) with ESMTP id E25201D5FB for ; Fri, 5 Jun 2020 02:16:06 +0200 (CEST) Received: by mail-lj1-f193.google.com with SMTP id e4so9574594ljn.4 for ; Thu, 04 Jun 2020 17:16:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=S+TC7N99zXT5a6/RsXjHec+cozrJNFNEpJT2rKyvjNI=; b=iz4icVHKUPr5V6jskLvRnTvOeQPg/5T0tz6tkNf47eTrKVaIXsVzHOSnWnxbWwpJjT py9LR2w5v9EXtcYgtezd4GJHuRpKVA5ktncA5rPA7cOuyuTz2lq4hCUcFTMz7j2wV92j Fo3/CJ6AEzAkxXeI6hYZIlBzf6rXnqZssU6D6fLjfiEO+5GZT6hQphovs3aYUOIi6RrV r6sZ3MdRj4zfyO1DJyCIrCBDsDTp4f18GiMv2zI0biIfqZ4qU+iQg7bfQABY5JmrAZIo trPXzasuomXcALRB9S+6aPrO28ZcD+JhjD5J1QIbjD6bkID8cOloSoiXOkFAow467m+3 D79g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=S+TC7N99zXT5a6/RsXjHec+cozrJNFNEpJT2rKyvjNI=; b=oHlSyXFHcQW4OWAFyS4MWwmVUE4hjODNpYjnR07iIp+9EcUcnSUBbKqk57bIx5mnoY tqJqG8QOhHx8pnURZwmvml+FJg9IGEJrKOoPHwn6oDr3enlQ0amRdeBzQtaYF90U+cEx kD6bOmcbvUP8QdKYJIagTSKERNZ6Ll0U2nV3gFBWsL/htBdCFoZ9RJmHHvTvcsglt2Ng M+rar/AGoVCxcbtqF80Bxg+fRVqUTFhYFOSVUmTo5uMB7abkk/c+fZ4q4Jo91p+Y4gjU sfOUy4BO+be3X1/tGh7QRPiryV7X4nh2TUNAvHyM5L7ALE8vJ+MZTPwI5Fjqn7QXAA8M jq9w== X-Gm-Message-State: AOAM532q8UrM4ir0PhpmNm+aaubLPNtjyFrED6mM9g+xMMghUqwd78Ck GghH2zT5Jbe+zmGWdVIlCPI= X-Google-Smtp-Source: ABdhPJyzx+XUpZOgwiPvr4gFaoBkILLlty6jkKecEa3UHHJUAUiDnDAVZ7AcOPmDcwqpzps1ur/mbA== X-Received: by 2002:a2e:150e:: with SMTP id s14mr3188144ljd.290.1591316165692; Thu, 04 Jun 2020 17:16:05 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id y22sm271546lfh.12.2020.06.04.17.16.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 Jun 2020 17:16:04 -0700 (PDT) Date: Fri, 5 Jun 2020 03:16:03 +0300 From: Dmitry Kozlyuk To: Neil Horman Cc: dev@dpdk.org, Dmitry Malloy , Narcisa Ana Maria Vasile , Fady Bader , Tal Shnaiderman , Thomas Monjalon , Anatoly Burakov , Bruce Richardson Message-ID: <20200605031603.2d8aec8b@sovereign> In-Reply-To: <20200604210707.GA789657@hmswarspite.think-freely.org> References: <20200525003720.6410-1-dmitry.kozliuk@gmail.com> <20200602230329.17838-1-dmitry.kozliuk@gmail.com> <20200602230329.17838-3-dmitry.kozliuk@gmail.com> <20200603120759.GA426574@hmswarspite.think-freely.org> <20200603153403.2e58ef90@sovereign> <20200604210707.GA789657@hmswarspite.think-freely.org> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 02/11] eal: introduce internal wrappers for file operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, 4 Jun 2020 17:07:07 -0400 Neil Horman wrote: > On Wed, Jun 03, 2020 at 03:34:03PM +0300, Dmitry Kozlyuk wrote: > > On Wed, 3 Jun 2020 08:07:59 -0400 > > Neil Horman wrote: > > > > [snip] > > > > +int > > > > +eal_file_create(const char *path) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = open(path, O_CREAT | O_RDWR, 0600); > > > > + if (ret < 0) > > > > + rte_errno = errno; > > > > + > > > > + return ret; > > > > +} > > > > + > > > You don't need this call if you support the oflags option in the open call > > > below. > > > > See below. > > > > > > +int > > > > +eal_file_open(const char *path, bool writable) > > > > +{ > > > > + int ret, flags; > > > > + > > > > + flags = writable ? O_RDWR : O_RDONLY; > > > > + ret = open(path, flags); > > > > + if (ret < 0) > > > > + rte_errno = errno; > > > > + > > > > + return ret; > > > > +} > > > > + > > > why are you changing this api from the posix file format (with oflags > > > specified). As far as I can see both unix and windows platforms support that > > > > There is a number of caveats, which IMO make this approach better: > > > > 1. Filesystem permissions on Windows are complicated. Supporting anything > > other than 0600 would add a lot of code, while EAL doesn't really need it. > > Microsoft's open() takes not permission bits, but a set of flags. > > > > 2. Restricted interface prevents EAL developers from accidentally using > > features not supported on all platforms via a seemingly rich API. > > > > 3. Microsoft CRT (the one Clang is using) deprecates open() in favor of > > _sopen_s() and issues a warning, and we're targeting -Werror. Disabling all > > such warnings (_CRT_SECURE_NO_DEPRECATE) doesn't seem right when CRT vendor > > encourages using alternatives. This is the primary reason for open() > > wrappers in v6. > > > > that seems a bit shortsighted to me. By creating wrappers that restrict > functionality to the least common demoninator of supported platforms restricts > what all platforms are capable of. For example, theres no reason that the eal > library shouldn't be able to open a file O_TRUNC or O_SYNC just because its > complex to do it on a single platform. The purpose of these wrappers is to maximize reuse of common code. It doesn't require POSIX par se, it's just implemented in terms of API that had been available on all supported OSes until Windows target was introduced. Wrapper interface is derived from common code requirements. > The API should be written to support the full range of functionality on all > platforms, and the individual implementations should write the code to make that > happen, or return an error that its unsupported on this particular platform. IMO, common code, by definition, should avoid partial support of anything. > I'm not saying that you have to implement everything now, but you shouldn't > restrict the API from being able to do so in the future. Otherwise, in the > future, if someone wants to implement O_TRUNC support (just to site an example), > they're going to have to make a change to the API above, and alter the > implementation for all the platforms anyway. You may as well make the API > robust enough to support that now. I agree that these particular wrappers can have a lot more options, so probably flags would be better. However, I wouldn't add parameters that have partial support, namely, permissions. What do you think of the following (names shortened)? enum mode { RO = 0, /* write-only is not portable */ RW = 1, CREATE = 2 /* always 0600 equivalent */ }; eal_file_open(const char *path, int mode); -- Dmitry Kozlyuk