* Re: [RFC net-next v2 1/8] net: sched: register callbacks for indirect tc block binds
From: Or Gerlitz @ 2018-10-29 15:12 UTC (permalink / raw)
To: John Hurley
Cc: Linux Netdev List, oss-drivers, Jiri Pirko, Oz Shlomo,
Jakub Kicinski, Simon Horman, Aviv Heller
In-Reply-To: <CAK+XE=mh6Ktka1MJkFzuGk8urJqAtBaaJX4ZfrFEHc+pzN-FVw@mail.gmail.com>
On Mon, Oct 29, 2018 at 2:54 PM John Hurley <john.hurley@netronome.com> wrote:
> On Sun, Oct 28, 2018 at 11:10 AM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> >
> > On Thu, Oct 25, 2018 at 3:28 PM John Hurley <john.hurley@netronome.com> wrote:
> > > Currently drivers can register to receive TC block bind/unbind callbacks
> > > by implementing the setup_tc ndo in any of their given netdevs. However,
> > > drivers may also be interested in binds to higher level devices (e.g.
> > > tunnel drivers) to potentially offload filters applied to them.
> >
> > > Introduce indirect block devs which allows drivers to register callbacks
> > > for block binds on other devices. The calling driver is expected to
> > > reference an 'owner' struct that it will pass to all block registrations.
> > > This is used to track the callbacks from a given driver and free them if
> > > the driver is removed while the upper level device is still active.
>> Maybe it would be better to follow the trusted environment model of the kernel
>> and not protect the core from driver bugs? If the driver does things right they
>> will unregister before bailing out and if not, they will have to fix..
> The owner stuff just makes it easier for a driver to track the blocks
> it has registered for and, in turn, release these when exiting.
> We could just leave this up to the driver to ensure it properly cleans
> up after itself.
If it makes the life of the driver easier and doesn't add notable complexity,
then I think I am good to leave it
> I don't feel that strongly either way.
m2
So lets see if other comment here, if not, we can just leave it, I guess
> > > Allow registering an indirect block dev callback for a device that is
> > > already bound to a block. In this case (if it is an ingress block),
> > > register and also trigger the callback meaning that any already installed
> > > rules can be replayed to the calling driver.
> > not just can be replayed.. they will be replayed, but through an
> > existing (tc re-offload?) facility, correct?
> Yes, currently in TC, when you register for rule callbacks to a block
> that already has rules, these rules are replayed.
> With the indirect block approach we still use the same mechanism for
> requesting rule callbacks,
sounds good
^ permalink raw reply
* [PATCH net] sctp: check policy more carefully when getting pr status
From: Xin Long @ 2018-10-29 15:13 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman
When getting pr_assocstatus and pr_streamstatus by sctp_getsockopt,
it doesn't correctly process the case when policy is set with
SCTP_PR_SCTP_ALL | SCTP_PR_SCTP_MASK. It even causes a
slab-out-of-bounds in sctp_getsockopt_pr_streamstatus().
This patch fixes it by return -EINVAL for this case.
Fixes: 0ac1077e3a54 ("sctp: get pr_assoc and pr_stream all status with SCTP_PR_SCTP_ALL")
Reported-by: syzbot+5da0d0a72a9e7d791748@syzkaller.appspotmail.com
Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/socket.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index fc0386e..739f3e5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -7083,14 +7083,15 @@ static int sctp_getsockopt_pr_assocstatus(struct sock *sk, int len,
}
policy = params.sprstat_policy;
- if (!policy || (policy & ~(SCTP_PR_SCTP_MASK | SCTP_PR_SCTP_ALL)))
+ if (!policy || (policy & ~(SCTP_PR_SCTP_MASK | SCTP_PR_SCTP_ALL)) ||
+ ((policy & SCTP_PR_SCTP_ALL) && (policy & SCTP_PR_SCTP_MASK)))
goto out;
asoc = sctp_id2assoc(sk, params.sprstat_assoc_id);
if (!asoc)
goto out;
- if (policy & SCTP_PR_SCTP_ALL) {
+ if (policy == SCTP_PR_SCTP_ALL) {
params.sprstat_abandoned_unsent = 0;
params.sprstat_abandoned_sent = 0;
for (policy = 0; policy <= SCTP_PR_INDEX(MAX); policy++) {
@@ -7142,7 +7143,8 @@ static int sctp_getsockopt_pr_streamstatus(struct sock *sk, int len,
}
policy = params.sprstat_policy;
- if (!policy || (policy & ~(SCTP_PR_SCTP_MASK | SCTP_PR_SCTP_ALL)))
+ if (!policy || (policy & ~(SCTP_PR_SCTP_MASK | SCTP_PR_SCTP_ALL)) ||
+ ((policy & SCTP_PR_SCTP_ALL) && (policy & SCTP_PR_SCTP_MASK)))
goto out;
asoc = sctp_id2assoc(sk, params.sprstat_assoc_id);
--
2.1.0
^ permalink raw reply related
* Re: [PATCH iproute2] Use libbsd for strlcpy if available
From: David Ahern @ 2018-10-29 15:27 UTC (permalink / raw)
To: Luca Boccassi, netdev; +Cc: stephen
In-Reply-To: <20181029104650.24924-1-bluca@debian.org>
On 10/29/18 4:46 AM, Luca Boccassi wrote:
> If libc does not provide strlcpy check for libbsd with pkg-config to
> avoid relying on inline version.
>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
> This allows distro maintainers to be able to choose to reduce
> duplication and let this code be maintained in one place, in the
> external library.
>
> configure | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index 744d6282..1dd9ce84 100755
> --- a/configure
> +++ b/configure
> @@ -330,8 +330,16 @@ EOF
> then
> echo "no"
> else
> - echo 'CFLAGS += -DNEED_STRLCPY' >>$CONFIG
> - echo "yes"
> + if ${PKG_CONFIG} libbsd --exists
> + then
> + echo 'CFLAGS += -include' `${PKG_CONFIG} libbsd --variable=includedir`'/bsd/string.h' \
> + `${PKG_CONFIG} libbsd --cflags` >>$CONFIG
> + echo 'LDLIBS +=' `${PKG_CONFIG} libbsd --libs` >> $CONFIG
> + echo "no"
> + else
> + echo 'CFLAGS += -DNEED_STRLCPY' >>$CONFIG
> + echo "yes"
> + fi
> fi
> rm -f $TMPDIR/strtest.c $TMPDIR/strtest
> }
>
How long has libbsd had an implementation of strlcpy? Would be safer to
have a compile test to verify libbsd has it.
^ permalink raw reply
* Re: [PATCH iproute2] Use libbsd for strlcpy if available
From: Luca Boccassi @ 2018-10-29 15:37 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: stephen
In-Reply-To: <78a4e635-1675-a92d-e7ba-ffc4a642b901@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]
On Mon, 2018-10-29 at 09:27 -0600, David Ahern wrote:
> On 10/29/18 4:46 AM, Luca Boccassi wrote:
> > If libc does not provide strlcpy check for libbsd with pkg-config
> > to
> > avoid relying on inline version.
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > This allows distro maintainers to be able to choose to reduce
> > duplication and let this code be maintained in one place, in the
> > external library.
> >
> > configure | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 744d6282..1dd9ce84 100755
> > --- a/configure
> > +++ b/configure
> > @@ -330,8 +330,16 @@ EOF
> > then
> > echo "no"
> > else
> > - echo 'CFLAGS += -DNEED_STRLCPY' >>$CONFIG
> > - echo "yes"
> > + if ${PKG_CONFIG} libbsd --exists
> > + then
> > + echo 'CFLAGS += -include' `${PKG_CONFIG} libbsd --
> > variable=includedir`'/bsd/string.h' \
> > + `${PKG_CONFIG} libbsd --cflags` >>$CONFIG
> > + echo 'LDLIBS +=' `${PKG_CONFIG} libbsd --libs` >>
> > $CONFIG
> > + echo "no"
> > + else
> > + echo 'CFLAGS += -DNEED_STRLCPY' >>$CONFIG
> > + echo "yes"
> > + fi
> > fi
> > rm -f $TMPDIR/strtest.c $TMPDIR/strtest
> > }
> >
>
> How long has libbsd had an implementation of strlcpy? Would be safer
> to
> have a compile test to verify libbsd has it.
Hi,
0.0 from 10+ years ago has it, so I think we are safe :-)
https://gitlab.freedesktop.org/libbsd/libbsd/blob/0.0/include/bsd/string.h#L34
--
Kind regards,
Luca Boccassi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v2 0/3] Add quirk for reading BD_ADDR from fwnode property
From: Matthias Kaehlcke @ 2018-10-30 0:44 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S . Miller, Loic Poulain
Cc: linux-bluetooth, netdev, linux-kernel, Balakrishna Godavarthi,
Brian Norris, Dmitry Grinberg, Matthias Kaehlcke
On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree (patched in by the bootloader). The btqcomsmd
driver is an example, it can read the address from the DT property
'local-bd-address'.
To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
retrieve the BD address of a device from the DT and adapts the
btqcomsmd and hci_qca drivers to use this quirk.
Matthias Kaehlcke (3):
Bluetooth: Add quirk for reading BD_ADDR from fwnode property
Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990
drivers/bluetooth/btqcomsmd.c | 29 +++-------------------
drivers/bluetooth/hci_qca.c | 1 +
include/net/bluetooth/hci.h | 12 ++++++++++
net/bluetooth/hci_core.c | 45 +++++++++++++++++++++++++++++++++++
net/bluetooth/mgmt.c | 6 +++--
5 files changed, 65 insertions(+), 28 deletions(-)
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply
* [PATCH v2 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property
From: Matthias Kaehlcke @ 2018-10-30 0:44 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S . Miller, Loic Poulain
Cc: linux-bluetooth, netdev, linux-kernel, Balakrishna Godavarthi,
Brian Norris, Dmitry Grinberg, Matthias Kaehlcke
In-Reply-To: <20181030004415.237101-1-mka@chromium.org>
Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
the public Bluetooth address from the firmware node property
'local-bd-address'. If quirk is set and the property does not exist
or is invalid the controller is marked as unconfigured.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
Changes in v2:
- added check for return value of ->setup()
- only read BD_ADDR from the property if it isn't assigned yet. This
is needed to support configuration from user space
- refactored the branch of the new quirk to get rid of 'bd_addr_set'
- added 'Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>' tag
---
include/net/bluetooth/hci.h | 12 ++++++++++
net/bluetooth/hci_core.c | 45 +++++++++++++++++++++++++++++++++++++
net/bluetooth/mgmt.c | 6 +++--
3 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556..fbba43e9bef5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -158,6 +158,18 @@ enum {
*/
HCI_QUIRK_INVALID_BDADDR,
+ /* When this quirk is set, the public Bluetooth address
+ * initially reported by HCI Read BD Address command
+ * is considered invalid. The public BD Address can be
+ * specified in the fwnode property 'local-bd-address'.
+ * If this property does not exist or is invalid controller
+ * configuration is required before this device can be used.
+ *
+ * This quirk can be set before hci_register_dev is called or
+ * during the hdev->setup vendor callback.
+ */
+ HCI_QUIRK_USE_BDADDR_PROPERTY,
+
/* When this quirk is set, the duplicate filtering during
* scanning is based on Bluetooth devices addresses. To allow
* RSSI based updates, restart scanning if needed.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..d4149005a661 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -30,6 +30,7 @@
#include <linux/rfkill.h>
#include <linux/debugfs.h>
#include <linux/crypto.h>
+#include <linux/property.h>
#include <asm/unaligned.h>
#include <net/bluetooth/bluetooth.h>
@@ -1355,6 +1356,36 @@ int hci_inquiry(void __user *arg)
return err;
}
+/**
+ * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
+ * (BD_ADDR) for a HCI device from
+ * a firmware node property.
+ * @hdev: The HCI device
+ *
+ * Search the firmware node for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+static int hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
+{
+ struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
+ bdaddr_t ba;
+ int ret;
+
+ ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+ (u8 *)&ba, sizeof(ba));
+ if (ret < 0)
+ return ret;
+ if (!bacmp(&ba, BDADDR_ANY))
+ return -ENODATA;
+
+ hdev->public_addr = ba;
+
+ return 0;
+}
+
static int hci_dev_do_open(struct hci_dev *hdev)
{
int ret = 0;
@@ -1422,6 +1453,20 @@ static int hci_dev_do_open(struct hci_dev *hdev)
if (hdev->setup)
ret = hdev->setup(hdev);
+ if (ret)
+ goto setup_failed;
+
+ if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
+ if (!bacmp(&hdev->public_addr, BDADDR_ANY))
+ hci_dev_get_bd_addr_from_property(hdev);
+
+ if (!bacmp(&hdev->public_addr, BDADDR_ANY) ||
+ !hdev->set_bdaddr ||
+ hdev->set_bdaddr(hdev, &hdev->public_addr))
+ hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
+ }
+
+setup_failed:
/* The transport driver can set these quirks before
* creating the HCI device or in its setup callback.
*
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ccce954f8146..fae84353d030 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -551,7 +551,8 @@ static bool is_configured(struct hci_dev *hdev)
!hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
return false;
- if (test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) &&
+ if ((test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) ||
+ test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) &&
!bacmp(&hdev->public_addr, BDADDR_ANY))
return false;
@@ -566,7 +567,8 @@ static __le32 get_missing_options(struct hci_dev *hdev)
!hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
options |= MGMT_OPTION_EXTERNAL_CONFIG;
- if (test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) &&
+ if ((test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) ||
+ test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) &&
!bacmp(&hdev->public_addr, BDADDR_ANY))
options |= MGMT_OPTION_PUBLIC_ADDRESS;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* RE: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
From: Keller, Jacob E @ 2018-10-29 16:02 UTC (permalink / raw)
To: Miroslav Lichvar
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
Richard Cochran
In-Reply-To: <20181029133109.GD31668@localhost>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Miroslav Lichvar
> Sent: Monday, October 29, 2018 6:31 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
>
> On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> > > Sent: Friday, October 26, 2018 9:28 AM
> > > To: netdev@vger.kernel.org
> > > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>;
> > > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar
> <mlichvar@redhat.com>
> > > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> > >
> > > Cc: Richard Cochran <richardcochran@gmail.com>
> > > Cc: Jacob Keller <jacob.e.keller@intel.com>
> > > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
>
> > What about replacing gettime64 with:
> >
> > static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
> > {
> > struct ptp_system_timestamp sts
> >
> > ixgbe_ptp_gettimex(ptp, &tst);
> > *ts = sts.phc_ts
> > }
>
> That will work, but it will be slower. With HPET as a clocksource
> there would be few microseconds of an extra (symmetric) delay and the
> applications would have to assume a larger maximum error.
>
Right. My intention for this would be that we'd switch from gettime64 to gettime64x going forward, and provide this as a way to avoid having to duplicate logic in drivers while we're transitioning? Thus, new applications should be using the new call if it's available in the driver.
Hmm.
> I think there could be a flag in ptp_system_timestamp, or a parameter
> of gettimex64(), which would enable/disable reading of the system
> clock.
>
> > Actually, could that even just be provided by the PTP core if gettime64 isn't
> implemented? This way new drivers only have to implement the new interface, and
> userspace will just get the old behavior if they use the old call?
>
> Good idea.
Ideally we can find a way that minimizes the overhead for the old call.
Thanks,
Jake
>
> Thanks,
>
> --
> Miroslav Lichvar
^ permalink raw reply
* Re: [GIT PULL] 9p updates for 4.20
From: Linus Torvalds @ 2018-10-29 16:19 UTC (permalink / raw)
To: asmadeus; +Cc: v9fs-developer, Linux Kernel Mailing List, netdev
In-Reply-To: <20181029012924.GA31433@nautica>
On Sun, Oct 28, 2018 at 6:29 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Highlights this time around are the end of Matthew's work to remove the
> custom 9p request cache and use a slab directly for requests, with some
> extra patches on my end to not degrade performance, but it's a very good
> cleanup.
> Tomas and I fixed a few more syzkaller bugs (refcount is the big one),
> and I had a go at the coverity bugs and at some of the bugzilla reports
> we had open for a while.
Pulled,
Linus
^ permalink raw reply
* Re: [iproute PATCH] utils.h: provide fallback CLOCK_TAI definition
From: Stephen Hemminger @ 2018-10-29 16:57 UTC (permalink / raw)
To: Peter Korsgaard; +Cc: Vinicius Costa Gomes, netdev
In-Reply-To: <20181027153102.32302-1-peter@korsgaard.com>
On Sat, 27 Oct 2018 17:31:02 +0200
Peter Korsgaard <peter@korsgaard.com> wrote:
> q_{etf,taprio}.c uses CLOCK_TAI, which isn't exposed by glibc < 2.21 or
> uClibc, breaking the build. Provide a fallback definition like it is done
> for IPPROTO_MPLS and others.
>
> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> ---
Applied, thanks.
^ permalink raw reply
* Re: [iproute2 PATCH v2] bridge: fix vlan show stats formatting
From: Stephen Hemminger @ 2018-10-29 16:58 UTC (permalink / raw)
To: Tobias Jungel; +Cc: netdev
In-Reply-To: <24a71a731668cd98e415b91136e59559cc21447e.camel@bisdn.de>
On Fri, 26 Oct 2018 23:51:03 +0200
Tobias Jungel <tobias.jungel@bisdn.de> wrote:
> The output of -statistics vlan show was broken previous change for json
> output. This aligns the format to vlan show.
>
> v2: fixed too greedy deletion that caused a -Wmaybe-uninitialized
>
> Signed-off-by: Tobias Jungel <tobias.jungel@gmail.com>
Applied
^ permalink raw reply
* Re: [PATCH] Fix ss Netid column and Local/Peer_Address
From: Stephen Hemminger @ 2018-10-29 17:02 UTC (permalink / raw)
To: Yoann P.; +Cc: netdev
In-Reply-To: <5185850.l7bAsbzJZX@yo-gs>
On Fri, 26 Oct 2018 22:53:32 +0200
"Yoann P." <yoann.p.public@gmail.com> wrote:
> When using ss -Hutn4 or -utn3, Netid and State columns are sometime merged, it
> can be confusing when trying to pipe into awk or column.
> Details (before and after output) are available on this github issue: https://
> github.com/shemminger/iproute2/issues/20
>
> Signed-off-by: YoyPa <yoann.p.public@gmail.com>
> ---
> misc/ss.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index c8970438..5e46cc0e 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -144,9 +144,9 @@ static struct column columns[] = {
> { ALIGN_LEFT, "State", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Recv-Q", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Send-Q", " ", 0, 0, 0 },
> - { ALIGN_RIGHT, "Local Address:", " ", 0, 0, 0 },
> + { ALIGN_RIGHT, "Local_Address:", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Port", "", 0, 0, 0 },
> - { ALIGN_RIGHT, "Peer Address:", " ", 0, 0, 0 },
> + { ALIGN_RIGHT, "Peer_Address:", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Port", "", 0, 0, 0 },
> { ALIGN_LEFT, "", "", 0, 0, 0 },
> };
> @@ -1334,7 +1334,7 @@ static void sock_state_print(struct sockstat *s)
> out("`- %s", sctp_sstate_name[s->state]);
> } else {
> field_set(COL_NETID);
> - out("%s", sock_name);
> + out("%-6s", sock_name);
> field_set(COL_STATE);
> out("%s", sstate_name[s->state]);
> }
Thank for your patch, it does address a bug.
But iproute2 uses kernel coding style and your patch uses spaces instead
of tabs.
WARNING: please, no spaces at the start of a line
#35: FILE: misc/ss.c:147:
+ { ALIGN_RIGHT, "Local_Address:", " ", 0, 0, 0 },$
WARNING: please, no spaces at the start of a line
#38: FILE: misc/ss.c:149:
+ { ALIGN_RIGHT, "Peer_Address:", " ", 0, 0, 0 },$
ERROR: code indent should use tabs where possible
#47: FILE: misc/ss.c:1337:
+ out("%-6s", sock_name);$
WARNING: please, no spaces at the start of a line
#47: FILE: misc/ss.c:1337:
+ out("%-6s", sock_name);$
^ permalink raw reply
* Re: [PATCH iproute2-next 3/3] rdma: Add an option to rename IB device interface
From: David Ahern @ 2018-10-29 17:07 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: netdev, RDMA mailing list, Stephen Hemminger
In-Reply-To: <20181029103046.GQ3974@mtr-leonro.mtl.com>
On 10/29/18 4:30 AM, Leon Romanovsky wrote:
>
> Sorry for being slow in response, I was on vacation.
> ----------
>
> This print is intended to warn about missing "new name" and it is not
> checked by rd_exec_require_dev()
>
> To emphasize it:
>
> diff --git a/rdma/dev.c b/rdma/dev.c
> index 760b7fb3..468ae686 100644
> --- a/rdma/dev.c
> +++ b/rdma/dev.c
> @@ -246,6 +246,7 @@ static int dev_set_name(struct rd *rd)
> uint32_t seq;
>
> if (rd_no_arg(rd)) {
> + pr_err("hello world\n");
> pr_err("Please provide device new name.\n");
> return -EINVAL;
> }
>
> Produces the following output:
> [leonro@server iproute2]$ ./rdma/rdma dev set mlx5_0 name
> hello world
> Please provide device new name.
>
> So how will we progress from here? Should I respin?
ok, thanks for the explanation.
Steve mentioned an update to the man page is needed as well. Please do
that in the respin.
Thanks,
^ permalink raw reply
* Greetings From Mrs.Elodie Antoine,
From: Mrs Elodie Antoine @ 2018-10-29 17:20 UTC (permalink / raw)
Greetings From Mrs.Elodie Antoine,
May be this letter will definitely come to you as a huge surprise, but
I implore you to take the time to go through it carefully as the
decision you make will go off a long way to determine my future and
continued existence. I am Mrs.Elodie Antoine aging widow of 59 years
old suffering from long time illness.
I have some funds I inherited from my late husband Dr. jean baptiste
antoine,The sum of (US$4.5 Million Dollars) please i want you to
withdraw this fund and use it for Charity works. I found your email
address from the internet after honest prayers to the LORD to bring me
a good person that can handle this project and i decided to contact
you, if you may be willing and interested to handle these trust funds
in good faith before anything happens to me,
Please kindly respond for further details.
Thanks and God bless you,
Mrs.Elodie Antoine
^ permalink raw reply
* Re: [PATCH iproute2-next 3/3] rdma: Add an option to rename IB device interface
From: Leon Romanovsky @ 2018-10-29 17:34 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, RDMA mailing list, Stephen Hemminger
In-Reply-To: <b7151495-d8ab-209b-a86e-575fc2f4ed53@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]
On Mon, Oct 29, 2018 at 11:07:06AM -0600, David Ahern wrote:
> On 10/29/18 4:30 AM, Leon Romanovsky wrote:
> >
> > Sorry for being slow in response, I was on vacation.
> > ----------
> >
> > This print is intended to warn about missing "new name" and it is not
> > checked by rd_exec_require_dev()
> >
> > To emphasize it:
> >
> > diff --git a/rdma/dev.c b/rdma/dev.c
> > index 760b7fb3..468ae686 100644
> > --- a/rdma/dev.c
> > +++ b/rdma/dev.c
> > @@ -246,6 +246,7 @@ static int dev_set_name(struct rd *rd)
> > uint32_t seq;
> >
> > if (rd_no_arg(rd)) {
> > + pr_err("hello world\n");
> > pr_err("Please provide device new name.\n");
> > return -EINVAL;
> > }
> >
> > Produces the following output:
> > [leonro@server iproute2]$ ./rdma/rdma dev set mlx5_0 name
> > hello world
> > Please provide device new name.
> >
> > So how will we progress from here? Should I respin?
>
> ok, thanks for the explanation.
>
> Steve mentioned an update to the man page is needed as well. Please do
> that in the respin.
No problem, will do, thanks
>
> Thanks,
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [Patch V4 net 01/11] net: hns3: add error handler for hns3_nic_init_vector_data()
From: Joe Perches @ 2018-10-29 17:44 UTC (permalink / raw)
To: Huazhong Tan, davem, sergei.shtylyov
Cc: netdev, linuxarm, salil.mehta, yisen.zhuang, lipeng321,
linyunsheng
In-Reply-To: <1540821261-55002-2-git-send-email-tanhuazhong@huawei.com>
On Mon, 2018-10-29 at 21:54 +0800, Huazhong Tan wrote:
> When hns3_nic_init_vector_data() fails to map ring to vector,
> it should cancel the netif_napi_add() that has been successfully
> done and then exits.
[]
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
[]
> @@ -2821,7 +2821,7 @@ static int hns3_nic_init_vector_data(struct hns3_nic_priv *priv)
> struct hnae3_handle *h = priv->ae_handle;
> struct hns3_enet_tqp_vector *tqp_vector;
> int ret = 0;
> - u16 i;
> + int i, j;
>
> hns3_nic_set_cpumask(priv);
>
> @@ -2868,13 +2868,19 @@ static int hns3_nic_init_vector_data(struct hns3_nic_priv *priv)
> hns3_free_vector_ring_chain(tqp_vector, &vector_ring_chain);
>
> if (ret)
> - return ret;
> + goto map_ring_fail;
>
> netif_napi_add(priv->netdev, &tqp_vector->napi,
> hns3_nic_common_poll, NAPI_POLL_WEIGHT);
> }
>
> return 0;
> +
> +map_ring_fail:
> + for (j = i - 1; j >= 0; j--)
> + netif_napi_del(&priv->tqp_vector[j].napi);
style trivia:
Error clearing is most commonly done without another variable
by decrementing i in the loop.
^ permalink raw reply
* Re: [PATCH net-next v2 6/6] net/ncsi: Configure multi-package, multi-channel modes with failover
From: Samuel Mendoza-Jonas @ 2018-10-30 3:05 UTC (permalink / raw)
To: Justin.Lee1, netdev; +Cc: davem, linux-kernel, openbmc
In-Reply-To: <93324b3c0a9b4929aa64801b4ed9c25c@AUSX13MPS306.AMER.DELL.COM>
On Fri, 2018-10-26 at 21:48 +0000, Justin.Lee1@Dell.com wrote:
> Hi Samuel,
>
> There is one place that we assume the next available TX channel is under the same package.
> Please see the comment below.
>
> Thanks,
> Justin
>
>
> +/* Change the active Tx channel in a multi-channel setup */
> +int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
> > + struct ncsi_package *np,
> > + struct ncsi_channel *disable,
> > + struct ncsi_channel *enable)
> > +{
> > + struct ncsi_cmd_arg nca;
> > + struct ncsi_channel *nc;
> > + int ret = 0;
> > +
> > + if (!np->multi_channel)
> > + netdev_warn(ndp->ndev.dev,
> > + "NCSI: Trying to update Tx channel in single-channel mode\n");
> > + nca.ndp = ndp;
> > + nca.package = np->id;
>
> If the channel may be on different package, the package ID here may not be correct
> in some cases.
>
> > + nca.req_flags = 0;
> > +
> > + /* Find current channel with Tx enabled */
> > + if (!disable) {
> > + NCSI_FOR_EACH_CHANNEL(np, nc)
> > + if (nc->modes[NCSI_MODE_TX_ENABLE].enable)
> > + disable = nc;
> > + }
> > +
> > + /* Find a suitable channel for Tx */
> > + if (!enable) {
> > + if (np->preferred_channel &&
> > + ncsi_channel_has_link(np->preferred_channel)) {
> > + enable = np->preferred_channel;
> > + } else {
> > + NCSI_FOR_EACH_CHANNEL(np, nc) {
> > + if (!(np->channel_whitelist & 0x1 << nc->id))
> > + continue;
> > + if (nc->state != NCSI_CHANNEL_ACTIVE)
> > + continue;
> > + if (ncsi_channel_has_link(nc)) {
> > + enable = nc;
> > + break;
> > + }
> > + }
>
> When we search, we need to consider the other available channel might be on the
> package.
Good point, I've updated this to allow for enabling Tx on a different
package.
Thanks,
Sam
>
> > + }
> > + }
> > +
> > + if (disable == enable)
> > + return -1;
> > +
> > + if (!enable)
> > + return -1;
> > +
> > + if (disable) {
> > + nca.channel = disable->id;
> > + nca.type = NCSI_PKT_CMD_DCNT;
> > + ret = ncsi_xmit_cmd(&nca);
> > + if (ret)
> > + netdev_err(ndp->ndev.dev,
> > + "Error %d sending DCNT\n",
> > + ret);
> > + }
>
> I remove the cable from ncsi0 and it doesn't failover to ncsi3 as ncsi0 and ncsi3 are not under
> the same package.
>
> cat /sys/kernel/debug/ncsi_protocol/ncsi_device_
> IFIDX IFNAME NAME PID CID RX TX MP MC WP WC PC CS PS LS RU CR NQ HA
> ======================================================================
> 2 eth2 ncsi0 000 000 1 1 1 1 1 1 1 3 0 0 1 1 0 1
> 2 eth2 ncsi1 000 001 0 0 1 1 1 0 0 1 0 1 1 1 0 1
> 2 eth2 ncsi2 001 000 0 0 1 1 1 0 0 1 0 1 1 1 0 1
> 2 eth2 ncsi3 001 001 1 0 1 1 1 1 0 2 1 1 1 1 0 1
> ======================================================================
> MP: Multi-mode Package WP: Whitelist Package
> MC: Multi-mode Channel WC: Whitelist Channel
> PC: Primary Channel CS: Channel State
> PS: Poll Status LS: Link Status
> RU: Running CR: Carrier OK
> NQ: Queue Stopped HA: Hardware Arbitration
^ permalink raw reply
* Re: [PATCH] Fix ss Netid column and Local/Peer_Address
From: Stefano Brivio @ 2018-10-29 18:20 UTC (permalink / raw)
To: Yoann P.; +Cc: netdev, Stephen Hemminger
In-Reply-To: <5185850.l7bAsbzJZX@yo-gs>
Hi Yohann,
On Fri, 26 Oct 2018 22:53:32 +0200
"Yoann P." <yoann.p.public@gmail.com> wrote:
> When using ss -Hutn4 or -utn3, Netid and State columns are sometime merged, it
> can be confusing when trying to pipe into awk or column.
Thanks for fixing this. A few comments though:
> @@ -144,9 +144,9 @@ static struct column columns[] = {
> { ALIGN_LEFT, "State", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Recv-Q", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Send-Q", " ", 0, 0, 0 },
> - { ALIGN_RIGHT, "Local Address:", " ", 0, 0, 0 },
> + { ALIGN_RIGHT, "Local_Address:", " ", 0, 0, 0 },
> { ALIGN_LEFT, "Port", "", 0, 0, 0 },
> - { ALIGN_RIGHT, "Peer Address:", " ", 0, 0, 0 },
> + { ALIGN_RIGHT, "Peer_Address:", " ", 0, 0, 0 },
This is needed only if you pipe the output to column(1), I don't think
it's a bug, because printing the header when you pass the output to
column(1) makes little sense -- one should use -H then.
By the way, why do you use column(1), when ss already prints output in
columns? Any other issue you are working around?
> { ALIGN_LEFT, "Port", "", 0, 0, 0 },
> { ALIGN_LEFT, "", "", 0, 0, 0 },
> };
> @@ -1334,7 +1334,7 @@ static void sock_state_print(struct sockstat *s)
> out("`- %s", sctp_sstate_name[s->state]);
> } else {
> field_set(COL_NETID);
> - out("%s", sock_name);
> + out("%-6s", sock_name);
I could reproduce this issue with a 70-columns terminal and the options
you gave.
Anyway, I don't think this is the right way to fix it: this will waste
one to two columns in case we have three letters for the Netid
specifier, and won't work the day we get six-letters names. In general,
it looks like a bad idea to reintroduce hardcoded width counts.
The actual issue seems to be that in some cases the left delimiter for
the State column is not printed, and I think you should fix that
instead. I'll look into this within a couple of days and give you some
more specific hints in case you still need them by then.
--
Stefano
^ permalink raw reply
* [PATCH v2] kselftests/bpf: use ping6 as the default ipv6 ping binary if it exists
From: Li Zhijian @ 2018-10-30 3:15 UTC (permalink / raw)
To: shuah, netdev, linux-kselftest; +Cc: linux-kernel, ast, daniel, Li Zhijian
ping binary on some distros doesn't support "ping -6" anymore.
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
tools/testing/selftests/bpf/test_skb_cgroup_id.sh | 3 ++-
tools/testing/selftests/bpf/test_sock_addr.sh | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
index 42544a9..a9bc6f8 100755
--- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
+++ b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
@@ -10,7 +10,7 @@ wait_for_ip()
echo -n "Wait for testing link-local IP to become available "
for _i in $(seq ${MAX_PING_TRIES}); do
echo -n "."
- if ping -6 -q -c 1 -W 1 ff02::1%${TEST_IF} >/dev/null 2>&1; then
+ if $PING6 -c 1 -W 1 ff02::1%${TEST_IF} >/dev/null 2>&1; then
echo " OK"
return
fi
@@ -58,5 +58,6 @@ BPF_PROG_OBJ="${DIR}/test_skb_cgroup_id_kern.o"
BPF_PROG_SECTION="cgroup_id_logger"
BPF_PROG_ID=0
PROG="${DIR}/test_skb_cgroup_id_user"
+type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"
main
diff --git a/tools/testing/selftests/bpf/test_sock_addr.sh b/tools/testing/selftests/bpf/test_sock_addr.sh
index 9832a87..3b9fdb8 100755
--- a/tools/testing/selftests/bpf/test_sock_addr.sh
+++ b/tools/testing/selftests/bpf/test_sock_addr.sh
@@ -4,7 +4,8 @@ set -eu
ping_once()
{
- ping -${1} -q -c 1 -W 1 ${2%%/*} >/dev/null 2>&1
+ type ping${1} >/dev/null 2>&1 && PING="ping${1}" || PING="ping -${1}"
+ $PING -q -c 1 -W 1 ${2%%/*} >/dev/null 2>&1
}
wait_for_ip()
--
2.7.4
^ permalink raw reply related
* Re: [RFC net-next v2 1/8] net: sched: register callbacks for indirect tc block binds
From: Jakub Kicinski @ 2018-10-29 18:36 UTC (permalink / raw)
To: Or Gerlitz
Cc: John Hurley, Linux Netdev List, oss-drivers, Jiri Pirko,
Oz Shlomo, Simon Horman, Aviv Heller
In-Reply-To: <CAJ3xEMh-7FUnXRJks11cmvwjfrSsYG_O0pGEew-VmefWFJ5R_g@mail.gmail.com>
On Mon, 29 Oct 2018 17:12:27 +0200, Or Gerlitz wrote:
> >> Maybe it would be better to follow the trusted environment model of the kernel
> >> and not protect the core from driver bugs? If the driver does things right they
> >> will unregister before bailing out and if not, they will have to fix..
>
> > The owner stuff just makes it easier for a driver to track the blocks
> > it has registered for and, in turn, release these when exiting.
> > We could just leave this up to the driver to ensure it properly cleans
> > up after itself.
>
> If it makes the life of the driver easier and doesn't add notable complexity,
> then I think I am good to leave it
>
> > I don't feel that strongly either way.
>
> m2
>
> So lets see if other comment here, if not, we can just leave it, I guess
To be honest big part of why we retained this mechanism was to keep the
per-driver core structure in existence (struct tcf_indr_block_owner).
In my experience it is way easier to move common functionality into the
core if there is a place where core can track offload-related state.
Growing core structures just for offloads is not super advisable, so
unless there is a separate structure core allocates - all state lands in
the drivers. This lesson comes from BPF offload, which started off as
mostly stateless from core's perspective where all operations were muxed
via a single NDO, but that became increasingly awkward to use. We are
gradually moving to a "offload device + ops" form.
I'm not 100% sure the indirect callbacks are a good place for a core
structure, given we didn't seem to need such a thing for normal TC
blocks. So yes, perhaps we should drop that code.
Hope that explanation makes sense.
^ permalink raw reply
* Re: [PATCH] Fix ss Netid column and Local/Peer_Address
From: Stefano Brivio @ 2018-10-29 18:49 UTC (permalink / raw)
To: Yoann P.; +Cc: netdev, Stephen Hemminger
In-Reply-To: <20181029192036.567fc122@redhat.com>
On Mon, 29 Oct 2018 19:20:36 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:
> The actual issue seems to be that in some cases the left delimiter for
> the State column is not printed
Much worse, we always print the left delimiter of the last buffered
column, which is usually empty. My bad.
The issue is not so visible in general as we almost always have spaces
to distribute around, but not if you start going below 70/75 columns.
Can you try this?
diff --git a/misc/ss.c b/misc/ss.c
index f99b6874c228..90986b1dc15f 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1260,7 +1260,7 @@ static void render(void)
while (token) {
/* Print left delimiter only if we already started a line */
if (line_started++)
- printed = printf("%s", current_field->ldelim);
+ printed = printf("%s", f->ldelim);
else
printed = 0;
--
Stefano
^ permalink raw reply related
* Re: [PATCH] hinic: Fix l4_type parameter in hinic_task_set_tunnel_l4
From: David Miller @ 2018-10-30 3:43 UTC (permalink / raw)
To: natechancellor; +Cc: aviad.krawczyk, netdev, linux-kernel, ndesaulniers
In-Reply-To: <20181029105158.6340-1-natechancellor@gmail.com>
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Mon, 29 Oct 2018 03:51:58 -0700
> Clang warns:
>
> drivers/net/ethernet/huawei/hinic/hinic_tx.c:392:34: error: implicit
> conversion from enumeration type 'enum hinic_l4_tunnel_type' to
> different enumeration type 'enum hinic_l4_offload_type'
> [-Werror,-Wenum-conversion]
> hinic_task_set_tunnel_l4(task, TUNNEL_UDP_NO_CSUM,
> ~~~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~
> 1 error generated.
>
> It seems that hinic_task_set_tunnel_l4 was meant to take an enum of type
> hinic_l4_tunnel_type, not hinic_l4_offload_type, given both the name of
> the functions and the values used.
>
> Fixes: cc18a7543d2f ("net-next/hinic: add checksum offload and TSO support")
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] hinic: Fix l4_type parameter in hinic_task_set_tunnel_l4
From: David Miller @ 2018-10-30 3:52 UTC (permalink / raw)
To: ndesaulniers; +Cc: aviad.krawczyk, netdev, linux-kernel, natechancellor
In-Reply-To: <CAKwvOdkg=4BnDKVSuZ0ag4n2G0brL7XS1NL3u9D9mwxaP-6dfA@mail.gmail.com>
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Mon, 29 Oct 2018 10:35:01 -0700
> So I think we'll need you to merge up Nathan's patch. Is there
> anything additional to mark this driver as unmaintained in
> MAINTAINERS?
Just submit a patch stating as such if you cannot reach any of
the listed maintainters.
^ permalink raw reply
* [PATCH] bpf: tcp_bpf_recvmsg should return EAGAIN when nonblocking and no data
From: John Fastabend @ 2018-10-29 19:31 UTC (permalink / raw)
To: daniel, ast; +Cc: john.fastabend, netdev
We return 0 in the case of a nonblocking socket that has no data
available. However, this is incorrect and may confuse applications.
After this patch we do the correct thing and return the error
EAGAIN.
Quoting return codes from recvmsg manpage,
EAGAIN or EWOULDBLOCK
The socket is marked nonblocking and the receive operation would
block, or a receive timeout had been set and the timeout expired
before data was received.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
net/ipv4/tcp_bpf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index b7918d4..3b45fe5 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -145,6 +145,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
ret = err;
goto out;
}
+ copied = -EAGAIN;
}
ret = copied;
out:
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] Fix ss Netid column and Local/Peer_Address
From: Yoann P. @ 2018-10-29 20:06 UTC (permalink / raw)
To: Stefano Brivio; +Cc: netdev, Stephen Hemminger
In-Reply-To: <20181029192036.567fc122@redhat.com>
> Hi Yohann,
>
> On Fri, 26 Oct 2018 22:53:32 +0200
>
> "Yoann P." <yoann.p.public@gmail.com> wrote:
> > When using ss -Hutn4 or -utn3, Netid and State columns are sometime
> > merged, it can be confusing when trying to pipe into awk or column.
>
> Thanks for fixing this. A few comments though:
> > @@ -144,9 +144,9 @@ static struct column columns[] = {
> >
> > { ALIGN_LEFT, "State", " ", 0, 0, 0 },
> > { ALIGN_LEFT, "Recv-Q", " ", 0, 0, 0 },
> > { ALIGN_LEFT, "Send-Q", " ", 0, 0, 0 },
> >
> > - { ALIGN_RIGHT, "Local Address:", " ", 0, 0, 0 },
> > + { ALIGN_RIGHT, "Local_Address:", " ", 0, 0, 0 },
> >
> > { ALIGN_LEFT, "Port", "", 0, 0, 0 },
> >
> > - { ALIGN_RIGHT, "Peer Address:", " ", 0, 0, 0 },
> > + { ALIGN_RIGHT, "Peer_Address:", " ", 0, 0, 0 },
>
> This is needed only if you pipe the output to column(1), I don't think
> it's a bug, because printing the header when you pass the output to
> column(1) makes little sense -- one should use -H then.
I don't really care about this modification, I came across it while making the
github issue example, seemed to be little change, so I dit it.
>
> By the way, why do you use column(1), when ss already prints output in
> columns? Any other issue you are working around?
column can hide columns with "-H -" and is a bit faster than awk to output a
single column according to time, it's the only reason I mentioned it.
>
> > { ALIGN_LEFT, "Port", "", 0, 0, 0 },
> > { ALIGN_LEFT, "", "", 0, 0, 0 },
> >
> > };
> >
> > @@ -1334,7 +1334,7 @@ static void sock_state_print(struct sockstat *s)
> >
> > out("`- %s", sctp_sstate_name[s->state]);
> >
> > } else {
> >
> > field_set(COL_NETID);
> >
> > - out("%s", sock_name);
> > + out("%-6s", sock_name);
>
> I could reproduce this issue with a 70-columns terminal and the options
> you gave.
>
> Anyway, I don't think this is the right way to fix it: this will waste
> one to two columns in case we have three letters for the Netid
> specifier, and won't work the day we get six-letters names. In general,
> it looks like a bad idea to reintroduce hardcoded width counts.
I agree, I just not found the proper way to do it (Not a programmer).
>
> The actual issue seems to be that in some cases the left delimiter for
> the State column is not printed, and I think you should fix that
> instead. I'll look into this within a couple of days and give you some
> more specific hints in case you still need them by then.
^ permalink raw reply
* Re: [PATCH] Fix ss Netid column and Local/Peer_Address
From: Yoann P. @ 2018-10-29 20:07 UTC (permalink / raw)
To: Stefano Brivio; +Cc: netdev, Stephen Hemminger
In-Reply-To: <20181029194938.7df26333@redhat.com>
> On Mon, 29 Oct 2018 19:20:36 +0100
>
> Stefano Brivio <sbrivio@redhat.com> wrote:
> > The actual issue seems to be that in some cases the left delimiter for
> > the State column is not printed
>
> Much worse, we always print the left delimiter of the last buffered
> column, which is usually empty. My bad.
>
> The issue is not so visible in general as we almost always have spaces
> to distribute around, but not if you start going below 70/75 columns.
> Can you try this?
>
> diff --git a/misc/ss.c b/misc/ss.c
> index f99b6874c228..90986b1dc15f 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -1260,7 +1260,7 @@ static void render(void)
> while (token) {
> /* Print left delimiter only if we already started a line */
> if (line_started++)
> - printed = printf("%s", current_field->ldelim);
> + printed = printf("%s", f->ldelim);
> else
> printed = 0;
I can't reproduce the issue with this modification. :).
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox