From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 753F5A0544; Fri, 23 Sep 2022 10:49:51 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 17C2D400D7; Fri, 23 Sep 2022 10:49:51 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 90D354003C for <dev@dpdk.org>; Fri, 23 Sep 2022 10:49:49 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663922989; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=e2T1zrB5xjjWgUkirDOfPrXYcJIGSgJE0uoeBrWIWZ4=; b=KoQS8kBeMipkk/wx5no8DT1/u017P8tZ02dpH9fhflLGOS6JNk8RlNtfsbA3kePguUTwhD MQiefDGBBtm1A+lxoac28a8CH+alOkdSnk3IfQr+iz/ud2N9vFKQbyC1h8koHS0GAVLhNJ EmaGcUYcYfjshGnspluEhcwSXcjsSmY= Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-599-q6lU50fJNCSyeKhj3EUptA-1; Fri, 23 Sep 2022 04:49:47 -0400 X-MC-Unique: q6lU50fJNCSyeKhj3EUptA-1 Received: by mail-lf1-f72.google.com with SMTP id q26-20020ac2529a000000b0049f564887d2so3967261lfm.17 for <dev@dpdk.org>; Fri, 23 Sep 2022 01:49:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=e2T1zrB5xjjWgUkirDOfPrXYcJIGSgJE0uoeBrWIWZ4=; b=vozymAPSG6rpNPxi/thCPFEzeT5edf/+dzNn1xnHi+KFXn9pmdptboTbaW7aFvpZv9 pAqDsret9hv2s6CGRcJr33PiiZpwYMkVm2Klph0VKRtGP10gU7WZqo9Og2oKwKdJgVtL vDoAjTT4HWXUJ9Jhb8Wjb8ULGnv+Gvpb1hkaVZP0Ln0SdhP6DyZU1bslsUzvyCy1qB4d ZcmciZaUk8DuqRR7Zb+GgbrM4TEFW3uCj6qGovj71l7aL/NR3Q/0TNRK/gBJMxOxP/gh xLGEmFvm5kKMSYIxa0gD4AX3M5hcOkIMZ4pZy97ol1Q79kb8ps49V8oQCqspAm/kqcKx R4GQ== X-Gm-Message-State: ACrzQf12/h3EMZ6kRnxDF5/bq7Ob4zdpNzgroPVSpUyI6YafGfwNZ4+e 7IqVVLAfnpuUcSOqBVh/g69qF2pquij8Cxyo6w/G7e8/sCEyejyPtrpl64+RzgoRr1C82rlpKyp 9ALs10aMizyxEl7d1KRs= X-Received: by 2002:a2e:8541:0:b0:261:b44b:1a8b with SMTP id u1-20020a2e8541000000b00261b44b1a8bmr2596936ljj.46.1663922986275; Fri, 23 Sep 2022 01:49:46 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6tAInPT1zj+Kg3MmsEyjPjd4x09sGSUyTNjoLB0qqi8YJ64A6LZ3UcG4mlxDm2ML5dIRD5qP8aCB5DE2BqGfc= X-Received: by 2002:a2e:8541:0:b0:261:b44b:1a8b with SMTP id u1-20020a2e8541000000b00261b44b1a8bmr2596914ljj.46.1663922986036; Fri, 23 Sep 2022 01:49:46 -0700 (PDT) MIME-Version: 1.0 References: <20220628144643.1213026-1-david.marchand@redhat.com> <20220628144643.1213026-12-david.marchand@redhat.com> <20220628162213.GA17875@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20220628092905.2da82a8c@hermes.local> <20220628170712.GC17875@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20220628103827.5060e318@hermes.local> <20220628182335.GD17875@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <CAJFAV8zfpZ0ci3zS8TJ+w5tYM4SWsPK8+VsV41fbU10OijYq+Q@mail.gmail.com> <20220709092803.433e6644@hermes.local> In-Reply-To: <20220709092803.433e6644@hermes.local> From: David Marchand <david.marchand@redhat.com> Date: Fri, 23 Sep 2022 10:49:34 +0200 Message-ID: <CAJFAV8zQDH9W_WECaPUQinjqiUoRerQK5LAu2NXE776X5iZjgw@mail.gmail.com> Subject: Re: [RFC PATCH 11/11] bus: hide bus object To: Stephen Hemminger <stephen@networkplumber.org>, Tyler Retzlaff <roretzla@linux.microsoft.com>, Thomas Monjalon <thomas@monjalon.net> Cc: dev <dev@dpdk.org>, Bruce Richardson <bruce.richardson@intel.com>, Kevin Laatz <kevin.laatz@intel.com>, Parav Pandit <parav@nvidia.com>, Xueming Li <xuemingl@nvidia.com>, Hemant Agrawal <hemant.agrawal@nxp.com>, Sachin Saxena <sachin.saxena@oss.nxp.com>, Rosen Xu <rosen.xu@intel.com>, Stephen Hemminger <sthemmin@microsoft.com>, Long Li <longli@microsoft.com>, Chas Williams <chas3@att.com>, "Min Hu (Connor)" <humin29@huawei.com>, Matan Azrad <matan@nvidia.com>, Ray Kinsella <mdr@ashroe.eu>, Ferruh Yigit <ferruh.yigit@xilinx.com>, Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org On Sat, Jul 9, 2022 at 6:28 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > > > > > > to avoid people tripping over mishandling pointers in/out of the api > > > > > > > surface taking the opaque object you could declare opaque handle for the > > > > > > > api to operate on instead. it would force the use of a cast in the > > > > > > > implementation but would avoid accidental void * of the wrong thing that > > > > > > > got cached being passed in. if the cast was really undesirable just > > > > > > > whack it under an inline / internal function. > > > > > > > > > > > > I don't like that because it least to dangerous casts in the internal code. > > > > > > Better to keep the the type of the object. As long as the API only passes > > > > > > around an pointer to a struct, without exposing the contents of the struct; > > > > > > it is safer and easier to debug. > > > > > > > > > > as i mentioned you can use an inline/internal function or even a macro > > > > > to hide the cast, you could provide some additional integrity checks > > > > > here if desired as a value add. > > > > > > > > > > the fact that you expose that it is a struct is an internal > > > > > implementation detail, if truly opaque tomorrow you could convert it > > > > > to a simple integer that indexes or enumerates something and prevents > > > > > any meaningful interpretation in the application. > > > > > > > > > > when you say it is safer to debug i think you mean for dpdk devs not the > > > > > application developer because unless the app developer does something > > > > > really gross/dangerous casting they really can't "mishandle" the opaque > > > > > object except to use one that isn't initialized at all which we > > > > > can detect and handle internally in a general way. > > > > > > > > > > i will however concede there would be slightly more finger work when > > > > > debugging dpdk itself since gdb / debugger doesn't automatically infer > > > > > type so you end up having to tell gdb what is in the uintptr_t. > > > > > > > > > > anyway just drawing from experience in the driver frameworks we maintain > > > > > in windows, i think one of our regrets is that we didn't do this from > > > > > day 1 and subsequentl that we initially only used one opaque type > > > > > instead of defining separate (not implicitly convertable) types to each > > > > > opaque type. > > > > > > > > It seems to be a difference in style/taste. > > > > > > it's not i've sited at least one example of a mistake that becomes a > > > compile time failure where application code is incorrectly authored > > > where use of a pointer offers no such protection. > > > > > > > The Linux/Unix side prefers opaque structure pointers. > > > > Windows (and LLVM) uses numeric handles. > > > > > > > > At this point DPDK should follow the Linux bus. > > > > > > dpdk is multi-platform and unix does not necessarily standardize on > > > pointer to struct for opaque objects. freebsd has many apis notably > > > bus_space that does uses handles and as previously mentioned posix > > > threads uses handles. > > > > > > i understand that linux is an important platform but it isn't the only > > > platform dpdk targets and just because it is important doesn't mean it > > > should always enjoy being the defacto standard. > > > > > > anyway, i'll leave it for the patch author to decide. i still like the > > > patch series either way. i just think this would make applications more > > > robust. > > > > Thanks for this feedback Tyler. > > I would lean towards Stephen opinion atm, but I am not decided yet. > > > > For now, I'll post a v2, extending the series to other internal objects. > > We can conclude on this topic during 22.11. > > If you get chance to deconstruct API, switching to a numeric index > is safest similar to what Tyler suggested. Think of ethdev port number > and Posix file descriptor model. The advantage of an index is that > it can be validated more easily by the code that is called. This is still a heavy change in the APIs. The deprecation notice did not mention such change, and I don't think many people are aware of this suggestion. This would need more discussion but I don't want to block the series on concluding on this question. So I am for going ahead with the series in its current form. -- David Marchand