[PATCH] fs: ext4: mballoc: amend goto to cleanup groupinfo memory correctly
When flexible block groups are enabled on a filesystem, and there are
too many log groups per flexible block group, goto err_freebuddy rather
than err_freesgi within ext4_mb_init_backend. Cleanup code for new_inode
and successive executions of ext4_mb_add_groupinfo in the previous loop
is then correctly run. Fixes memory leak reported by syzbot at:
https://syzkaller.appspot.com/bug?extid=aa12d6106ea4ca1b6aae
Reported-by: syzbot+aa12d6106ea4ca1b6...@syzkaller.appspotmail.com
Signed-off-by: Phillip Potter
---
fs/ext4/mballoc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a02fadf4fc84..d24cb3dc79ff 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2715,7 +2715,7 @@ static int ext4_mb_init_backend(struct super_block *sb)
*/
if (sbi->s_es->s_log_groups_per_flex >= 32) {
ext4_msg(sb, KERN_ERR, "too many log groups per
flexible block group");
- goto err_freesgi;
+ goto err_freebuddy;
}
sbi->s_mb_prefetch = min_t(uint, 1 <<
sbi->s_es->s_log_groups_per_flex,
BLK_MAX_SEGMENT_SIZE >> (sb->s_blocksize_bits - 9));
--
2.30.2
[PATCH] net: geneve: check skb is large enough for IPv4/IPv6 header
Check within geneve_xmit_skb/geneve6_xmit_skb that sk_buff structure is large enough to include IPv4 or IPv6 header, and reject if not. The geneve_xmit_skb portion and overall idea was contributed by Eric Dumazet. Fixes a KMSAN-found uninit-value bug reported by syzbot at: https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f Suggested-by: Eric Dumazet Reported-by: syzbot+2e406a9ac75bb71d4...@syzkaller.appspotmail.com Signed-off-by: Phillip Potter --- drivers/net/geneve.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 5523f069b9a5..adfceb60f7f8 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -891,6 +891,9 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev, __be16 sport; int err; + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) + return -EINVAL; + sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true); rt = geneve_get_v4_rt(skb, dev, gs4, , info, geneve->cfg.info.key.tp_dst, sport); @@ -977,6 +980,9 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, __be16 sport; int err; + if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) + return -EINVAL; + sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true); dst = geneve_get_v6_dst(skb, dev, gs6, , info, geneve->cfg.info.key.tp_dst, sport); -- 2.30.2
Re: [PATCH] net: core: sk_buff: zero-fill skb->data in __alloc_skb function
On Sat, Apr 10, 2021 at 01:00:34PM +0200, Eric Dumazet wrote: > On Sat, Apr 10, 2021 at 12:12 PM Eric Dumazet wrote: > > > > On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter > > wrote: > > > > > > Zero-fill skb->data in __alloc_skb function of net/core/skbuff.c, > > > up to start of struct skb_shared_info bytes. Fixes a KMSAN-found > > > uninit-value bug reported by syzbot at: > > > https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f > > > > > > Reported-by: syzbot+2e406a9ac75bb71d4...@syzkaller.appspotmail.com > > > Signed-off-by: Phillip Potter > > > --- > > > net/core/skbuff.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > > index 785daff48030..9ac26cdb5417 100644 > > > --- a/net/core/skbuff.c > > > +++ b/net/core/skbuff.c > > > @@ -215,6 +215,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t > > > gfp_mask, > > > * to allow max possible filling before reallocation. > > > */ > > > size = SKB_WITH_OVERHEAD(ksize(data)); > > > + memset(data, 0, size); > > > prefetchw(data + size); > > > > > > Certainly not. > > > > There is a difference between kmalloc() and kzalloc() > > > > Here you are basically silencing KMSAN and make it useless. > > > > Please fix the real issue, or stop using KMSAN if it bothers you. > > My understanding of the KMSAN bug (when I released it months ago) was > that it was triggered by some invalid assumptions in geneve_xmit() > > The syzbot repro sends a packet with a very small size (Ethernet > header only) and no IP/IPv6 header > > Fix for ipv4 part (sorry, not much time during week end to test all this) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index > e3b2375ac5eb55f544bbc1f309886cc9be189fd1..0a72779bc74bc50c20c34c05b2c525cca829f33c > 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -892,6 +892,9 @@ static int geneve_xmit_skb(struct sk_buff *skb, > struct net_device *dev, > __be16 sport; > int err; > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr)) > + return -EINVAL; > + > sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true); > rt = geneve_get_v4_rt(skb, dev, gs4, , info, > geneve->cfg.info.key.tp_dst, sport); Dear Eric, Thank you for your help/feedback. I have crafted a patch using your code above and the equivalent check for geneve6_xmit_skb, and this works for me on my local KMSAN build. I am also running it through syzbot to check there as well. I will send out with appropriate attribution assuming all is OK on the testing front. Regards, Phil
[PATCH] net: core: sk_buff: zero-fill skb->data in __alloc_skb function
Zero-fill skb->data in __alloc_skb function of net/core/skbuff.c, up to start of struct skb_shared_info bytes. Fixes a KMSAN-found uninit-value bug reported by syzbot at: https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f Reported-by: syzbot+2e406a9ac75bb71d4...@syzkaller.appspotmail.com Signed-off-by: Phillip Potter --- net/core/skbuff.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 785daff48030..9ac26cdb5417 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -215,6 +215,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, * to allow max possible filling before reallocation. */ size = SKB_WITH_OVERHEAD(ksize(data)); + memset(data, 0, size); prefetchw(data + size); /* -- 2.30.2
[PATCH v3] net: tun: set tun->dev->addr_len during TUNSETLINK processing
When changing type with TUNSETLINK ioctl command, set tun->dev->addr_len
to match the appropriate type, using new tun_get_addr_len utility function
which returns appropriate address length for given type. Fixes a
KMSAN-found uninit-value bug reported by syzbot at:
https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51
Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com
Diagnosed-by: Eric Dumazet
Signed-off-by: Phillip Potter
---
V2: Removed inline specifier from tun_get_addr_len function.
V3: Gave appropriate credit to Eric Dumazet for diagnosing the issue.
---
drivers/net/tun.c | 48 +++
1 file changed, 48 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 978ac0981d16..524a9f771b86 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -69,6 +69,14 @@
#include
#include
#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
#include
#include
@@ -2925,6 +2933,45 @@ static int tun_set_ebpf(struct tun_struct *tun, struct
tun_prog __rcu **prog_p,
return __tun_set_ebpf(tun, prog_p, prog);
}
+/* Return correct value for tun->dev->addr_len based on tun->dev->type. */
+static unsigned char tun_get_addr_len(unsigned short type)
+{
+ switch (type) {
+ case ARPHRD_IP6GRE:
+ case ARPHRD_TUNNEL6:
+ return sizeof(struct in6_addr);
+ case ARPHRD_IPGRE:
+ case ARPHRD_TUNNEL:
+ case ARPHRD_SIT:
+ return 4;
+ case ARPHRD_ETHER:
+ return ETH_ALEN;
+ case ARPHRD_IEEE802154:
+ case ARPHRD_IEEE802154_MONITOR:
+ return IEEE802154_EXTENDED_ADDR_LEN;
+ case ARPHRD_PHONET_PIPE:
+ case ARPHRD_PPP:
+ case ARPHRD_NONE:
+ return 0;
+ case ARPHRD_6LOWPAN:
+ return EUI64_ADDR_LEN;
+ case ARPHRD_FDDI:
+ return FDDI_K_ALEN;
+ case ARPHRD_HIPPI:
+ return HIPPI_ALEN;
+ case ARPHRD_IEEE802:
+ return FC_ALEN;
+ case ARPHRD_ROSE:
+ return ROSE_ADDR_LEN;
+ case ARPHRD_NETROM:
+ return AX25_ADDR_LEN;
+ case ARPHRD_LOCALTLK:
+ return LTALK_ALEN;
+ default:
+ return 0;
+ }
+}
+
static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
unsigned long arg, int ifreq_len)
{
@@ -3088,6 +3135,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
break;
}
tun->dev->type = (int) arg;
+ tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
--
2.30.2
Re: [PATCH] net: tun: set tun->dev->addr_len during TUNSETLINK processing
On Tue, Apr 06, 2021 at 07:26:29PM +0200, Eric Dumazet wrote: > > > On 4/5/21 1:35 PM, Phillip Potter wrote: > > When changing type with TUNSETLINK ioctl command, set tun->dev->addr_len > > to match the appropriate type, using new tun_get_addr_len utility function > > which returns appropriate address length for given type. Fixes a > > KMSAN-found uninit-value bug reported by syzbot at: > > https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51 > > > > Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com > > Signed-off-by: Phillip Potter > > --- > > Please give credits to people who helped. > > You could have : > > Suggested-by: Eric Dumazet > > Or > Diagnosed-by: Eric Dumazet > > Or at least CCed me :/ > Dear Eric, Please accept my apology for this oversight. It certainly wasn't intentional on my part, and entirely down to inexperience. I will send the patch again with Diagnosed-by. Regards, Phil
[PATCH] staging: rtl8723bs: remove unused variable from rtw_os_recv_indicate_pkt
Remove unused 'ret' variable from rtw_os_recv_indicate_pkt function in
drivers/staging/rtl8723bs/os_dep/recv_linux.c as nothing is actually
done with it. A function return val is conditionally stored to it under
certain circumstances, but nothing is done with it after. Fixes a
warning from kernel test robot.
Reported-by: kernel test robot
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/os_dep/recv_linux.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/recv_linux.c
b/drivers/staging/rtl8723bs/os_dep/recv_linux.c
index fbdbcd04d44a..f6a9482be8e3 100644
--- a/drivers/staging/rtl8723bs/os_dep/recv_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/recv_linux.c
@@ -98,7 +98,6 @@ struct sk_buff *rtw_os_alloc_msdu_pkt(union recv_frame
*prframe, u16 nSubframe_L
void rtw_os_recv_indicate_pkt(struct adapter *padapter, struct sk_buff *pkt,
struct rx_pkt_attrib *pattrib)
{
struct mlme_priv *pmlmepriv = >mlmepriv;
- int ret;
/* Indicate the packets to upper layer */
if (pkt) {
@@ -140,7 +139,7 @@ void rtw_os_recv_indicate_pkt(struct adapter *padapter,
struct sk_buff *pkt, str
pkt->ip_summed = CHECKSUM_NONE;
- ret = rtw_netif_rx(padapter->pnetdev, pkt);
+ rtw_netif_rx(padapter->pnetdev, pkt);
}
}
--
2.30.2
[PATCH v2] net: tun: set tun->dev->addr_len during TUNSETLINK processing
When changing type with TUNSETLINK ioctl command, set tun->dev->addr_len
to match the appropriate type, using new tun_get_addr_len utility function
which returns appropriate address length for given type. Fixes a
KMSAN-found uninit-value bug reported by syzbot at:
https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51
Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com
Signed-off-by: Phillip Potter
---
V2: Removed inline specifier from tun_get_addr_len function.
---
drivers/net/tun.c | 48 +++
1 file changed, 48 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 978ac0981d16..524a9f771b86 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -69,6 +69,14 @@
#include
#include
#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
#include
#include
@@ -2925,6 +2933,45 @@ static int tun_set_ebpf(struct tun_struct *tun, struct
tun_prog __rcu **prog_p,
return __tun_set_ebpf(tun, prog_p, prog);
}
+/* Return correct value for tun->dev->addr_len based on tun->dev->type. */
+static unsigned char tun_get_addr_len(unsigned short type)
+{
+ switch (type) {
+ case ARPHRD_IP6GRE:
+ case ARPHRD_TUNNEL6:
+ return sizeof(struct in6_addr);
+ case ARPHRD_IPGRE:
+ case ARPHRD_TUNNEL:
+ case ARPHRD_SIT:
+ return 4;
+ case ARPHRD_ETHER:
+ return ETH_ALEN;
+ case ARPHRD_IEEE802154:
+ case ARPHRD_IEEE802154_MONITOR:
+ return IEEE802154_EXTENDED_ADDR_LEN;
+ case ARPHRD_PHONET_PIPE:
+ case ARPHRD_PPP:
+ case ARPHRD_NONE:
+ return 0;
+ case ARPHRD_6LOWPAN:
+ return EUI64_ADDR_LEN;
+ case ARPHRD_FDDI:
+ return FDDI_K_ALEN;
+ case ARPHRD_HIPPI:
+ return HIPPI_ALEN;
+ case ARPHRD_IEEE802:
+ return FC_ALEN;
+ case ARPHRD_ROSE:
+ return ROSE_ADDR_LEN;
+ case ARPHRD_NETROM:
+ return AX25_ADDR_LEN;
+ case ARPHRD_LOCALTLK:
+ return LTALK_ALEN;
+ default:
+ return 0;
+ }
+}
+
static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
unsigned long arg, int ifreq_len)
{
@@ -3088,6 +3135,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
break;
}
tun->dev->type = (int) arg;
+ tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
--
2.30.2
Re: [PATCH] net: tun: set tun->dev->addr_len during TUNSETLINK processing
On Mon, Apr 05, 2021 at 02:59:21PM -0700, David Miller wrote:
> From: Phillip Potter
> Date: Mon, 5 Apr 2021 12:35:55 +0100
>
> > When changing type with TUNSETLINK ioctl command, set tun->dev->addr_len
> > to match the appropriate type, using new tun_get_addr_len utility function
> > which returns appropriate address length for given type. Fixes a
> > KMSAN-found uninit-value bug reported by syzbot at:
> > https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51
> >
> > Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com
> > Signed-off-by: Phillip Potter
> > ---
> > drivers/net/tun.c | 48 +++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 978ac0981d16..56c26339ee3b 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -69,6 +69,14 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> >
> > #include
> > #include
> > @@ -2925,6 +2933,45 @@ static int tun_set_ebpf(struct tun_struct *tun,
> > struct tun_prog __rcu **prog_p,
> > return __tun_set_ebpf(tun, prog_p, prog);
> > }
> >
> > +/* Return correct value for tun->dev->addr_len based on tun->dev->type. */
> > +static inline unsigned char tun_get_addr_len(unsigned short type)
> > +{
>
> Please do not use inline in foo.c files, let the compiler decide.
>
> Thanks.
Dear David,
Thank you for the feedback, I will resend.
Regards,
Phil
[PATCH] net: tun: set tun->dev->addr_len during TUNSETLINK processing
When changing type with TUNSETLINK ioctl command, set tun->dev->addr_len
to match the appropriate type, using new tun_get_addr_len utility function
which returns appropriate address length for given type. Fixes a
KMSAN-found uninit-value bug reported by syzbot at:
https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51
Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com
Signed-off-by: Phillip Potter
---
drivers/net/tun.c | 48 +++
1 file changed, 48 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 978ac0981d16..56c26339ee3b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -69,6 +69,14 @@
#include
#include
#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
#include
#include
@@ -2925,6 +2933,45 @@ static int tun_set_ebpf(struct tun_struct *tun, struct
tun_prog __rcu **prog_p,
return __tun_set_ebpf(tun, prog_p, prog);
}
+/* Return correct value for tun->dev->addr_len based on tun->dev->type. */
+static inline unsigned char tun_get_addr_len(unsigned short type)
+{
+ switch (type) {
+ case ARPHRD_IP6GRE:
+ case ARPHRD_TUNNEL6:
+ return sizeof(struct in6_addr);
+ case ARPHRD_IPGRE:
+ case ARPHRD_TUNNEL:
+ case ARPHRD_SIT:
+ return 4;
+ case ARPHRD_ETHER:
+ return ETH_ALEN;
+ case ARPHRD_IEEE802154:
+ case ARPHRD_IEEE802154_MONITOR:
+ return IEEE802154_EXTENDED_ADDR_LEN;
+ case ARPHRD_PHONET_PIPE:
+ case ARPHRD_PPP:
+ case ARPHRD_NONE:
+ return 0;
+ case ARPHRD_6LOWPAN:
+ return EUI64_ADDR_LEN;
+ case ARPHRD_FDDI:
+ return FDDI_K_ALEN;
+ case ARPHRD_HIPPI:
+ return HIPPI_ALEN;
+ case ARPHRD_IEEE802:
+ return FC_ALEN;
+ case ARPHRD_ROSE:
+ return ROSE_ADDR_LEN;
+ case ARPHRD_NETROM:
+ return AX25_ADDR_LEN;
+ case ARPHRD_LOCALTLK:
+ return LTALK_ALEN;
+ default:
+ return 0;
+ }
+}
+
static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
unsigned long arg, int ifreq_len)
{
@@ -3088,6 +3135,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
break;
}
tun->dev->type = (int) arg;
+ tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
--
2.30.2
Re: [PATCH] net: initialize local variables in net/ipv6/mcast.c and net/ipv4/igmp.c
On Fri, Apr 02, 2021 at 11:12:36PM +0200, Eric Dumazet wrote: > > > On 4/2/21 10:53 PM, Eric Dumazet wrote: > > > > > > On 4/2/21 8:10 PM, Phillip Potter wrote: > >> On Fri, Apr 02, 2021 at 07:49:44PM +0200, Eric Dumazet wrote: > >>> > >>> > >>> On 4/2/21 7:36 PM, Phillip Potter wrote: > >>>> Use memset to initialize two local buffers in net/ipv6/mcast.c, > >>>> and another in net/ipv4/igmp.c. Fixes a KMSAN found uninit-value > >>>> bug reported by syzbot at: > >>>> https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51 > >>> > >>> > >>> According to this link, the bug no longer triggers. > >>> > >>> Please explain why you think it is still there. > >>> > >> > >> Dear Eric, > >> > >> It definitely still triggers, tested it on the master branch of > >> https://github.com/google/kmsan last night. The patch which fixes the > >> crash on that page is the same patch I've sent in. > > > > Please send the full report (stack trace) > > I think your patch just silences the real problem. > > The issue at hand is that TUNSETLINK changes dev->type without making > any change to dev->addr_len > > This is the real issue. > > If you care about this, please fix tun driver. > Dear Eric, Thank you for pointing me in the right direction. I will do as you suggest. Regards, Phil
Re: [PATCH] net: initialize local variables in net/ipv6/mcast.c and net/ipv4/igmp.c
On Fri, Apr 02, 2021 at 07:49:44PM +0200, Eric Dumazet wrote: > > > On 4/2/21 7:36 PM, Phillip Potter wrote: > > Use memset to initialize two local buffers in net/ipv6/mcast.c, > > and another in net/ipv4/igmp.c. Fixes a KMSAN found uninit-value > > bug reported by syzbot at: > > https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51 > > > According to this link, the bug no longer triggers. > > Please explain why you think it is still there. > Dear Eric, It definitely still triggers, tested it on the master branch of https://github.com/google/kmsan last night. The patch which fixes the crash on that page is the same patch I've sent in. Regards, Phil
[PATCH] net: initialize local variables in net/ipv6/mcast.c and net/ipv4/igmp.c
Use memset to initialize two local buffers in net/ipv6/mcast.c, and another in net/ipv4/igmp.c. Fixes a KMSAN found uninit-value bug reported by syzbot at: https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51 Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com Signed-off-by: Phillip Potter --- net/ipv4/igmp.c | 2 ++ net/ipv6/mcast.c | 4 2 files changed, 6 insertions(+) diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c index 7b272bbed2b4..bc8e358a9a2a 100644 --- a/net/ipv4/igmp.c +++ b/net/ipv4/igmp.c @@ -1131,6 +1131,8 @@ static void ip_mc_filter_add(struct in_device *in_dev, __be32 addr) char buf[MAX_ADDR_LEN]; struct net_device *dev = in_dev->dev; + memset(buf, 0, sizeof(buf)); + /* Checking for IFF_MULTICAST here is WRONG-WRONG-WRONG. We will get multicast token leakage, when IFF_MULTICAST is changed. This check should be done in ndo_set_rx_mode diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index 6c8604390266..ad90dc28f318 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -658,6 +658,8 @@ static void igmp6_group_added(struct ifmcaddr6 *mc) struct net_device *dev = mc->idev->dev; char buf[MAX_ADDR_LEN]; + memset(buf, 0, sizeof(buf)); + if (IPV6_ADDR_MC_SCOPE(>mca_addr) < IPV6_ADDR_SCOPE_LINKLOCAL) return; @@ -694,6 +696,8 @@ static void igmp6_group_dropped(struct ifmcaddr6 *mc) struct net_device *dev = mc->idev->dev; char buf[MAX_ADDR_LEN]; + memset(buf, 0, sizeof(buf)); + if (IPV6_ADDR_MC_SCOPE(>mca_addr) < IPV6_ADDR_SCOPE_LINKLOCAL) return; -- 2.30.2
Re: [PATCH] zero-fill colormap in drivers/video/fbdev/core/fbcmap.c
On Thu, Apr 01, 2021 at 11:55:50AM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 1, 2021 at 12:09 AM Phillip Potter wrote:
> > Use kzalloc() rather than kmalloc() for the dynamically allocated parts
> > of the colormap in fb_alloc_cmap_gfp, to prevent a leak of random kernel
> > data to userspace under certain circumstances.
> >
> > Fixes a KMSAN-found infoleak bug reported by syzbot at:
> > https://syzkaller.appspot.com/bug?id=741578659feabd108ad9e06696f0c1f2e69c4b6e
> >
> > Reported-by: syzbot+47fa9c9c648b76530...@syzkaller.appspotmail.com
> > Signed-off-by: Phillip Potter
>
> Reviewed-by: Geert Uytterhoeven
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Dear Geert
Thank you for your review :-)
Regards,
Phil
[PATCH 1/2] net: initialize local variables in net/ipv6/mcast.c and net/ipv4/igmp.c
Use memset to initialize two local buffers in net/ipv6/mcast.c, and another in net/ipv4/igmp.c. Fixes a KMSAN found uninit-value bug reported by syzbot at: https://syzkaller.appspot.com/bug?id=0766d38c656abeace60621896d705743aeefed51 Reported-by: syzbot+001516d86dbe88862...@syzkaller.appspotmail.com Signed-off-by: Phillip Potter --- net/ipv4/igmp.c | 2 ++ net/ipv6/mcast.c | 4 2 files changed, 6 insertions(+) diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c index 7b272bbed2b4..bc8e358a9a2a 100644 --- a/net/ipv4/igmp.c +++ b/net/ipv4/igmp.c @@ -1131,6 +1131,8 @@ static void ip_mc_filter_add(struct in_device *in_dev, __be32 addr) char buf[MAX_ADDR_LEN]; struct net_device *dev = in_dev->dev; + memset(buf, 0, sizeof(buf)); + /* Checking for IFF_MULTICAST here is WRONG-WRONG-WRONG. We will get multicast token leakage, when IFF_MULTICAST is changed. This check should be done in ndo_set_rx_mode diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index 6c8604390266..ad90dc28f318 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -658,6 +658,8 @@ static void igmp6_group_added(struct ifmcaddr6 *mc) struct net_device *dev = mc->idev->dev; char buf[MAX_ADDR_LEN]; + memset(buf, 0, sizeof(buf)); + if (IPV6_ADDR_MC_SCOPE(>mca_addr) < IPV6_ADDR_SCOPE_LINKLOCAL) return; @@ -694,6 +696,8 @@ static void igmp6_group_dropped(struct ifmcaddr6 *mc) struct net_device *dev = mc->idev->dev; char buf[MAX_ADDR_LEN]; + memset(buf, 0, sizeof(buf)); + if (IPV6_ADDR_MC_SCOPE(>mca_addr) < IPV6_ADDR_SCOPE_LINKLOCAL) return; -- 2.30.2
[PATCH] net: usb: ax88179_178a: initialize local variables before use
Use memset to initialize local array in drivers/net/usb/ax88179_178a.c, and
also set a local u16 and u32 variable to 0. Fixes a KMSAN found uninit-value bug
reported by syzbot at:
https://syzkaller.appspot.com/bug?id=00371c73c72f72487c1d0bfe0cc9d00de339d5aa
Reported-by: syzbot+4993e4a0e237f1b53...@syzkaller.appspotmail.com
Signed-off-by: Phillip Potter
---
drivers/net/usb/ax88179_178a.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index d650b39b6e5d..c1316718304d 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -296,12 +296,12 @@ static int ax88179_read_cmd(struct usbnet *dev, u8 cmd,
u16 value, u16 index,
int ret;
if (2 == size) {
- u16 buf;
+ u16 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, , 0);
le16_to_cpus();
*((u16 *)data) = buf;
} else if (4 == size) {
- u32 buf;
+ u32 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, , 0);
le32_to_cpus();
*((u32 *)data) = buf;
@@ -1296,6 +1296,8 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
{
u8 mac[ETH_ALEN];
+ memset(mac, 0, sizeof(mac));
+
/* Maybe the boot loader passed the MAC address via device tree */
if (!eth_platform_get_mac_address(>udev->dev, mac)) {
netif_dbg(dev, ifup, dev->net,
--
2.30.2
[PATCH] zero-fill colormap in drivers/video/fbdev/core/fbcmap.c
Use kzalloc() rather than kmalloc() for the dynamically allocated parts
of the colormap in fb_alloc_cmap_gfp, to prevent a leak of random kernel
data to userspace under certain circumstances.
Fixes a KMSAN-found infoleak bug reported by syzbot at:
https://syzkaller.appspot.com/bug?id=741578659feabd108ad9e06696f0c1f2e69c4b6e
Reported-by: syzbot+47fa9c9c648b76530...@syzkaller.appspotmail.com
Signed-off-by: Phillip Potter
---
drivers/video/fbdev/core/fbcmap.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbcmap.c
b/drivers/video/fbdev/core/fbcmap.c
index 757d5c3f620b..ff09e57f3c38 100644
--- a/drivers/video/fbdev/core/fbcmap.c
+++ b/drivers/video/fbdev/core/fbcmap.c
@@ -101,17 +101,17 @@ int fb_alloc_cmap_gfp(struct fb_cmap *cmap, int len, int
transp, gfp_t flags)
if (!len)
return 0;
- cmap->red = kmalloc(size, flags);
+ cmap->red = kzalloc(size, flags);
if (!cmap->red)
goto fail;
- cmap->green = kmalloc(size, flags);
+ cmap->green = kzalloc(size, flags);
if (!cmap->green)
goto fail;
- cmap->blue = kmalloc(size, flags);
+ cmap->blue = kzalloc(size, flags);
if (!cmap->blue)
goto fail;
if (transp) {
- cmap->transp = kmalloc(size, flags);
+ cmap->transp = kzalloc(size, flags);
if (!cmap->transp)
goto fail;
} else {
--
2.30.2
Re: [PATCH 0/6] staging: rtl8723bs: remove DBG_COUNTER
On Tue, Feb 16, 2021 at 12:24:38PM +0300, Dan Carpenter wrote: > Looks good. > > Reviewed-by: Dan Carpenter > > regards, > dan carpenter > Thank you Dan. Regards, Phil
[PATCH 2/6] staging: rtl8723bs: remove DBG_COUNTER calls from os_dep/xmit_linux.c
Remove all DBG_COUNTER macro calls from os_dep/xmit_linux.c, as the
corresponding variables are only ever written to and not used. This
makes the code cleaner, and is necessary prior to removing the
DBG_COUNTER definition itself.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 17 ++---
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
index b060a6a2df34..1c23fbe58881 100644
--- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
@@ -139,8 +139,6 @@ static int rtw_mlcst2unicst(struct adapter *padapter,
struct sk_buff *skb)
int i;
s32 res;
- DBG_COUNTER(padapter->tx_logs.os_tx_m2u);
-
spin_lock_bh(>asoc_list_lock);
phead = >asoc_list;
plist = get_next(phead);
@@ -160,20 +158,14 @@ static int rtw_mlcst2unicst(struct adapter *padapter,
struct sk_buff *skb)
for (i = 0; i < chk_alive_num; i++) {
psta = rtw_get_stainfo_by_offset(pstapriv, chk_alive_list[i]);
- if (!(psta->state & _FW_LINKED)) {
-
DBG_COUNTER(padapter->tx_logs.os_tx_m2u_ignore_fw_linked);
+ if (!(psta->state & _FW_LINKED))
continue;
- }
/* avoid come from STA1 and send back STA1 */
if (!memcmp(psta->hwaddr, >data[6], 6) ||
!memcmp(psta->hwaddr, null_addr, 6) ||
- !memcmp(psta->hwaddr, bc_addr, 6)) {
- DBG_COUNTER(padapter->tx_logs.os_tx_m2u_ignore_self);
+ !memcmp(psta->hwaddr, bc_addr, 6))
continue;
- }
-
- DBG_COUNTER(padapter->tx_logs.os_tx_m2u_entry);
newskb = rtw_skb_copy(skb);
@@ -181,13 +173,11 @@ static int rtw_mlcst2unicst(struct adapter *padapter,
struct sk_buff *skb)
memcpy(newskb->data, psta->hwaddr, 6);
res = rtw_xmit(padapter, );
if (res < 0) {
-
DBG_COUNTER(padapter->tx_logs.os_tx_m2u_entry_err_xmit);
DBG_871X("%s()-%d: rtw_xmit() return error!\n",
__func__, __LINE__);
pxmitpriv->tx_drop++;
dev_kfree_skb_any(newskb);
}
} else {
- DBG_COUNTER(padapter->tx_logs.os_tx_m2u_entry_err_skb);
DBG_871X("%s-%d: rtw_skb_copy() failed!\n", __func__,
__LINE__);
pxmitpriv->tx_drop++;
/* dev_kfree_skb_any(skb); */
@@ -206,11 +196,9 @@ int _rtw_xmit_entry(_pkt *pkt, _nic_hdl pnetdev)
struct mlme_priv *pmlmepriv = >mlmepriv;
s32 res = 0;
- DBG_COUNTER(padapter->tx_logs.os_tx);
RT_TRACE(_module_rtl871x_mlme_c_, _drv_info_, ("+xmit_enry\n"));
if (rtw_if_up(padapter) == false) {
- DBG_COUNTER(padapter->tx_logs.os_tx_err_up);
RT_TRACE(_module_xmit_osdep_c_, _drv_err_, ("rtw_xmit_entry:
rtw_if_up fail\n"));
#ifdef DBG_TX_DROP_FRAME
DBG_871X("DBG_TX_DROP_FRAME %s if_up fail\n", __func__);
@@ -236,7 +224,6 @@ int _rtw_xmit_entry(_pkt *pkt, _nic_hdl pnetdev)
} else {
/* DBG_871X("Stop M2U(%d, %d)! ",
pxmitpriv->free_xmitframe_cnt, pxmitpriv->free_xmitbuf_cnt); */
/* DBG_871X("!m2u); */
- DBG_COUNTER(padapter->tx_logs.os_tx_m2u_stop);
}
}
--
2.29.2
[PATCH 3/6] staging: rtl8723bs: remove DBG_COUNTER calls from core/rtw_xmit.c
Remove all DBG_COUNTER macro calls from core/rtw_xmit.c, as the
corresponding variables are only ever written to and not used. This
makes the code cleaner, and is necessary prior to removing the
DBG_COUNTER definition itself.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/core/rtw_xmit.c | 44 ++-
1 file changed, 3 insertions(+), 41 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c
b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 41632fa0b3c8..19aecbabbc4d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -653,8 +653,6 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
struct qos_priv *pqospriv = >qospriv;
sint res = _SUCCESS;
- DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib);
-
_rtw_open_pktfile(pkt, );
_rtw_pktfile_read(, (u8 *), ETH_HLEN);
@@ -667,17 +665,12 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
(check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE) == true)) {
memcpy(pattrib->ra, pattrib->dst, ETH_ALEN);
memcpy(pattrib->ta, pattrib->src, ETH_ALEN);
- DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_adhoc);
} else if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
memcpy(pattrib->ra, get_bssid(pmlmepriv), ETH_ALEN);
memcpy(pattrib->ta, pattrib->src, ETH_ALEN);
- DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_sta);
} else if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
memcpy(pattrib->ra, pattrib->dst, ETH_ALEN);
memcpy(pattrib->ta, get_bssid(pmlmepriv), ETH_ALEN);
- DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_ap);
- } else {
- DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_unknown);
}
pattrib->pktlen = pktfile.pkt_len;
@@ -699,7 +692,6 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
/* 67 : UDP BOOTP server */
RT_TRACE(_module_rtl871x_xmit_c_,
_drv_err_, ("==update_attrib: get DHCP Packet\n"));
pattrib->dhcp_pkt = 1;
-
DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_dhcp);
}
}
}
@@ -709,10 +701,8 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
struct iphdr *piphdr = (struct iphdr *)tmp;
pattrib->icmp_pkt = 0;
- if (piphdr->protocol == 0x1) { /* protocol type in ip
header 0x1 is ICMP */
+ if (piphdr->protocol == 0x1) /* protocol type in ip
header 0x1 is ICMP */
pattrib->icmp_pkt = 1;
-
DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_icmp);
- }
}
} else if (0x888e == pattrib->ether_type) {
DBG_871X_LEVEL(_drv_always_, "send eapol packet\n");
@@ -724,10 +714,8 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
/* If EAPOL , ARP , OR DHCP packet, driver must be in active mode. */
if (pattrib->icmp_pkt == 1)
rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_LEAVE, 1);
- else if (pattrib->dhcp_pkt == 1) {
- DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_active);
+ else if (pattrib->dhcp_pkt == 1)
rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_SPECIAL_PACKET, 1);
- }
bmcast = IS_MCAST(pattrib->ra);
@@ -737,7 +725,6 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
} else {
psta = rtw_get_stainfo(pstapriv, pattrib->ra);
if (!psta) { /* if we cannot get psta => drop the pkt */
-
DBG_COUNTER(padapter->tx_logs.core_tx_upd_attrib_err_ucast_sta);
RT_TRACE(_module_rtl871x_xmit_c_, _drv_alert_,
("\nupdate_attrib => get sta_info fail, ra:%pM\n", MAC_ARG(pattrib->ra)));
#ifdef DBG_TX_DROP_FRAME
DBG_871X("DBG_TX_DROP_FRAME %s get sta_info fail,
ra:%pM\n", __func__, MAC_ARG(pattrib->ra));
@@ -745,7 +732,6 @@ static s32 update_attrib(struct adapter *padapter, _pkt
*pkt, struct pkt_attrib
res = _FAIL;
goto exit;
} else if ((check_fwstate(pmlmepriv, WIFI_AP_STATE) == true) &&
(!(psta->state & _FW_LINKED))) {
-
DBG_C
[PATCH 1/6] staging: rtl8723bs: remove DBG_COUNTER calls from os_dep/recv_linux.c
Remove all DBG_COUNTER macro calls from os_dep/recv_linux.c, as the
corresponding variables are only ever written to and not used. This
makes the code cleaner, and is necessary prior to removing the
DBG_COUNTER definition itself.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/os_dep/recv_linux.c | 15 ++-
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/recv_linux.c
b/drivers/staging/rtl8723bs/os_dep/recv_linux.c
index ac35277fbacd..f52802f24466 100644
--- a/drivers/staging/rtl8723bs/os_dep/recv_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/recv_linux.c
@@ -124,18 +124,14 @@ void rtw_os_recv_indicate_pkt(struct adapter *padapter,
_pkt *pkt, struct rx_pkt
_rtw_xmit_entry(pkt, pnetdev);
- if (bmcast && pskb2) {
+ if (bmcast && pskb2)
pkt = pskb2;
-
DBG_COUNTER(padapter->rx_logs.os_indicate_ap_mcast);
- } else {
-
DBG_COUNTER(padapter->rx_logs.os_indicate_ap_forward);
+ else
return;
- }
}
} else {
/* to APself */
/* DBG_871X("to APSelf\n"); */
-
DBG_COUNTER(padapter->rx_logs.os_indicate_ap_self);
}
}
@@ -153,10 +149,6 @@ void rtw_os_recv_indicate_pkt(struct adapter *padapter,
_pkt *pkt, struct rx_pkt
#endif /* CONFIG_TCP_CSUM_OFFLOAD_RX */
ret = rtw_netif_rx(padapter->pnetdev, pkt);
- if (ret == NET_RX_SUCCESS)
- DBG_COUNTER(padapter->rx_logs.os_netif_ok);
- else
- DBG_COUNTER(padapter->rx_logs.os_netif_err);
}
}
@@ -246,8 +238,6 @@ int rtw_recv_indicatepkt(struct adapter *padapter, union
recv_frame *precv_frame
_pkt *skb;
struct rx_pkt_attrib *pattrib = _frame->u.hdr.attrib;
- DBG_COUNTER(padapter->rx_logs.os_indicate);
-
precvpriv = &(padapter->recvpriv);
pfree_recv_queue = &(precvpriv->free_recv_queue);
@@ -293,7 +283,6 @@ int rtw_recv_indicatepkt(struct adapter *padapter, union
recv_frame *precv_frame
/* enqueue back to free_recv_queue */
rtw_free_recvframe(precv_frame, pfree_recv_queue);
- DBG_COUNTER(padapter->rx_logs.os_indicate_err);
return _FAIL;
}
--
2.29.2
[PATCH 4/6] staging: rtl8723bs: remove DBG_COUNTER calls from core/rtw_recv.c
Remove all DBG_COUNTER macro calls from core/rtw_recv.c, as the
corresponding variables are only ever written to and not used. This
makes the code cleaner, and is necessary prior to removing the
DBG_COUNTER definition itself.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/core/rtw_recv.c | 44 ++-
1 file changed, 3 insertions(+), 41 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c
b/drivers/staging/rtl8723bs/core/rtw_recv.c
index 3c9dbd7443d9..f35a134bb75f 100644
--- a/drivers/staging/rtl8723bs/core/rtw_recv.c
+++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
@@ -445,8 +445,6 @@ union recv_frame *decryptor(struct adapter *padapter, union
recv_frame *precv_fr
union recv_frame *return_packet = precv_frame;
u32 res = _SUCCESS;
- DBG_COUNTER(padapter->rx_logs.core_rx_post_decrypt);
-
RT_TRACE(_module_rtl871x_recv_c_, _drv_info_, ("prxstat->decrypted =%x
prxattrib->encrypt = 0x%03x\n", prxattrib->bdecrypted, prxattrib->encrypt));
if (prxattrib->encrypt > 0) {
@@ -485,15 +483,12 @@ union recv_frame *decryptor(struct adapter *padapter,
union recv_frame *precv_fr
switch (prxattrib->encrypt) {
case _WEP40_:
case _WEP104_:
- DBG_COUNTER(padapter->rx_logs.core_rx_post_decrypt_wep);
rtw_wep_decrypt(padapter, (u8 *)precv_frame);
break;
case _TKIP_:
-
DBG_COUNTER(padapter->rx_logs.core_rx_post_decrypt_tkip);
res = rtw_tkip_decrypt(padapter, (u8 *)precv_frame);
break;
case _AES_:
- DBG_COUNTER(padapter->rx_logs.core_rx_post_decrypt_aes);
res = rtw_aes_decrypt(padapter, (u8 *)precv_frame);
break;
default:
@@ -502,8 +497,6 @@ union recv_frame *decryptor(struct adapter *padapter, union
recv_frame *precv_fr
} else if (prxattrib->bdecrypted == 1 && prxattrib->encrypt > 0 &&
(psecuritypriv->busetkipkey == 1 || prxattrib->encrypt !=
_TKIP_)
) {
- DBG_COUNTER(padapter->rx_logs.core_rx_post_decrypt_hw);
-
psecuritypriv->hw_decrypted = true;
#ifdef DBG_RX_DECRYPTOR
DBG_871X("[%s] %d:prxstat->bdecrypted:%d,
prxattrib->encrypt:%d, Setting psecuritypriv->hw_decrypted = %d\n",
@@ -515,7 +508,6 @@ union recv_frame *decryptor(struct adapter *padapter, union
recv_frame *precv_fr
#endif
} else {
- DBG_COUNTER(padapter->rx_logs.core_rx_post_decrypt_unknown);
#ifdef DBG_RX_DECRYPTOR
DBG_871X("[%s] %d:prxstat->bdecrypted:%d,
prxattrib->encrypt:%d, Setting psecuritypriv->hw_decrypted = %d\n",
__func__,
@@ -1488,7 +1480,6 @@ sint validate_recv_frame(struct adapter *adapter, union
recv_frame *precv_frame)
if (ver != 0) {
RT_TRACE(_module_rtl871x_recv_c_, _drv_err_,
("validate_recv_data_frame fail! (ver!= 0)\n"));
retval = _FAIL;
- DBG_COUNTER(adapter->rx_logs.core_rx_pre_ver_err);
goto exit;
}
@@ -1515,39 +1506,29 @@ sint validate_recv_frame(struct adapter *adapter, union
recv_frame *precv_frame)
switch (type) {
case WIFI_MGT_TYPE: /* mgnt */
- DBG_COUNTER(adapter->rx_logs.core_rx_pre_mgmt);
if (validate_80211w_mgmt(adapter, precv_frame) == _FAIL) {
retval = _FAIL;
-
DBG_COUNTER(padapter->rx_logs.core_rx_pre_mgmt_err_80211w);
break;
}
retval = validate_recv_mgnt_frame(adapter, precv_frame);
- if (retval == _FAIL) {
+ if (retval == _FAIL)
RT_TRACE(_module_rtl871x_recv_c_, _drv_err_,
("validate_recv_mgnt_frame fail\n"));
- DBG_COUNTER(adapter->rx_logs.core_rx_pre_mgmt_err);
- }
retval = _FAIL; /* only data frame return _SUCCESS */
break;
case WIFI_CTRL_TYPE: /* ctrl */
- DBG_COUNTER(adapter->rx_logs.core_rx_pre_ctrl);
retval = validate_recv_ctrl_frame(adapter, precv_frame);
- if (retval == _FAIL) {
+ if (retval == _FAIL)
RT_TRACE(_module_rtl871x_recv_c_, _drv_err_,
("validate_recv_ctrl_frame fail\n"));
- DBG_COUNTER(adapter->rx_logs.core_rx_pre_ctrl_err);
- }
retval = _FAIL; /* only data frame return _SUCCESS */
break;
case WIFI_DATA_TYPE: /* data */
-
[PATCH 6/6] staging: rtl8723bs: remove rx_logs/tx_logs/int_logs from drv_types.h
Remove the rx_logs/tx_logs/int_logs struct definitions and their
inclusion within struct adapter as fields, from include/drv_types.h.
They were conditionally compiled based on CONFIG_DBG_COUNTER which
now has no other users in the driver, and were only ever accessed
in a write only fashion via the DBG_COUNTER macro, which has also
been removed.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/include/drv_types.h | 131 --
1 file changed, 131 deletions(-)
diff --git a/drivers/staging/rtl8723bs/include/drv_types.h
b/drivers/staging/rtl8723bs/include/drv_types.h
index c73f581aea06..cfde6e3ba400 100644
--- a/drivers/staging/rtl8723bs/include/drv_types.h
+++ b/drivers/staging/rtl8723bs/include/drv_types.h
@@ -219,131 +219,6 @@ struct registry_priv {
#define GET_IFACE_NUMS(padapter) (((struct adapter
*)padapter)->dvobj->iface_nums)
#define GET_ADAPTER(padapter, iface_id) (((struct adapter
*)padapter)->dvobj->padapters[iface_id])
-#ifdef CONFIG_DBG_COUNTER
-
-struct rx_logs {
- u32 intf_rx;
- u32 intf_rx_err_recvframe;
- u32 intf_rx_err_skb;
- u32 intf_rx_report;
- u32 core_rx;
- u32 core_rx_pre;
- u32 core_rx_pre_ver_err;
- u32 core_rx_pre_mgmt;
- u32 core_rx_pre_mgmt_err_80211w;
- u32 core_rx_pre_mgmt_err;
- u32 core_rx_pre_ctrl;
- u32 core_rx_pre_ctrl_err;
- u32 core_rx_pre_data;
- u32 core_rx_pre_data_wapi_seq_err;
- u32 core_rx_pre_data_wapi_key_err;
- u32 core_rx_pre_data_handled;
- u32 core_rx_pre_data_err;
- u32 core_rx_pre_data_unknown;
- u32 core_rx_pre_unknown;
- u32 core_rx_enqueue;
- u32 core_rx_dequeue;
- u32 core_rx_post;
- u32 core_rx_post_decrypt;
- u32 core_rx_post_decrypt_wep;
- u32 core_rx_post_decrypt_tkip;
- u32 core_rx_post_decrypt_aes;
- u32 core_rx_post_decrypt_wapi;
- u32 core_rx_post_decrypt_hw;
- u32 core_rx_post_decrypt_unknown;
- u32 core_rx_post_decrypt_err;
- u32 core_rx_post_defrag_err;
- u32 core_rx_post_portctrl_err;
- u32 core_rx_post_indicate;
- u32 core_rx_post_indicate_in_oder;
- u32 core_rx_post_indicate_reoder;
- u32 core_rx_post_indicate_err;
- u32 os_indicate;
- u32 os_indicate_ap_mcast;
- u32 os_indicate_ap_forward;
- u32 os_indicate_ap_self;
- u32 os_indicate_err;
- u32 os_netif_ok;
- u32 os_netif_err;
-};
-
-struct tx_logs {
- u32 os_tx;
- u32 os_tx_err_up;
- u32 os_tx_err_xmit;
- u32 os_tx_m2u;
- u32 os_tx_m2u_ignore_fw_linked;
- u32 os_tx_m2u_ignore_self;
- u32 os_tx_m2u_entry;
- u32 os_tx_m2u_entry_err_xmit;
- u32 os_tx_m2u_entry_err_skb;
- u32 os_tx_m2u_stop;
- u32 core_tx;
- u32 core_tx_err_pxmitframe;
- u32 core_tx_err_brtx;
- u32 core_tx_upd_attrib;
- u32 core_tx_upd_attrib_adhoc;
- u32 core_tx_upd_attrib_sta;
- u32 core_tx_upd_attrib_ap;
- u32 core_tx_upd_attrib_unknown;
- u32 core_tx_upd_attrib_dhcp;
- u32 core_tx_upd_attrib_icmp;
- u32 core_tx_upd_attrib_active;
- u32 core_tx_upd_attrib_err_ucast_sta;
- u32 core_tx_upd_attrib_err_ucast_ap_link;
- u32 core_tx_upd_attrib_err_sta;
- u32 core_tx_upd_attrib_err_link;
- u32 core_tx_upd_attrib_err_sec;
- u32 core_tx_ap_enqueue_warn_fwstate;
- u32 core_tx_ap_enqueue_warn_sta;
- u32 core_tx_ap_enqueue_warn_nosta;
- u32 core_tx_ap_enqueue_warn_link;
- u32 core_tx_ap_enqueue_warn_trigger;
- u32 core_tx_ap_enqueue_mcast;
- u32 core_tx_ap_enqueue_ucast;
- u32 core_tx_ap_enqueue;
- u32 intf_tx;
- u32 intf_tx_pending_ac;
- u32 intf_tx_pending_fw_under_survey;
- u32 intf_tx_pending_fw_under_linking;
- u32 intf_tx_pending_xmitbuf;
- u32 intf_tx_enqueue;
- u32 core_tx_enqueue;
- u32 core_tx_enqueue_class;
- u32 core_tx_enqueue_class_err_sta;
- u32 core_tx_enqueue_class_err_nosta;
- u32 core_tx_enqueue_class_err_fwlink;
- u32 intf_tx_direct;
- u32 intf_tx_direct_err_coalesce;
- u32 intf_tx_dequeue;
- u32 intf_tx_dequeue_err_coalesce;
- u32 intf_tx_dump_xframe;
- u32 intf_tx_dump_xframe_err_txdesc;
- u32 intf_tx_dump_xframe_err_port;
-};
-
-struct int_logs {
- u32 all;
- u32 err;
- u32 tbdok;
- u32 tbder;
- u32 bcnderr;
- u32 bcndma;
- u32 bcndma_e;
- u32 rx;
- u32 rx_rdu;
- u32 rx_fovw;
- u32 txfovw;
- u32 mgntok;
- u32 highdok;
- u32 bkdok;
- u32 bedok;
- u32 vidok;
- u32 vodok;
-};
-
-#endif /* CONFIG_DBG_COUNTER */
-
struct debug_priv {
u32 dbg_sdio_free_irq_error_cnt;
u32 dbg_sdio_alloc_irq_error_cnt;
@@ -608,12 +483,6 @@ struct adapter {
u8 driver_rx_ampdu_factor;/* 0xff: disable drv c
[PATCH 5/6] staging: rtl8723bs: remove DBG_COUNTER definition from rtw_debug.h
Remove DBG_COUNTER macro definition from include/rtw_debug.h, as
all uses of it have now been removed and it is no longer required.
The DBG_COUNTER incremented values were never actually used anywhere
else in the driver.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/include/rtw_debug.h | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
b/drivers/staging/rtl8723bs/include/rtw_debug.h
index c90adfb87261..d1c557818305 100644
--- a/drivers/staging/rtl8723bs/include/rtw_debug.h
+++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
@@ -252,12 +252,6 @@
#endif /* defined(_dbgdump) */
#endif /* DEBUG_RTL871X */
-#ifdef CONFIG_DBG_COUNTER
-#define DBG_COUNTER(counter) counter++
-#else
-#define DBG_COUNTER(counter) do {} while (0)
-#endif
-
void dump_drv_version(void *sel);
void dump_log_level(void *sel);
--
2.29.2
[PATCH 0/6] staging: rtl8723bs: remove DBG_COUNTER
This patch set removes all calls of the DBG_COUNTER macro from the driver, as the macro only increments the relevant values, which are never then used anywhere else. It then removes the DBG_COUNTER macro definition itself. In addition, it removes rx_logs/tx_logs/int_logs struct definitions from the codebase as well. These are inside a CONFIG_DBG_COUNTER preprocessor ifdef, and the only thing that was using them was the aforementioned DBG_COUNTER calls. Removing this code goes some way towards cleaning up this driver, and is therefore worth doing. Phillip Potter (6): staging: rtl8723bs: remove DBG_COUNTER calls from os_dep/recv_linux.c staging: rtl8723bs: remove DBG_COUNTER calls from os_dep/xmit_linux.c staging: rtl8723bs: remove DBG_COUNTER calls from core/rtw_xmit.c staging: rtl8723bs: remove DBG_COUNTER calls from core/rtw_recv.c staging: rtl8723bs: remove DBG_COUNTER definition from rtw_debug.h staging: rtl8723bs: remove rx_logs/tx_logs/int_logs from drv_types.h drivers/staging/rtl8723bs/core/rtw_recv.c | 44 +- drivers/staging/rtl8723bs/core/rtw_xmit.c | 44 +- drivers/staging/rtl8723bs/include/drv_types.h | 131 -- drivers/staging/rtl8723bs/include/rtw_debug.h | 6 - drivers/staging/rtl8723bs/os_dep/recv_linux.c | 15 +- drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 17 +-- 6 files changed, 10 insertions(+), 247 deletions(-) -- 2.29.2
Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h
> > So I'm in the process of stripping out _dbgdump entirely as per Greg > > K-H's suggestion - am I to understand raw printk is frowned upon though, > > even with the correct KERN_x level specified? > > Yes. Ideally in drivers everything would use dev_dbg() and dev_err() or > whatever. But it's perhaps tricky to convert everything in a single > patch so changing _dbgdump() to "#define pr_debug" as an intermediate > step is probably fine. > > Look at how people do pr_fmt(): > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > You could do a patch that does a mass replacement of DBG_871X with > pr_debug(). Again, I haven't really looked at this code so you'll have > to double check and consider what is the best way to break up the > patches. > That sounds great, I'll take a look, thanks. > > One query I have is that individual patches I'm working on for this file are > > generating an awful lot of checkpatch warnings themselves due to the > > nature of the existing violations on the relevant lines. Is it > > considered acceptable for me to still submit these, providing I do so in > > a series which cleans up the other violations in separate patches? > > It's tricky to know how to break up patches. Probably the simplest > advice is to only clean up a single type of checkpatch warning at a > time. But fix all the instances of that warning in a file. Don't > change anything else even if it is tempting. Do that in the next patch. > > The actuall rules are slightly more complicated and nuanced than that, > but if you just fix one type at a time then that's okay. > > One thing is that your patches should not introduce new checkpatch > warnings. So if you have two statements in an if statement and you > delete one, then that means you have to delete he curly braces as well. > > regards, > dan carpenter > Thanks again for the feedback. I will work on something over the next few days. Regards, Phil
Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h
On Wed, Feb 10, 2021 at 09:40:27PM +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 05:00:03PM +0000, Phillip Potter wrote:
> > Remove do/while loops from DBG_871X, MSG_8192C and DBG_8192C.
>
> I'm pretty hip to checkpatch.pl warnings, but I had forgotten what the
> warning was for this:
>
> WARNING: Single statement macros should not use a do {} while (0) loop
>
> Please, include it for people who are forgetful like I am.
>
> > Also
> > fix opening brace placements and trailing single statement layout within
> > RT_PRINT_DATA, as well as making newline character placement more
> > consistent and removing camel case where possible. Finally, add
> > parentheses for DBG_COUNTER definition.
> >
> > This fixes 3 checkpatch warnings, 5 checkpatch errors and 3 checkpatch
> > checks.
>
> This patch would be easier to review if it were split into multiple
> patches.
>
> >
> > Signed-off-by: Phillip Potter
> > ---
> > drivers/staging/rtl8723bs/include/rtw_debug.h | 40 +--
> > 1 file changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
> > b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > index c90adfb87261..d06ac9540cf7 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > @@ -201,19 +201,16 @@
> > #ifdef DEBUG
> > #ifdefined(_dbgdump)
> > #undef DBG_871X
> > - #define DBG_871X(...) do {\
> > - _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> > - } while (0)
> > + #define DBG_871X(...)\
> > + _dbgdump(DRIVER_PREFIX __VA_ARGS__)
>
> This can fit on one line:
>
> #define DBG_871X(...) _dbgdump(DRIVER_PREFIX __VA_ARGS__)
>
> It's tough with staging code to know how much to change at one time
> because even after you change the code then it still looks rubbish.
> This define shouldn't be indented. The _dbgdump() macro is just
>
> #define _dbgdump printk
>
> so you know, no printk level. Wow. etc. This code is crap.
So I'm in the process of stripping out _dbgdump entirely as per Greg
K-H's suggestion - am I to understand raw printk is frowned upon though,
even with the correct KERN_x level specified?
>
> >
> > #undef MSG_8192C
> > - #define MSG_8192C(...) do {\
> > - _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> > - } while (0)
> > + #define MSG_8192C(...)\
> > + _dbgdump(DRIVER_PREFIX __VA_ARGS__)
> >
> > #undef DBG_8192C
> > - #define DBG_8192C(...) do {\
> > - _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> > - } while (0)
> > + #define DBG_8192C(...)\
> > + _dbgdump(DRIVER_PREFIX __VA_ARGS__)
> > #endif /* defined(_dbgdump) */
> > #endif /* DEBUG */
> >
>
> Yeah. Do all the above as one patch.
>
> > @@ -235,25 +232,26 @@
> >
> > #ifdefined(_dbgdump)
> > #undef RT_PRINT_DATA
> > - #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData,
> > _HexDataLen) \
> > - if (((_Comp) & GlobalDebugComponents) && (_Level <=
> > GlobalDebugLevel)) \
> > - {
> > \
> > + #define RT_PRINT_DATA(_comp, _level, _title_string, _hex_data,
> > _hex_data_len) \
> > + do {
> > \
> > + if (((_comp) & GlobalDebugComponents) && ((_level) <=
> > GlobalDebugLevel)) { \
> > int __i;
> > \
> > - u8 *ptr = (u8 *)_HexData;
> > \
> > + u8 *ptr = (u8 *)_hex_data;
> > \
> > _dbgdump("%s", DRIVER_PREFIX);
> > \
> > - _dbgdump(_TitleString);
> > \
> > - for (__i = 0; __i < (int)_HexDataLen; __i++)
> > \
> > - {
> > \
> > + _dbgdump(_title_string);
> > \
> > + for (__i = 0; __i < (int)_hex_data_len; _
Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h
On Wed, Feb 10, 2021 at 06:12:54PM +0100, Greg KH wrote:
> On Wed, Feb 10, 2021 at 05:00:03PM +0000, Phillip Potter wrote:
> > Remove do/while loops from DBG_871X, MSG_8192C and DBG_8192C. Also
> > fix opening brace placements and trailing single statement layout within
> > RT_PRINT_DATA, as well as making newline character placement more
> > consistent and removing camel case where possible. Finally, add
> > parentheses for DBG_COUNTER definition.
> >
> > This fixes 3 checkpatch warnings, 5 checkpatch errors and 3 checkpatch
> > checks.
> >
> > Signed-off-by: Phillip Potter
> > ---
> > drivers/staging/rtl8723bs/include/rtw_debug.h | 40 +--
> > 1 file changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
> > b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > index c90adfb87261..d06ac9540cf7 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > @@ -201,19 +201,16 @@
> > #ifdef DEBUG
> > #ifdefined(_dbgdump)
> > #undef DBG_871X
> > - #define DBG_871X(...) do {\
> > - _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> > - } while (0)
> > + #define DBG_871X(...)\
> > + _dbgdump(DRIVER_PREFIX __VA_ARGS__)
> >
> > #undef MSG_8192C
> > - #define MSG_8192C(...) do {\
> > - _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> > - } while (0)
> > + #define MSG_8192C(...)\
> > + _dbgdump(DRIVER_PREFIX __VA_ARGS__)
> >
> > #undef DBG_8192C
> > - #define DBG_8192C(...) do {\
> > - _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> > - } while (0)
> > + #define DBG_8192C(...)\
> > + _dbgdump(DRIVER_PREFIX __VA_ARGS__)
>
> Odd, the do/while is correct here, why is checkpatch complaining about
> it?
The warning it gives me for these is:
WARNING: Single statement macros should not use a do {} while (0) loop
>
> > #endif /* defined(_dbgdump) */
> > #endif /* DEBUG */
> >
> > @@ -235,25 +232,26 @@
> >
> > #ifdefined(_dbgdump)
> > #undef RT_PRINT_DATA
> > - #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData,
> > _HexDataLen) \
> > - if (((_Comp) & GlobalDebugComponents) && (_Level <=
> > GlobalDebugLevel)) \
> > - {
> > \
> > + #define RT_PRINT_DATA(_comp, _level, _title_string, _hex_data,
> > _hex_data_len) \
> > + do {
> > \
> > + if (((_comp) & GlobalDebugComponents) && ((_level) <=
> > GlobalDebugLevel)) { \
> > int __i;
> > \
>
> This is not the same as the above stuff, when you find yourself writing
> "also" in a changelog text, that's a huge hint you should break the
> patch up into a patch series.
>
> Please do that here, this is too much for one patch.
>
> thanks,
>
> greg k-h
Thank you for the feedback, I'll do this - shall I leave out the
do/while stuff if you're saying checkpatch is wrong?
Regards,
Phil
[PATCH] staging: rtl8723bs: remove blank line from include/autoconf.h
Remove additional blank line from include/autoconf.h, fixes one checkpatch check notice. Signed-off-by: Phillip Potter --- drivers/staging/rtl8723bs/include/autoconf.h | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/rtl8723bs/include/autoconf.h b/drivers/staging/rtl8723bs/include/autoconf.h index 8f4c1e734473..86cf09ca5f06 100644 --- a/drivers/staging/rtl8723bs/include/autoconf.h +++ b/drivers/staging/rtl8723bs/include/autoconf.h @@ -5,7 +5,6 @@ * **/ - /* * Automatically generated C config: don't edit */ -- 2.29.2
[PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h
Remove do/while loops from DBG_871X, MSG_8192C and DBG_8192C. Also
fix opening brace placements and trailing single statement layout within
RT_PRINT_DATA, as well as making newline character placement more
consistent and removing camel case where possible. Finally, add
parentheses for DBG_COUNTER definition.
This fixes 3 checkpatch warnings, 5 checkpatch errors and 3 checkpatch
checks.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/include/rtw_debug.h | 40 +--
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
b/drivers/staging/rtl8723bs/include/rtw_debug.h
index c90adfb87261..d06ac9540cf7 100644
--- a/drivers/staging/rtl8723bs/include/rtw_debug.h
+++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
@@ -201,19 +201,16 @@
#ifdef DEBUG
#ifdefined(_dbgdump)
#undef DBG_871X
- #define DBG_871X(...) do {\
- _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
- } while (0)
+ #define DBG_871X(...)\
+ _dbgdump(DRIVER_PREFIX __VA_ARGS__)
#undef MSG_8192C
- #define MSG_8192C(...) do {\
- _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
- } while (0)
+ #define MSG_8192C(...)\
+ _dbgdump(DRIVER_PREFIX __VA_ARGS__)
#undef DBG_8192C
- #define DBG_8192C(...) do {\
- _dbgdump(DRIVER_PREFIX __VA_ARGS__);\
- } while (0)
+ #define DBG_8192C(...)\
+ _dbgdump(DRIVER_PREFIX __VA_ARGS__)
#endif /* defined(_dbgdump) */
#endif /* DEBUG */
@@ -235,25 +232,26 @@
#ifdefined(_dbgdump)
#undef RT_PRINT_DATA
- #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData,
_HexDataLen) \
- if (((_Comp) & GlobalDebugComponents) && (_Level <=
GlobalDebugLevel)) \
- {
\
+ #define RT_PRINT_DATA(_comp, _level, _title_string, _hex_data,
_hex_data_len) \
+ do {
\
+ if (((_comp) & GlobalDebugComponents) && ((_level) <=
GlobalDebugLevel)) { \
int __i;
\
- u8 *ptr = (u8 *)_HexData;
\
+ u8 *ptr = (u8 *)_hex_data;
\
_dbgdump("%s", DRIVER_PREFIX);
\
- _dbgdump(_TitleString);
\
- for (__i = 0; __i < (int)_HexDataLen; __i++)
\
- {
\
+ _dbgdump(_title_string);
\
+ for (__i = 0; __i < (int)_hex_data_len; __i++) {
\
_dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4)
== 0)?" ":" "); \
- if (((__i + 1) % 16) == 0) _dbgdump("\n");
\
- }
\
- _dbgdump("\n");
\
- }
+ if (((__i + 1) % 16) == 0)
\
+ _dbgdump("\n");
\
+ }
\
+ _dbgdump("\n");
\
+ }
\
+ } while (0)
#endif /* defined(_dbgdump) */
#endif /* DEBUG_RTL871X */
#ifdef CONFIG_DBG_COUNTER
-#define DBG_COUNTER(counter) counter++
+#define DBG_COUNTER(counter) ((counter)++)
#else
#define DBG_COUNTER(counter) do {} while (0)
#endif
--
2.29.2
[PATCH] staging: rtl8723bs: remove typedefs from rtl8723b_recv.h
Remove typedefs from include/rtl8723b_recv.h and convert one usage in
hal/rtl8723bs_recv.c to use the actual structure name in its pointer
declaration. Fixes two checkpatch warnings.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c| 2 +-
drivers/staging/rtl8723bs/include/rtl8723b_recv.h | 8
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c
b/drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c
index 1fbf89cb72d0..2d15a5f7648d 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c
@@ -24,7 +24,7 @@ static void update_recvframe_attrib(struct adapter *padapter,
{
struct rx_pkt_attrib *pattrib;
struct recv_stat report;
- PRXREPORT prxreport = (PRXREPORT)
+ struct rxreport_8723b *prxreport = (struct rxreport_8723b *)
report.rxdw0 = prxstat->rxdw0;
report.rxdw1 = prxstat->rxdw1;
diff --git a/drivers/staging/rtl8723bs/include/rtl8723b_recv.h
b/drivers/staging/rtl8723bs/include/rtl8723b_recv.h
index fad6749af768..60a1df703c8e 100644
--- a/drivers/staging/rtl8723bs/include/rtl8723b_recv.h
+++ b/drivers/staging/rtl8723bs/include/rtl8723b_recv.h
@@ -9,7 +9,7 @@
#include
-typedef struct rxreport_8723b {
+struct rxreport_8723b {
/* DWORD 0 */
u32 pktlen:14;
u32 crc32:1;
@@ -79,9 +79,9 @@ typedef struct rxreport_8723b {
/* DWORD 5 */
u32 tsfl;
-} RXREPORT, *PRXREPORT;
+};
-typedef struct phystatus_8723b {
+struct phystatus_8723b {
u32 rxgain_a:7;
u32 trsw_a:1;
u32 rxgain_b:7;
@@ -123,7 +123,7 @@ typedef struct phystatus_8723b {
u32 anttrainen:1;
u32 antselb:1;
u32 antsel:1;
-} PHYSTATUS, *PPHYSTATUS;
+};
s32 rtl8723bs_init_recv_priv(struct adapter *padapter);
void rtl8723bs_free_recv_priv(struct adapter *padapter);
--
2.29.2
[PATCH] staging: rtl8723bs: fix blank lines and comments in rtl8723b_hal.h
Remove unnecessary blank line, and move close of multiple-line comments
to their own trailing lines. This fixes four checkpatch warnings and one
checkpatch check notice for the include/rtl8723b_hal.h file.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/include/rtl8723b_hal.h | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
index f36516fa84c7..8e6e972dd843 100644
--- a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
+++ b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
@@ -42,11 +42,13 @@ struct rt_firmware_hdr {
/* LONG WORD 0 */
__le16 signature; /* 92C0: test chip; 92C, 88C0: test chip;
- * 88C1: MP A-cut; 92C1: MP A-cut */
+ * 88C1: MP A-cut; 92C1: MP A-cut
+ */
u8 category; /* AP/NIC and USB/PCI */
u8 function; /* Reserved for different FW function indications,
* for further use when driver needs to download
- * different FW in different conditions. */
+ * different FW in different conditions.
+ */
__le16 version;/* FW Version */
__le16 subversion; /* FW Subversion, default 0x00 */
@@ -135,7 +137,6 @@ struct rt_firmware_hdr {
#define WMM_NORMAL_PAGE_NUM_LPQ_8723B 0x20
#define WMM_NORMAL_PAGE_NUM_NPQ_8723B 0x20
-
#include "HalVerDef.h"
#include "hal_com.h"
@@ -149,7 +150,8 @@ struct rt_firmware_hdr {
#define EFUSE_MAX_SECTION_8723B 64
#define EFUSE_IC_ID_OFFSET 506 /* For some inferiority IC purpose.
- * Added by Roger, 2009.09.02. */
+ * Added by Roger, 2009.09.02.
+ */
#define AVAILABLE_EFUSE_ADDR(addr) (addr < EFUSE_REAL_CONTENT_LEN_8723B)
#define EFUSE_ACCESS_ON 0x69 /* For RTL8723 only. */
@@ -173,7 +175,8 @@ typedef enum _C2H_EVT {
C2H_TSF = 1,
C2H_AP_RPT_RSP = 2,
C2H_CCX_TX_RPT = 3, /* The FW notify the report
-* of the specific tx packet. */
+* of the specific tx packet.
+*/
C2H_BT_RSSI = 4,
C2H_BT_OP_MODE = 5,
C2H_EXT_RA_RPT = 6,
--
2.29.2
[PATCH] staging: rtl8723bs: fix braces for os_dep/mlme_linux.c
Add braces to both branches of an if block for consistency, and also
remove braces from a single line for loop. Fixes a checkpatch check
and warning, thus clearing this file of any brace check/warning
notices.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
index fb2df871c0cb..d46c65ab384b 100644
--- a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
@@ -48,8 +48,9 @@ void rtw_os_indicate_connect(struct adapter *adapter)
if ((check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE) == true) ||
(check_fwstate(pmlmepriv, WIFI_ADHOC_STATE) == true)) {
rtw_cfg80211_ibss_indicate_connect(adapter);
- } else
+ } else {
rtw_cfg80211_indicate_connect(adapter);
+ }
rtw_indicate_wx_assoc_event(adapter);
netif_carrier_on(adapter->pnetdev);
@@ -163,9 +164,8 @@ void rtw_report_sec_ie(struct adapter *adapter, u8
authmode, u8 *sec_ie)
len = sec_ie[1] + 2;
len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX;
- for (i = 0; i < len; i++) {
+ for (i = 0; i < len; i++)
p += sprintf(p, "%02x", sec_ie[i]);
- }
p += sprintf(p, ")");
--
2.29.2
[PATCH] staging: rtl8723bs: remove braces from two single line if blocks
Remove braces from both occurences of single line if blocks in
include/rtw_mlme.h, fixes two checkpatch warnings, thus clearing
this type of warning from this file.
Also swaps two if statement comparisons around, so the variable is on
the left in each one. This fixes two warnings also.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8723bs/include/rtw_mlme.h | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme.h
b/drivers/staging/rtl8723bs/include/rtw_mlme.h
index ea0dd156051e..d8655cb619a1 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme.h
@@ -524,18 +524,16 @@ static inline void set_fwstate(struct mlme_priv
*pmlmepriv, sint state)
{
pmlmepriv->fw_state |= state;
/* FOR HW integration */
- if (_FW_UNDER_SURVEY == state) {
+ if (state == _FW_UNDER_SURVEY)
pmlmepriv->bScanInProcess = true;
- }
}
static inline void _clr_fwstate_(struct mlme_priv *pmlmepriv, sint state)
{
pmlmepriv->fw_state &= ~state;
/* FOR HW integration */
- if (_FW_UNDER_SURVEY == state) {
+ if (state == _FW_UNDER_SURVEY)
pmlmepriv->bScanInProcess = false;
- }
}
/*
--
2.29.2
Re: [PATCH] staging: octeon: remove braces from single-line block
On Mon, Feb 08, 2021 at 08:14:02AM +0100, Alexander Sverdlin wrote:
> Hi!
>
> On 06/02/2021 21:17, Phillip Potter wrote:
> > This removes the braces from the if statement that checks the
> > physical node return value in cvm_oct_phy_setup_device, as this
> > block contains only one statement. Fixes a style warning.
> >
> > Signed-off-by: Phillip Potter
>
> Reviewed-by: Alexander Sverdlin
>
Thank you Alexander.
> > ---
> > drivers/staging/octeon/ethernet-mdio.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/octeon/ethernet-mdio.c
> > b/drivers/staging/octeon/ethernet-mdio.c
> > index 0bf545849b11..b0fd083a5bf2 100644
> > --- a/drivers/staging/octeon/ethernet-mdio.c
> > +++ b/drivers/staging/octeon/ethernet-mdio.c
> > @@ -146,9 +146,8 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
> > goto no_phy;
> >
> > phy_node = of_parse_phandle(priv->of_node, "phy-handle", 0);
> > - if (!phy_node && of_phy_is_fixed_link(priv->of_node)) {
> > + if (!phy_node && of_phy_is_fixed_link(priv->of_node))
> > phy_node = of_node_get(priv->of_node);
> > - }
> > if (!phy_node)
> > goto no_phy;
> >
>
> --
> Best regards,
> Alexander Sverdlin.
Regards,
Phil Potter
[PATCH] staging: rtl8192e: remove braces from single-line block
This removes the braces from the if statement that checks the
wps_ie_len and ieee->wps_ie values in rtllib_association_req of
rtllib_softmac.c as this block contains only one statement.
Fixes a checkpatch warning.
Signed-off-by: Phillip Potter
---
drivers/staging/rtl8192e/rtllib_softmac.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c
b/drivers/staging/rtl8192e/rtllib_softmac.c
index 2c752ba5a802..2d3be91b113d 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1352,9 +1352,8 @@ rtllib_association_req(struct rtllib_network *beacon,
rtllib_WMM_Info(ieee, );
}
- if (wps_ie_len && ieee->wps_ie) {
+ if (wps_ie_len && ieee->wps_ie)
skb_put_data(skb, ieee->wps_ie, wps_ie_len);
- }
if (turbo_info_len) {
tag = skb_put(skb, turbo_info_len);
--
2.29.2
[PATCH] staging: rtl8192e: replace spaces with tab for a closing if brace
Remove spaces preceding closing brace of one of the nested if statement blocks inside the rtl92e_leisure_ps_leave function, and replace with a tab, to align it properly with the start of the block. Fixes a checkpatch warning. Signed-off-by: Phillip Potter --- drivers/staging/rtl8192e/rtl8192e/rtl_ps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_ps.c b/drivers/staging/rtl8192e/rtl8192e/rtl_ps.c index 9475f8c6edf7..c5e89eb40342 100644 --- a/drivers/staging/rtl8192e/rtl8192e/rtl_ps.c +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_ps.c @@ -290,7 +290,7 @@ void rtl92e_leisure_ps_leave(struct net_device *dev) if (priv->rtllib->SetFwCmdHandler) priv->rtllib->SetFwCmdHandler(dev, FW_CMD_LPS_LEAVE); - } + } } } } -- 2.29.2
Re: [PATCH v2] staging: octeon: convert all uses of strlcpy to strscpy in ethernet-mdio.c
On Sun, Feb 07, 2021 at 04:35:06PM +0100, Greg KH wrote:
> On Sun, Feb 07, 2021 at 03:13:20PM +0000, Phillip Potter wrote:
> > Convert three calls to strlcpy inside the cvm_oct_get_drvinfo function
> > to strscpy calls. As return values were not checked for these three
> > calls before, change should be safe as functionality is equivalent.
> >
> > Signed-off-by: Phillip Potter
> > ---
> >
> > v2: Modified changelog to take account of feedback from Greg KH.
> >
> > drivers/staging/octeon/ethernet-mdio.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/octeon/ethernet-mdio.c
> > b/drivers/staging/octeon/ethernet-mdio.c
> > index b0fd083a5bf2..b3049108edc4 100644
> > --- a/drivers/staging/octeon/ethernet-mdio.c
> > +++ b/drivers/staging/octeon/ethernet-mdio.c
> > @@ -21,9 +21,9 @@
> > static void cvm_oct_get_drvinfo(struct net_device *dev,
> > struct ethtool_drvinfo *info)
> > {
> > - strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> > - strlcpy(info->version, UTS_RELEASE, sizeof(info->version));
> > - strlcpy(info->bus_info, "Builtin", sizeof(info->bus_info));
> > + strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> > + strscpy(info->version, UTS_RELEASE, sizeof(info->version));
> > + strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
> > }
> >
> > static int cvm_oct_nway_reset(struct net_device *dev)
>
> Sorry, this does not apply to my tree, someone already did this
> conversion before you :(
>
> greg k-h
Thank you anyway, and thank you to you and Joe for your feedback, much
appreciated.
Regards,
Phil
[PATCH v2] staging: octeon: convert all uses of strlcpy to strscpy in ethernet-mdio.c
Convert three calls to strlcpy inside the cvm_oct_get_drvinfo function
to strscpy calls. As return values were not checked for these three
calls before, change should be safe as functionality is equivalent.
Signed-off-by: Phillip Potter
---
v2: Modified changelog to take account of feedback from Greg KH.
drivers/staging/octeon/ethernet-mdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-mdio.c
b/drivers/staging/octeon/ethernet-mdio.c
index b0fd083a5bf2..b3049108edc4 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -21,9 +21,9 @@
static void cvm_oct_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
{
- strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
- strlcpy(info->version, UTS_RELEASE, sizeof(info->version));
- strlcpy(info->bus_info, "Builtin", sizeof(info->bus_info));
+ strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
+ strscpy(info->version, UTS_RELEASE, sizeof(info->version));
+ strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
}
static int cvm_oct_nway_reset(struct net_device *dev)
--
2.29.2
[PATCH v2] staging: octeon: convert all uses of strlcpy to strscpy in ethernet-mdio.c
Convert three calls to strlcpy inside the cvm_oct_get_drvinfo function
to strscpy calls. As return values were not checked for these three
calls before, change should be safe as functionality is equivalent.
Signed-off-by: Phillip Potter
---
drivers/staging/octeon/ethernet-mdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-mdio.c
b/drivers/staging/octeon/ethernet-mdio.c
index b0fd083a5bf2..b3049108edc4 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -21,9 +21,9 @@
static void cvm_oct_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
{
- strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
- strlcpy(info->version, UTS_RELEASE, sizeof(info->version));
- strlcpy(info->bus_info, "Builtin", sizeof(info->bus_info));
+ strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
+ strscpy(info->version, UTS_RELEASE, sizeof(info->version));
+ strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
}
static int cvm_oct_nway_reset(struct net_device *dev)
--
2.29.2
[PATCH] staging: octeon: convert all uses of strlcpy to strscpy in ethernet-mdio.c
Convert three calls to strlcpy inside the cvm_oct_get_drvinfo function
to strscpy calls. Fixes a style warning.
Signed-off-by: Phillip Potter
---
drivers/staging/octeon/ethernet-mdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-mdio.c
b/drivers/staging/octeon/ethernet-mdio.c
index b0fd083a5bf2..b3049108edc4 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -21,9 +21,9 @@
static void cvm_oct_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
{
- strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
- strlcpy(info->version, UTS_RELEASE, sizeof(info->version));
- strlcpy(info->bus_info, "Builtin", sizeof(info->bus_info));
+ strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
+ strscpy(info->version, UTS_RELEASE, sizeof(info->version));
+ strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
}
static int cvm_oct_nway_reset(struct net_device *dev)
--
2.29.2
[PATCH] staging: octeon: remove braces from single-line block
This removes the braces from the if statement that checks the
physical node return value in cvm_oct_phy_setup_device, as this
block contains only one statement. Fixes a style warning.
Signed-off-by: Phillip Potter
---
drivers/staging/octeon/ethernet-mdio.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-mdio.c
b/drivers/staging/octeon/ethernet-mdio.c
index 0bf545849b11..b0fd083a5bf2 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -146,9 +146,8 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
goto no_phy;
phy_node = of_parse_phandle(priv->of_node, "phy-handle", 0);
- if (!phy_node && of_phy_is_fixed_link(priv->of_node)) {
+ if (!phy_node && of_phy_is_fixed_link(priv->of_node))
phy_node = of_node_get(priv->of_node);
- }
if (!phy_node)
goto no_phy;
--
2.29.2
Re: [RFC][PATCH v3 01/10] fs: common implementation of file type
On Wed, Oct 24, 2018 at 12:44:50PM +0300, Amir Goldstein wrote: > On Wed, Oct 24, 2018 at 12:31 PM Phillip Potter wrote: > > > > On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote: > > > On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter > > > wrote: > > > > Dear Amir, > > > > > > > > Yes, I applied each patch manually to my tree, fixed it up where needed, > > > > then after rebuilding and testing each one I committed it and > > > > regenerated > > > > each patch. Thank you very much for your advice, I will take it into > > > > account and make the necessary changes. In the meantime, do I add other > > > > tags in the order they are received also (such as Reviewed-by:) and am > > > > I safe to add these in when I re-send the patches with the changes you > > > > and others have suggested (or would that offend people that have > > > > offered the tags)? > > > > > > > > > > Reviewed-by before of after Signed-off-by. > > > I prefer Signed-off-by last which conceptually covers the entire patch, > > > the commit message including all the review tags that you may have added. > > > > > > Some developers add Reviewed-by after Signed-off-by signifying the > > > order that things happened, so choose your own preference. > > > > > > As a reviewer, and I speak only for myself, if I offered my Reviewed-by > > > I expect it to be removed if a future revision of the patch has changed > > > so I have an indication of patches that I need to re-review. > > > But if the patch changed very lightly, like small edits to commit message > > > and code nits in general, that would not invalidate my review. > > > When in doubt, you can always explicitly ask the reviewer. > > > > > > Thanks, > > > Amir. > > > > Dear Amir, > > > > Thanks - I am just going to fix up the commit messages as you suggested > > using git am etc. The content of the patches themselves will not change > > (until further feedback is received). > > > > Well, I did request to change some content (the location and the comment > above BUILD_BUG_ON section) which is relevant for several patches. > However, so far affected patched did not get any Reviewed-by. > > Thanks, > Amir. Sorry, I missed that bit at the end, was too keen to click through to the note about the alleged ext2 bug :-) I will make sure those changes are made as well. By location do you mean the location of the v3, v2, etc. stuff and your point about including it in the main [0/10] message rather than the patches themselves? Again, thank you for your feedback and for being patient with me, I really do appreciate it. Regards, Phil
Re: [RFC][PATCH v3 01/10] fs: common implementation of file type
On Wed, Oct 24, 2018 at 12:44:50PM +0300, Amir Goldstein wrote: > On Wed, Oct 24, 2018 at 12:31 PM Phillip Potter wrote: > > > > On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote: > > > On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter > > > wrote: > > > > Dear Amir, > > > > > > > > Yes, I applied each patch manually to my tree, fixed it up where needed, > > > > then after rebuilding and testing each one I committed it and > > > > regenerated > > > > each patch. Thank you very much for your advice, I will take it into > > > > account and make the necessary changes. In the meantime, do I add other > > > > tags in the order they are received also (such as Reviewed-by:) and am > > > > I safe to add these in when I re-send the patches with the changes you > > > > and others have suggested (or would that offend people that have > > > > offered the tags)? > > > > > > > > > > Reviewed-by before of after Signed-off-by. > > > I prefer Signed-off-by last which conceptually covers the entire patch, > > > the commit message including all the review tags that you may have added. > > > > > > Some developers add Reviewed-by after Signed-off-by signifying the > > > order that things happened, so choose your own preference. > > > > > > As a reviewer, and I speak only for myself, if I offered my Reviewed-by > > > I expect it to be removed if a future revision of the patch has changed > > > so I have an indication of patches that I need to re-review. > > > But if the patch changed very lightly, like small edits to commit message > > > and code nits in general, that would not invalidate my review. > > > When in doubt, you can always explicitly ask the reviewer. > > > > > > Thanks, > > > Amir. > > > > Dear Amir, > > > > Thanks - I am just going to fix up the commit messages as you suggested > > using git am etc. The content of the patches themselves will not change > > (until further feedback is received). > > > > Well, I did request to change some content (the location and the comment > above BUILD_BUG_ON section) which is relevant for several patches. > However, so far affected patched did not get any Reviewed-by. > > Thanks, > Amir. Sorry, I missed that bit at the end, was too keen to click through to the note about the alleged ext2 bug :-) I will make sure those changes are made as well. By location do you mean the location of the v3, v2, etc. stuff and your point about including it in the main [0/10] message rather than the patches themselves? Again, thank you for your feedback and for being patient with me, I really do appreciate it. Regards, Phil
Re: [RFC][PATCH v3 01/10] fs: common implementation of file type
On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote: > On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter wrote: > > Dear Amir, > > > > Yes, I applied each patch manually to my tree, fixed it up where needed, > > then after rebuilding and testing each one I committed it and regenerated > > each patch. Thank you very much for your advice, I will take it into > > account and make the necessary changes. In the meantime, do I add other > > tags in the order they are received also (such as Reviewed-by:) and am > > I safe to add these in when I re-send the patches with the changes you > > and others have suggested (or would that offend people that have > > offered the tags)? > > > > Reviewed-by before of after Signed-off-by. > I prefer Signed-off-by last which conceptually covers the entire patch, > the commit message including all the review tags that you may have added. > > Some developers add Reviewed-by after Signed-off-by signifying the > order that things happened, so choose your own preference. > > As a reviewer, and I speak only for myself, if I offered my Reviewed-by > I expect it to be removed if a future revision of the patch has changed > so I have an indication of patches that I need to re-review. > But if the patch changed very lightly, like small edits to commit message > and code nits in general, that would not invalidate my review. > When in doubt, you can always explicitly ask the reviewer. > > Thanks, > Amir. Dear Amir, Thanks - I am just going to fix up the commit messages as you suggested using git am etc. The content of the patches themselves will not change (until further feedback is received). Regards, Phil
Re: [RFC][PATCH v3 01/10] fs: common implementation of file type
On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote: > On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter wrote: > > Dear Amir, > > > > Yes, I applied each patch manually to my tree, fixed it up where needed, > > then after rebuilding and testing each one I committed it and regenerated > > each patch. Thank you very much for your advice, I will take it into > > account and make the necessary changes. In the meantime, do I add other > > tags in the order they are received also (such as Reviewed-by:) and am > > I safe to add these in when I re-send the patches with the changes you > > and others have suggested (or would that offend people that have > > offered the tags)? > > > > Reviewed-by before of after Signed-off-by. > I prefer Signed-off-by last which conceptually covers the entire patch, > the commit message including all the review tags that you may have added. > > Some developers add Reviewed-by after Signed-off-by signifying the > order that things happened, so choose your own preference. > > As a reviewer, and I speak only for myself, if I offered my Reviewed-by > I expect it to be removed if a future revision of the patch has changed > so I have an indication of patches that I need to re-review. > But if the patch changed very lightly, like small edits to commit message > and code nits in general, that would not invalidate my review. > When in doubt, you can always explicitly ask the reviewer. > > Thanks, > Amir. Dear Amir, Thanks - I am just going to fix up the commit messages as you suggested using git am etc. The content of the patches themselves will not change (until further feedback is received). Regards, Phil
Re: [RFC][PATCH v3 01/10] fs: common implementation of file type
On Wed, Oct 24, 2018 at 09:16:20AM +0300, Amir Goldstein wrote: > On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter wrote: > > > > Many file systems use a copy implementation > > of dirent to on-disk file type conversions. > > > > Create a common implementation to be used by file systems > > with some useful conversion helpers to reduce open coded > > file type conversions in file system code. > > > > Original patch written by Amir Goldstein. > > Looks good. > I guess you used 'git apply' or just 'patch' > What you usually do when applying someone else mostly unchanged > patches is use 'git am -s -3' so you preserve the original author and > original commit message including the Signed-of-by line. > You can edit your patch by hand to change the From: line to change the > author and add > Signed-off-by: Amir Goldstein > (you sign below me as you changed the patch last) > Dear Amir, Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)? Regards, Phil
Re: [RFC][PATCH v3 01/10] fs: common implementation of file type
On Wed, Oct 24, 2018 at 09:16:20AM +0300, Amir Goldstein wrote: > On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter wrote: > > > > Many file systems use a copy implementation > > of dirent to on-disk file type conversions. > > > > Create a common implementation to be used by file systems > > with some useful conversion helpers to reduce open coded > > file type conversions in file system code. > > > > Original patch written by Amir Goldstein. > > Looks good. > I guess you used 'git apply' or just 'patch' > What you usually do when applying someone else mostly unchanged > patches is use 'git am -s -3' so you preserve the original author and > original commit message including the Signed-of-by line. > You can edit your patch by hand to change the From: line to change the > author and add > Signed-off-by: Amir Goldstein > (you sign below me as you changed the patch last) > Dear Amir, Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)? Regards, Phil
[RFC][PATCH v2 04/10] ext2: use common file type conversion
Deduplicate the ext2 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not remove EXT2_FT_x enum from fs/ext2/ext2.h,
as these values are now used in compile-time checks added by
Phillip Potter to make sure they remain the same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/ext2/dir.c | 51 ++-
1 file changed, 22 insertions(+), 29 deletions(-)
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 3b8114def693..420d4b9e8980 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -252,33 +252,10 @@ ext2_validate_entry(char *base, unsigned offset, unsigned
mask)
return (char *)p - base;
}
-static unsigned char ext2_filetype_table[EXT2_FT_MAX] = {
- [EXT2_FT_UNKNOWN] = DT_UNKNOWN,
- [EXT2_FT_REG_FILE] = DT_REG,
- [EXT2_FT_DIR] = DT_DIR,
- [EXT2_FT_CHRDEV]= DT_CHR,
- [EXT2_FT_BLKDEV]= DT_BLK,
- [EXT2_FT_FIFO] = DT_FIFO,
- [EXT2_FT_SOCK] = DT_SOCK,
- [EXT2_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= EXT2_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXT2_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXT2_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXT2_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXT2_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXT2_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXT2_FT_SYMLINK,
-};
-
static inline void ext2_set_de_type(ext2_dirent *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
if (EXT2_HAS_INCOMPAT_FEATURE(inode->i_sb,
EXT2_FEATURE_INCOMPAT_FILETYPE))
- de->file_type = ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
else
de->file_type = 0;
}
@@ -293,14 +270,14 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
unsigned long n = pos >> PAGE_SHIFT;
unsigned long npages = dir_pages(inode);
unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
- unsigned char *types = NULL;
bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
+ bool has_filetype;
if (pos > inode->i_size - EXT2_DIR_REC_LEN(1))
return 0;
- if (EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
- types = ext2_filetype_table;
+ has_filetype =
+ EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE);
for ( ; n < npages; n++, offset = 0) {
char *kaddr, *limit;
@@ -335,8 +312,24 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
if (de->inode) {
unsigned char d_type = DT_UNKNOWN;
- if (types && de->file_type < EXT2_FT_MAX)
- d_type = types[de->file_type];
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXT2_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX);
+
+ if (has_filetype)
+ d_type = fs_dtype(de->file_type);
if (!dir_emit(ctx, de->name, de->name_len,
le32_to_cpu(de->inode),
--
2.17.2
[RFC][PATCH v2 04/10] ext2: use common file type conversion
Deduplicate the ext2 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not remove EXT2_FT_x enum from fs/ext2/ext2.h,
as these values are now used in compile-time checks added by
Phillip Potter to make sure they remain the same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/ext2/dir.c | 51 ++-
1 file changed, 22 insertions(+), 29 deletions(-)
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 3b8114def693..420d4b9e8980 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -252,33 +252,10 @@ ext2_validate_entry(char *base, unsigned offset, unsigned
mask)
return (char *)p - base;
}
-static unsigned char ext2_filetype_table[EXT2_FT_MAX] = {
- [EXT2_FT_UNKNOWN] = DT_UNKNOWN,
- [EXT2_FT_REG_FILE] = DT_REG,
- [EXT2_FT_DIR] = DT_DIR,
- [EXT2_FT_CHRDEV]= DT_CHR,
- [EXT2_FT_BLKDEV]= DT_BLK,
- [EXT2_FT_FIFO] = DT_FIFO,
- [EXT2_FT_SOCK] = DT_SOCK,
- [EXT2_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= EXT2_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXT2_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXT2_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXT2_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXT2_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXT2_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXT2_FT_SYMLINK,
-};
-
static inline void ext2_set_de_type(ext2_dirent *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
if (EXT2_HAS_INCOMPAT_FEATURE(inode->i_sb,
EXT2_FEATURE_INCOMPAT_FILETYPE))
- de->file_type = ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
else
de->file_type = 0;
}
@@ -293,14 +270,14 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
unsigned long n = pos >> PAGE_SHIFT;
unsigned long npages = dir_pages(inode);
unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
- unsigned char *types = NULL;
bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
+ bool has_filetype;
if (pos > inode->i_size - EXT2_DIR_REC_LEN(1))
return 0;
- if (EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
- types = ext2_filetype_table;
+ has_filetype =
+ EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE);
for ( ; n < npages; n++, offset = 0) {
char *kaddr, *limit;
@@ -335,8 +312,24 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
if (de->inode) {
unsigned char d_type = DT_UNKNOWN;
- if (types && de->file_type < EXT2_FT_MAX)
- d_type = types[de->file_type];
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXT2_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX);
+
+ if (has_filetype)
+ d_type = fs_dtype(de->file_type);
if (!dir_emit(ctx, de->name, de->name_len,
le32_to_cpu(de->inode),
--
2.17.2
[RFC][PATCH v2 08/10] f2fs: use common file type conversion
Deduplicate the f2fs file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- Compile-time checks added by Phillip Potter to make
sure the F2FS_FT_x enum values stay same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/f2fs/dir.c | 43 +
fs/f2fs/inline.c| 2 +-
include/linux/f2fs_fs.h | 8 +---
3 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index ecc3a4e2be96..dcb503da0d86 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -39,37 +39,30 @@ static unsigned int bucket_blocks(unsigned int level)
return 4;
}
-static unsigned char f2fs_filetype_table[F2FS_FT_MAX] = {
- [F2FS_FT_UNKNOWN] = DT_UNKNOWN,
- [F2FS_FT_REG_FILE] = DT_REG,
- [F2FS_FT_DIR] = DT_DIR,
- [F2FS_FT_CHRDEV]= DT_CHR,
- [F2FS_FT_BLKDEV]= DT_BLK,
- [F2FS_FT_FIFO] = DT_FIFO,
- [F2FS_FT_SOCK] = DT_SOCK,
- [F2FS_FT_SYMLINK] = DT_LNK,
-};
-
-static unsigned char f2fs_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= F2FS_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= F2FS_FT_DIR,
- [S_IFCHR >> S_SHIFT]= F2FS_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= F2FS_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= F2FS_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = F2FS_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= F2FS_FT_SYMLINK,
-};
-
static void set_de_type(struct f2fs_dir_entry *de, umode_t mode)
{
- de->file_type = f2fs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
+ /*
+* compile-time asserts that generic FT_x types
+* still match F2FS_FT_x types - no need to list
+* in other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(F2FS_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(F2FS_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(F2FS_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(F2FS_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(F2FS_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(F2FS_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(F2FS_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(F2FS_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(F2FS_FT_MAX != FT_MAX);
+
+ de->file_type = fs_umode_to_ftype(mode);
}
unsigned char f2fs_get_de_type(struct f2fs_dir_entry *de)
{
- if (de->file_type < F2FS_FT_MAX)
- return f2fs_filetype_table[de->file_type];
- return DT_UNKNOWN;
+ return fs_dtype(de->file_type);
}
static unsigned long dir_block_index(unsigned int level,
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 115dc219344b..d47448904f66 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -460,7 +460,7 @@ static int f2fs_add_inline_entries(struct inode *dir, void
*inline_dentry)
new_name.len = le16_to_cpu(de->name_len);
ino = le32_to_cpu(de->ino);
- fake_mode = f2fs_get_de_type(de) << S_SHIFT;
+ fake_mode = f2fs_get_de_type(de) << S_DT_SHIFT;
err = f2fs_add_regular_entry(dir, _name, NULL, NULL,
ino, fake_mode);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index f70f8ac9c4f4..fb8ad4d87132 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -524,7 +524,11 @@ struct f2fs_dentry_block {
__u8 filename[NR_DENTRY_IN_BLOCK][F2FS_SLOT_LEN];
} __packed;
-/* file types used in inode_info->flags */
+/*
+ * file types used in inode_info->flags
+ *
+ * Values should match common file type values in file_type.h.
+ */
enum {
F2FS_FT_UNKNOWN,
F2FS_FT_REG_FILE,
@@ -537,8 +541,6 @@ enum {
F2FS_FT_MAX
};
-#define S_SHIFT 12
-
#defineF2FS_DEF_PROJID 0 /* default project ID */
#endif /* _LINUX_F2FS_FS_H */
--
2.17.2
[RFC][PATCH 03/10] hfsplus: use fs_umode_to_dtype() helper
Replace if/else statements with common lookup table implementation.
Original patch written by Amir Goldstein.
Signed-off-by: Phillip Potter
---
fs/hfsplus/dir.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index f37662675c3a..7b798a46c8ac 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -223,7 +223,6 @@ static int hfsplus_readdir(struct file *file, struct
dir_context *ctx)
break;
} else if (type == HFSPLUS_FILE) {
u16 mode;
- unsigned type = DT_UNKNOWN;
if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
pr_err("small file entry\n");
@@ -232,21 +231,10 @@ static int hfsplus_readdir(struct file *file, struct
dir_context *ctx)
}
mode = be16_to_cpu(entry.file.permissions.mode);
- if (S_ISREG(mode))
- type = DT_REG;
- else if (S_ISLNK(mode))
- type = DT_LNK;
- else if (S_ISFIFO(mode))
- type = DT_FIFO;
- else if (S_ISCHR(mode))
- type = DT_CHR;
- else if (S_ISBLK(mode))
- type = DT_BLK;
- else if (S_ISSOCK(mode))
- type = DT_SOCK;
if (!dir_emit(ctx, strbuf, len,
- be32_to_cpu(entry.file.id), type))
+ be32_to_cpu(entry.file.id),
+ fs_umode_to_dtype(mode)))
break;
} else {
pr_err("bad catalog entry type\n");
--
2.17.2
[RFC][PATCH v3 01/10] fs: common implementation of file type
Many file systems use a copy implementation
of dirent to on-disk file type conversions.
Create a common implementation to be used by file systems
with some useful conversion helpers to reduce open coded
file type conversions in file system code.
Original patch written by Amir Goldstein.
v3:
- Rebased against Linux 4.19 by Phillip Potter
- Added SPDX tag to new include/linux/file_type.h
- Added include/linux/file_type.h to MAINTAINERS
v2:
- s/DT_MASK/S_DT_MASK to fix redefinition in drivers/scsi/qla2xxx/qla_def.h
- Explicit initialization of fs_dtype_by_ftype[] using [FT_*] = DT_*
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
MAINTAINERS | 1 +
include/linux/file_type.h | 108 ++
include/linux/fs.h| 17 +-
3 files changed, 110 insertions(+), 16 deletions(-)
create mode 100644 include/linux/file_type.h
diff --git a/MAINTAINERS b/MAINTAINERS
index b2f710eee67a..8e5b029886e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5689,6 +5689,7 @@ L:linux-fsde...@vger.kernel.org
S: Maintained
F: fs/*
F: include/linux/fs.h
+F: include/linux/file_type.h
F: include/uapi/linux/fs.h
FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
diff --git a/include/linux/file_type.h b/include/linux/file_type.h
new file mode 100644
index ..f015c41ca90c
--- /dev/null
+++ b/include/linux/file_type.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FILE_TYPE_H
+#define _LINUX_FILE_TYPE_H
+
+/*
+ * This is a common implementation of dirent to fs on-disk
+ * file type conversion. Although the fs on-disk bits are
+ * specific to every file system, in practice, many file systems
+ * use the exact same on-disk format to describe the lower 3
+ * file type bits that represent the 7 POSIX file types.
+ * All those file systems can use this generic code for the
+ * conversions:
+ * i_mode -> fs on-disk file type (ftype)
+ * fs on-disk file type (ftype) -> dirent file type (dtype)
+ * i_mode -> dirent file type (dtype)
+ */
+
+/*
+ * struct dirent file types
+ * exposed to user via getdents(2), readdir(3)
+ *
+ * These match bits 12..15 of stat.st_mode
+ * (ie "(i_mode >> 12) & 15").
+ */
+#define S_DT_SHIFT 12
+#define S_DT(mode) (((mode) & S_IFMT) >> S_DT_SHIFT)
+#define S_DT_MASK (S_IFMT >> S_DT_SHIFT)
+
+#define DT_UNKNOWN 0
+#define DT_FIFOS_DT(S_IFIFO) /* 1 */
+#define DT_CHR S_DT(S_IFCHR) /* 2 */
+#define DT_DIR S_DT(S_IFDIR) /* 4 */
+#define DT_BLK S_DT(S_IFBLK) /* 6 */
+#define DT_REG S_DT(S_IFREG) /* 8 */
+#define DT_LNK S_DT(S_IFLNK) /* 10 */
+#define DT_SOCKS_DT(S_IFSOCK) /* 12 */
+#define DT_WHT 14
+
+#define DT_MAX (S_DT_MASK + 1) /* 16 */
+
+/*
+ * fs on-disk file types.
+ * Only the low 3 bits are used for the POSIX file types.
+ * Other bits are reserved for fs private use.
+ *
+ * Note that no fs currently stores the whiteout type on-disk,
+ * so whiteout dirents are exposed to user as DT_CHR.
+ */
+#define FT_UNKNOWN 0
+#define FT_REG_FILE1
+#define FT_DIR 2
+#define FT_CHRDEV 3
+#define FT_BLKDEV 4
+#define FT_FIFO5
+#define FT_SOCK6
+#define FT_SYMLINK 7
+
+#define FT_MAX 8
+
+/*
+ * fs on-disk file type to dirent file type conversion
+ */
+static unsigned char fs_dtype_by_ftype[FT_MAX] = {
+ [FT_UNKNOWN]= DT_UNKNOWN,
+ [FT_REG_FILE] = DT_REG,
+ [FT_DIR]= DT_DIR,
+ [FT_CHRDEV] = DT_CHR,
+ [FT_BLKDEV] = DT_BLK,
+ [FT_FIFO] = DT_FIFO,
+ [FT_SOCK] = DT_SOCK,
+ [FT_SYMLINK]= DT_LNK
+};
+
+static inline unsigned char fs_dtype(int filetype)
+{
+ if (filetype >= FT_MAX)
+ return DT_UNKNOWN;
+
+ return fs_dtype_by_ftype[filetype];
+}
+
+/*
+ * dirent file type to fs on-disk file type conversion
+ * Values not initialized explicitly are FT_UNKNOWN (0).
+ */
+static unsigned char fs_ftype_by_dtype[DT_MAX] = {
+ [DT_REG]= FT_REG_FILE,
+ [DT_DIR]= FT_DIR,
+ [DT_LNK]= FT_SYMLINK,
+ [DT_CHR]= FT_CHRDEV,
+ [DT_BLK]= FT_BLKDEV,
+ [DT_FIFO] = FT_FIFO,
+ [DT_SOCK] = FT_SOCK,
+};
+
+/* st_mode to fs on-disk file type conversion */
+static inline unsigned char fs_umode_to_ftype(umode_t mode)
+{
+ return fs_ftype_by_dtype[S_DT(mode)];
+}
+
+/* st_mode to dirent file type conversion */
+static inline unsigned char fs_umode_to_dtype(umode_t mode)
+{
+ return fs_dtype(fs_umode_to_ftype(mode));
+}
+
+#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 897eae8faee1..b42f04acf06e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1663,
[RFC][PATCH v2 09/10] nilfs2: use common file type conversion
Deduplicate the nilfs2 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Compile-time checks added by Phillip Potter to make
sure the NILFS_FT_x enum values stay same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/nilfs2/dir.c| 54 +++---
include/uapi/linux/nilfs2_ondisk.h | 1 +
2 files changed, 20 insertions(+), 35 deletions(-)
diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index 81394e22d0a0..168238278d24 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -229,35 +229,25 @@ static struct nilfs_dir_entry *nilfs_next_entry(struct
nilfs_dir_entry *p)
nilfs_rec_len_from_disk(p->rec_len));
}
-static unsigned char
-nilfs_filetype_table[NILFS_FT_MAX] = {
- [NILFS_FT_UNKNOWN] = DT_UNKNOWN,
- [NILFS_FT_REG_FILE] = DT_REG,
- [NILFS_FT_DIR] = DT_DIR,
- [NILFS_FT_CHRDEV] = DT_CHR,
- [NILFS_FT_BLKDEV] = DT_BLK,
- [NILFS_FT_FIFO] = DT_FIFO,
- [NILFS_FT_SOCK] = DT_SOCK,
- [NILFS_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char
-nilfs_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= NILFS_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= NILFS_FT_DIR,
- [S_IFCHR >> S_SHIFT]= NILFS_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= NILFS_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= NILFS_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = NILFS_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= NILFS_FT_SYMLINK,
-};
-
static void nilfs_set_de_type(struct nilfs_dir_entry *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
-
- de->file_type = nilfs_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ /*
+* compile-time asserts that generic FT_x types
+* still match NILFS_FT_x types - no need to list
+* in other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(NILFS_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(NILFS_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(NILFS_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(NILFS_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(NILFS_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(NILFS_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(NILFS_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(NILFS_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(NILFS_FT_MAX != FT_MAX);
+
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
}
static int nilfs_readdir(struct file *file, struct dir_context *ctx)
@@ -293,15 +283,9 @@ static int nilfs_readdir(struct file *file, struct
dir_context *ctx)
return -EIO;
}
if (de->inode) {
- unsigned char t;
-
- if (de->file_type < NILFS_FT_MAX)
- t = nilfs_filetype_table[de->file_type];
- else
- t = DT_UNKNOWN;
-
if (!dir_emit(ctx, de->name, de->name_len,
- le64_to_cpu(de->inode), t)) {
+ le64_to_cpu(de->inode),
+ fs_dtype(de->file_type))) {
nilfs_put_page(page);
return 0;
}
diff --git a/include/uapi/linux/nilfs2_ondisk.h
b/include/uapi/linux/nilfs2_ondisk.h
index a7e66ab11d1d..90362b1957f1 100644
--- a/include/uapi/linux/nilfs2_ondisk.h
+++ b/include/uapi/linux/nilfs2_ondisk.h
@@ -309,6 +309,7 @@ struct nilfs_dir_entry {
/*
* NILFS directory file types. Only the low 3 bits are used. The
* other bits are reserved for now.
+ * Values should match common file type values in file_type.h.
*/
enum {
NILFS_FT_UNKNOWN,
--
2.17.2
[RFC][PATCH 02/10] ufs: use fs_umode_to_dtype() helper
Replace switch statement with common lookup table implementation.
Original patch written by Amir Goldstein.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 29 +
1 file changed, 1 insertion(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..8c7759860739 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -158,34 +158,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = fs_umode_to_dtype(mode);
}
static inline u32
--
2.17.2
[RFC][PATCH v2 08/10] f2fs: use common file type conversion
Deduplicate the f2fs file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- Compile-time checks added by Phillip Potter to make
sure the F2FS_FT_x enum values stay same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/f2fs/dir.c | 43 +
fs/f2fs/inline.c| 2 +-
include/linux/f2fs_fs.h | 8 +---
3 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index ecc3a4e2be96..dcb503da0d86 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -39,37 +39,30 @@ static unsigned int bucket_blocks(unsigned int level)
return 4;
}
-static unsigned char f2fs_filetype_table[F2FS_FT_MAX] = {
- [F2FS_FT_UNKNOWN] = DT_UNKNOWN,
- [F2FS_FT_REG_FILE] = DT_REG,
- [F2FS_FT_DIR] = DT_DIR,
- [F2FS_FT_CHRDEV]= DT_CHR,
- [F2FS_FT_BLKDEV]= DT_BLK,
- [F2FS_FT_FIFO] = DT_FIFO,
- [F2FS_FT_SOCK] = DT_SOCK,
- [F2FS_FT_SYMLINK] = DT_LNK,
-};
-
-static unsigned char f2fs_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= F2FS_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= F2FS_FT_DIR,
- [S_IFCHR >> S_SHIFT]= F2FS_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= F2FS_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= F2FS_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = F2FS_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= F2FS_FT_SYMLINK,
-};
-
static void set_de_type(struct f2fs_dir_entry *de, umode_t mode)
{
- de->file_type = f2fs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
+ /*
+* compile-time asserts that generic FT_x types
+* still match F2FS_FT_x types - no need to list
+* in other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(F2FS_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(F2FS_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(F2FS_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(F2FS_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(F2FS_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(F2FS_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(F2FS_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(F2FS_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(F2FS_FT_MAX != FT_MAX);
+
+ de->file_type = fs_umode_to_ftype(mode);
}
unsigned char f2fs_get_de_type(struct f2fs_dir_entry *de)
{
- if (de->file_type < F2FS_FT_MAX)
- return f2fs_filetype_table[de->file_type];
- return DT_UNKNOWN;
+ return fs_dtype(de->file_type);
}
static unsigned long dir_block_index(unsigned int level,
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 115dc219344b..d47448904f66 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -460,7 +460,7 @@ static int f2fs_add_inline_entries(struct inode *dir, void
*inline_dentry)
new_name.len = le16_to_cpu(de->name_len);
ino = le32_to_cpu(de->ino);
- fake_mode = f2fs_get_de_type(de) << S_SHIFT;
+ fake_mode = f2fs_get_de_type(de) << S_DT_SHIFT;
err = f2fs_add_regular_entry(dir, _name, NULL, NULL,
ino, fake_mode);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index f70f8ac9c4f4..fb8ad4d87132 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -524,7 +524,11 @@ struct f2fs_dentry_block {
__u8 filename[NR_DENTRY_IN_BLOCK][F2FS_SLOT_LEN];
} __packed;
-/* file types used in inode_info->flags */
+/*
+ * file types used in inode_info->flags
+ *
+ * Values should match common file type values in file_type.h.
+ */
enum {
F2FS_FT_UNKNOWN,
F2FS_FT_REG_FILE,
@@ -537,8 +541,6 @@ enum {
F2FS_FT_MAX
};
-#define S_SHIFT 12
-
#defineF2FS_DEF_PROJID 0 /* default project ID */
#endif /* _LINUX_F2FS_FS_H */
--
2.17.2
[RFC][PATCH 03/10] hfsplus: use fs_umode_to_dtype() helper
Replace if/else statements with common lookup table implementation.
Original patch written by Amir Goldstein.
Signed-off-by: Phillip Potter
---
fs/hfsplus/dir.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index f37662675c3a..7b798a46c8ac 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -223,7 +223,6 @@ static int hfsplus_readdir(struct file *file, struct
dir_context *ctx)
break;
} else if (type == HFSPLUS_FILE) {
u16 mode;
- unsigned type = DT_UNKNOWN;
if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
pr_err("small file entry\n");
@@ -232,21 +231,10 @@ static int hfsplus_readdir(struct file *file, struct
dir_context *ctx)
}
mode = be16_to_cpu(entry.file.permissions.mode);
- if (S_ISREG(mode))
- type = DT_REG;
- else if (S_ISLNK(mode))
- type = DT_LNK;
- else if (S_ISFIFO(mode))
- type = DT_FIFO;
- else if (S_ISCHR(mode))
- type = DT_CHR;
- else if (S_ISBLK(mode))
- type = DT_BLK;
- else if (S_ISSOCK(mode))
- type = DT_SOCK;
if (!dir_emit(ctx, strbuf, len,
- be32_to_cpu(entry.file.id), type))
+ be32_to_cpu(entry.file.id),
+ fs_umode_to_dtype(mode)))
break;
} else {
pr_err("bad catalog entry type\n");
--
2.17.2
[RFC][PATCH v3 01/10] fs: common implementation of file type
Many file systems use a copy implementation
of dirent to on-disk file type conversions.
Create a common implementation to be used by file systems
with some useful conversion helpers to reduce open coded
file type conversions in file system code.
Original patch written by Amir Goldstein.
v3:
- Rebased against Linux 4.19 by Phillip Potter
- Added SPDX tag to new include/linux/file_type.h
- Added include/linux/file_type.h to MAINTAINERS
v2:
- s/DT_MASK/S_DT_MASK to fix redefinition in drivers/scsi/qla2xxx/qla_def.h
- Explicit initialization of fs_dtype_by_ftype[] using [FT_*] = DT_*
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
MAINTAINERS | 1 +
include/linux/file_type.h | 108 ++
include/linux/fs.h| 17 +-
3 files changed, 110 insertions(+), 16 deletions(-)
create mode 100644 include/linux/file_type.h
diff --git a/MAINTAINERS b/MAINTAINERS
index b2f710eee67a..8e5b029886e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5689,6 +5689,7 @@ L:linux-fsde...@vger.kernel.org
S: Maintained
F: fs/*
F: include/linux/fs.h
+F: include/linux/file_type.h
F: include/uapi/linux/fs.h
FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
diff --git a/include/linux/file_type.h b/include/linux/file_type.h
new file mode 100644
index ..f015c41ca90c
--- /dev/null
+++ b/include/linux/file_type.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FILE_TYPE_H
+#define _LINUX_FILE_TYPE_H
+
+/*
+ * This is a common implementation of dirent to fs on-disk
+ * file type conversion. Although the fs on-disk bits are
+ * specific to every file system, in practice, many file systems
+ * use the exact same on-disk format to describe the lower 3
+ * file type bits that represent the 7 POSIX file types.
+ * All those file systems can use this generic code for the
+ * conversions:
+ * i_mode -> fs on-disk file type (ftype)
+ * fs on-disk file type (ftype) -> dirent file type (dtype)
+ * i_mode -> dirent file type (dtype)
+ */
+
+/*
+ * struct dirent file types
+ * exposed to user via getdents(2), readdir(3)
+ *
+ * These match bits 12..15 of stat.st_mode
+ * (ie "(i_mode >> 12) & 15").
+ */
+#define S_DT_SHIFT 12
+#define S_DT(mode) (((mode) & S_IFMT) >> S_DT_SHIFT)
+#define S_DT_MASK (S_IFMT >> S_DT_SHIFT)
+
+#define DT_UNKNOWN 0
+#define DT_FIFOS_DT(S_IFIFO) /* 1 */
+#define DT_CHR S_DT(S_IFCHR) /* 2 */
+#define DT_DIR S_DT(S_IFDIR) /* 4 */
+#define DT_BLK S_DT(S_IFBLK) /* 6 */
+#define DT_REG S_DT(S_IFREG) /* 8 */
+#define DT_LNK S_DT(S_IFLNK) /* 10 */
+#define DT_SOCKS_DT(S_IFSOCK) /* 12 */
+#define DT_WHT 14
+
+#define DT_MAX (S_DT_MASK + 1) /* 16 */
+
+/*
+ * fs on-disk file types.
+ * Only the low 3 bits are used for the POSIX file types.
+ * Other bits are reserved for fs private use.
+ *
+ * Note that no fs currently stores the whiteout type on-disk,
+ * so whiteout dirents are exposed to user as DT_CHR.
+ */
+#define FT_UNKNOWN 0
+#define FT_REG_FILE1
+#define FT_DIR 2
+#define FT_CHRDEV 3
+#define FT_BLKDEV 4
+#define FT_FIFO5
+#define FT_SOCK6
+#define FT_SYMLINK 7
+
+#define FT_MAX 8
+
+/*
+ * fs on-disk file type to dirent file type conversion
+ */
+static unsigned char fs_dtype_by_ftype[FT_MAX] = {
+ [FT_UNKNOWN]= DT_UNKNOWN,
+ [FT_REG_FILE] = DT_REG,
+ [FT_DIR]= DT_DIR,
+ [FT_CHRDEV] = DT_CHR,
+ [FT_BLKDEV] = DT_BLK,
+ [FT_FIFO] = DT_FIFO,
+ [FT_SOCK] = DT_SOCK,
+ [FT_SYMLINK]= DT_LNK
+};
+
+static inline unsigned char fs_dtype(int filetype)
+{
+ if (filetype >= FT_MAX)
+ return DT_UNKNOWN;
+
+ return fs_dtype_by_ftype[filetype];
+}
+
+/*
+ * dirent file type to fs on-disk file type conversion
+ * Values not initialized explicitly are FT_UNKNOWN (0).
+ */
+static unsigned char fs_ftype_by_dtype[DT_MAX] = {
+ [DT_REG]= FT_REG_FILE,
+ [DT_DIR]= FT_DIR,
+ [DT_LNK]= FT_SYMLINK,
+ [DT_CHR]= FT_CHRDEV,
+ [DT_BLK]= FT_BLKDEV,
+ [DT_FIFO] = FT_FIFO,
+ [DT_SOCK] = FT_SOCK,
+};
+
+/* st_mode to fs on-disk file type conversion */
+static inline unsigned char fs_umode_to_ftype(umode_t mode)
+{
+ return fs_ftype_by_dtype[S_DT(mode)];
+}
+
+/* st_mode to dirent file type conversion */
+static inline unsigned char fs_umode_to_dtype(umode_t mode)
+{
+ return fs_dtype(fs_umode_to_ftype(mode));
+}
+
+#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 897eae8faee1..b42f04acf06e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1663,
[RFC][PATCH v2 09/10] nilfs2: use common file type conversion
Deduplicate the nilfs2 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Compile-time checks added by Phillip Potter to make
sure the NILFS_FT_x enum values stay same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/nilfs2/dir.c| 54 +++---
include/uapi/linux/nilfs2_ondisk.h | 1 +
2 files changed, 20 insertions(+), 35 deletions(-)
diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index 81394e22d0a0..168238278d24 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -229,35 +229,25 @@ static struct nilfs_dir_entry *nilfs_next_entry(struct
nilfs_dir_entry *p)
nilfs_rec_len_from_disk(p->rec_len));
}
-static unsigned char
-nilfs_filetype_table[NILFS_FT_MAX] = {
- [NILFS_FT_UNKNOWN] = DT_UNKNOWN,
- [NILFS_FT_REG_FILE] = DT_REG,
- [NILFS_FT_DIR] = DT_DIR,
- [NILFS_FT_CHRDEV] = DT_CHR,
- [NILFS_FT_BLKDEV] = DT_BLK,
- [NILFS_FT_FIFO] = DT_FIFO,
- [NILFS_FT_SOCK] = DT_SOCK,
- [NILFS_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char
-nilfs_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= NILFS_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= NILFS_FT_DIR,
- [S_IFCHR >> S_SHIFT]= NILFS_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= NILFS_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= NILFS_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = NILFS_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= NILFS_FT_SYMLINK,
-};
-
static void nilfs_set_de_type(struct nilfs_dir_entry *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
-
- de->file_type = nilfs_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ /*
+* compile-time asserts that generic FT_x types
+* still match NILFS_FT_x types - no need to list
+* in other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(NILFS_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(NILFS_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(NILFS_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(NILFS_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(NILFS_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(NILFS_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(NILFS_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(NILFS_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(NILFS_FT_MAX != FT_MAX);
+
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
}
static int nilfs_readdir(struct file *file, struct dir_context *ctx)
@@ -293,15 +283,9 @@ static int nilfs_readdir(struct file *file, struct
dir_context *ctx)
return -EIO;
}
if (de->inode) {
- unsigned char t;
-
- if (de->file_type < NILFS_FT_MAX)
- t = nilfs_filetype_table[de->file_type];
- else
- t = DT_UNKNOWN;
-
if (!dir_emit(ctx, de->name, de->name_len,
- le64_to_cpu(de->inode), t)) {
+ le64_to_cpu(de->inode),
+ fs_dtype(de->file_type))) {
nilfs_put_page(page);
return 0;
}
diff --git a/include/uapi/linux/nilfs2_ondisk.h
b/include/uapi/linux/nilfs2_ondisk.h
index a7e66ab11d1d..90362b1957f1 100644
--- a/include/uapi/linux/nilfs2_ondisk.h
+++ b/include/uapi/linux/nilfs2_ondisk.h
@@ -309,6 +309,7 @@ struct nilfs_dir_entry {
/*
* NILFS directory file types. Only the low 3 bits are used. The
* other bits are reserved for now.
+ * Values should match common file type values in file_type.h.
*/
enum {
NILFS_FT_UNKNOWN,
--
2.17.2
[RFC][PATCH 02/10] ufs: use fs_umode_to_dtype() helper
Replace switch statement with common lookup table implementation.
Original patch written by Amir Goldstein.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 29 +
1 file changed, 1 insertion(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..8c7759860739 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -158,34 +158,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = fs_umode_to_dtype(mode);
}
static inline u32
--
2.17.2
[RFC][PATCH 00/11] common implementation of dirent file types
This cleanup series is a respin of Amir Goldstein's work, created in late 2016. It removes several instances of duplicated code. Most of the duplication dates back to git pre-historic era. The controversial aspect of this cleanup is that it uses common code to handle file system specific on-disk format bits. All 7 file systems use a single byte to store dirent file type on-disk and all of them use the same conversion routines from i_mode to 4bits DT_* constants to 3bits on-disk FT_* constants. Patch 1 places a common implementation in file_type.h and add some useful conversion helpers. Patches 2-3 make use of some helpers in ufs and hfsplus without any on-disk implications. Patches 4-10 replace the specific implementations in ext2, exofs, ext4, ocfs2, f2fs, nilfs and btrfs with the common implementation. These patches also now include compile-time checks to ensure that the file system specific on-disk bits are equivalent to the common implementation FT_* bits. These compile-time checks are only included once per file system, as my reasoning is that regardless of their location, the build will fail/succeed anyway. In addition, where needed (for patches which no longer apply), I've rebased them to apply to the newest 4.19 kernel sources. Each patch is independent of the others, except for the common implementation itself which they all depend on. I would love feedback as newish kernel contributor, particularly from the original author Amir who suggested this task for me. I hope the various subsystem/fs maintainers will see fit to accept this work also. Phillip Potter (10): fs: common implementation of file type conversions ufs: use fs_umode_to_dtype() helper hfsplus: use fs_umode_to_dtype() helper ext2: use common file type conversion exofs: use common file type conversion ext4: use common file type conversion ocfs2: use common file type conversion f2fs: use common file type conversion nilfs2: use common file type conversion btrfs: use common file type conversion --- MAINTAINERS| 1 + fs/btrfs/btrfs_inode.h | 2 - fs/btrfs/delayed-inode.c | 2 +- fs/btrfs/inode.c | 35 +- fs/exofs/dir.c | 49 + fs/ext2/dir.c | 51 ++ fs/ext4/ext4.h | 35 +- fs/f2fs/dir.c | 43 +--- fs/f2fs/inline.c | 2 +- fs/hfsplus/dir.c | 16 + fs/nilfs2/dir.c| 54 +-- fs/ocfs2/dir.c | 32 + fs/ocfs2/ocfs2_fs.h| 13 +--- fs/ufs/util.h | 29 +--- include/linux/f2fs_fs.h| 8 ++- include/linux/file_type.h | 108 + include/linux/fs.h | 17 + include/uapi/linux/btrfs_tree.h| 2 + include/uapi/linux/nilfs2_ondisk.h | 1 + 19 files changed, 256 insertions(+), 244 deletions(-) create mode 100644 include/linux/file_type.h -- 2.17.2
[RFC][PATCH 00/11] common implementation of dirent file types
This cleanup series is a respin of Amir Goldstein's work, created in late 2016. It removes several instances of duplicated code. Most of the duplication dates back to git pre-historic era. The controversial aspect of this cleanup is that it uses common code to handle file system specific on-disk format bits. All 7 file systems use a single byte to store dirent file type on-disk and all of them use the same conversion routines from i_mode to 4bits DT_* constants to 3bits on-disk FT_* constants. Patch 1 places a common implementation in file_type.h and add some useful conversion helpers. Patches 2-3 make use of some helpers in ufs and hfsplus without any on-disk implications. Patches 4-10 replace the specific implementations in ext2, exofs, ext4, ocfs2, f2fs, nilfs and btrfs with the common implementation. These patches also now include compile-time checks to ensure that the file system specific on-disk bits are equivalent to the common implementation FT_* bits. These compile-time checks are only included once per file system, as my reasoning is that regardless of their location, the build will fail/succeed anyway. In addition, where needed (for patches which no longer apply), I've rebased them to apply to the newest 4.19 kernel sources. Each patch is independent of the others, except for the common implementation itself which they all depend on. I would love feedback as newish kernel contributor, particularly from the original author Amir who suggested this task for me. I hope the various subsystem/fs maintainers will see fit to accept this work also. Phillip Potter (10): fs: common implementation of file type conversions ufs: use fs_umode_to_dtype() helper hfsplus: use fs_umode_to_dtype() helper ext2: use common file type conversion exofs: use common file type conversion ext4: use common file type conversion ocfs2: use common file type conversion f2fs: use common file type conversion nilfs2: use common file type conversion btrfs: use common file type conversion --- MAINTAINERS| 1 + fs/btrfs/btrfs_inode.h | 2 - fs/btrfs/delayed-inode.c | 2 +- fs/btrfs/inode.c | 35 +- fs/exofs/dir.c | 49 + fs/ext2/dir.c | 51 ++ fs/ext4/ext4.h | 35 +- fs/f2fs/dir.c | 43 +--- fs/f2fs/inline.c | 2 +- fs/hfsplus/dir.c | 16 + fs/nilfs2/dir.c| 54 +-- fs/ocfs2/dir.c | 32 + fs/ocfs2/ocfs2_fs.h| 13 +--- fs/ufs/util.h | 29 +--- include/linux/f2fs_fs.h| 8 ++- include/linux/file_type.h | 108 + include/linux/fs.h | 17 + include/uapi/linux/btrfs_tree.h| 2 + include/uapi/linux/nilfs2_ondisk.h | 1 + 19 files changed, 256 insertions(+), 244 deletions(-) create mode 100644 include/linux/file_type.h -- 2.17.2
[RFC][PATCH v2 05/10] exofs: use common file type conversion
Deduplicate the exofs file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- This version does not remove EXOFS_FT_x enum from fs/exofs/common.h,
as these values are now used in compile-time checks added by
Phillip Potter to make sure they remain the same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/exofs/dir.c | 49 ++---
1 file changed, 18 insertions(+), 31 deletions(-)
diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
index f0138674c1ed..2e3161ca9014 100644
--- a/fs/exofs/dir.c
+++ b/fs/exofs/dir.c
@@ -204,33 +204,10 @@ exofs_validate_entry(char *base, unsigned offset,
unsigned mask)
return (char *)p - base;
}
-static unsigned char exofs_filetype_table[EXOFS_FT_MAX] = {
- [EXOFS_FT_UNKNOWN] = DT_UNKNOWN,
- [EXOFS_FT_REG_FILE] = DT_REG,
- [EXOFS_FT_DIR] = DT_DIR,
- [EXOFS_FT_CHRDEV] = DT_CHR,
- [EXOFS_FT_BLKDEV] = DT_BLK,
- [EXOFS_FT_FIFO] = DT_FIFO,
- [EXOFS_FT_SOCK] = DT_SOCK,
- [EXOFS_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char exofs_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= EXOFS_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXOFS_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXOFS_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXOFS_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXOFS_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXOFS_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXOFS_FT_SYMLINK,
-};
-
static inline
void exofs_set_de_type(struct exofs_dir_entry *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
- de->file_type = exofs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
}
static int
@@ -279,17 +256,27 @@ exofs_readdir(struct file *file, struct dir_context *ctx)
exofs_put_page(page);
return -EIO;
}
- if (de->inode_no) {
- unsigned char t;
- if (de->file_type < EXOFS_FT_MAX)
- t = exofs_filetype_table[de->file_type];
- else
- t = DT_UNKNOWN;
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXOFS_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXOFS_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXOFS_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXOFS_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXOFS_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXOFS_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXOFS_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXOFS_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXOFS_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXOFS_FT_MAX != FT_MAX);
+ if (de->inode_no) {
if (!dir_emit(ctx, de->name, de->name_len,
le64_to_cpu(de->inode_no),
- t)) {
+ fs_dtype(de->file_type))) {
exofs_put_page(page);
return 0;
}
--
2.17.2
[RFC][PATCH v2 05/10] exofs: use common file type conversion
Deduplicate the exofs file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- This version does not remove EXOFS_FT_x enum from fs/exofs/common.h,
as these values are now used in compile-time checks added by
Phillip Potter to make sure they remain the same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/exofs/dir.c | 49 ++---
1 file changed, 18 insertions(+), 31 deletions(-)
diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
index f0138674c1ed..2e3161ca9014 100644
--- a/fs/exofs/dir.c
+++ b/fs/exofs/dir.c
@@ -204,33 +204,10 @@ exofs_validate_entry(char *base, unsigned offset,
unsigned mask)
return (char *)p - base;
}
-static unsigned char exofs_filetype_table[EXOFS_FT_MAX] = {
- [EXOFS_FT_UNKNOWN] = DT_UNKNOWN,
- [EXOFS_FT_REG_FILE] = DT_REG,
- [EXOFS_FT_DIR] = DT_DIR,
- [EXOFS_FT_CHRDEV] = DT_CHR,
- [EXOFS_FT_BLKDEV] = DT_BLK,
- [EXOFS_FT_FIFO] = DT_FIFO,
- [EXOFS_FT_SOCK] = DT_SOCK,
- [EXOFS_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char exofs_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= EXOFS_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXOFS_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXOFS_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXOFS_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXOFS_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXOFS_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXOFS_FT_SYMLINK,
-};
-
static inline
void exofs_set_de_type(struct exofs_dir_entry *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
- de->file_type = exofs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
}
static int
@@ -279,17 +256,27 @@ exofs_readdir(struct file *file, struct dir_context *ctx)
exofs_put_page(page);
return -EIO;
}
- if (de->inode_no) {
- unsigned char t;
- if (de->file_type < EXOFS_FT_MAX)
- t = exofs_filetype_table[de->file_type];
- else
- t = DT_UNKNOWN;
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXOFS_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXOFS_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXOFS_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXOFS_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXOFS_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXOFS_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXOFS_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXOFS_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXOFS_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXOFS_FT_MAX != FT_MAX);
+ if (de->inode_no) {
if (!dir_emit(ctx, de->name, de->name_len,
le64_to_cpu(de->inode_no),
- t)) {
+ fs_dtype(de->file_type))) {
exofs_put_page(page);
return 0;
}
--
2.17.2
[RFC][PATCH v2 06/10] ext4: use common file type conversion
Deduplicate the ext4 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not replace the EXT4_FT_x defines from
fs/ext4/ext4.h, as these values are now used in compile-time
checks added by Phillip Potter to make sure they remain the
same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/ext4/ext4.h | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index caff935fbeb8..4cb77a2cacdf 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2361,16 +2361,13 @@ static inline void ext4_update_dx_flag(struct inode
*inode)
if (!ext4_has_feature_dir_index(inode->i_sb))
ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
}
-static const unsigned char ext4_filetype_table[] = {
- DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
-};
static inline unsigned char get_dtype(struct super_block *sb, int filetype)
{
- if (!ext4_has_feature_filetype(sb) || filetype >= EXT4_FT_MAX)
+ if (!ext4_has_feature_filetype(sb))
return DT_UNKNOWN;
- return ext4_filetype_table[filetype];
+ return fs_dtype(filetype);
}
extern int ext4_check_all_de(struct inode *dir, struct buffer_head *bh,
void *buf, int buf_size);
@@ -3055,22 +3052,28 @@ extern void initialize_dirent_tail(struct
ext4_dir_entry_tail *t,
extern int ext4_handle_dirty_dirent_node(handle_t *handle,
struct inode *inode,
struct buffer_head *bh);
-#define S_SHIFT 12
-static const unsigned char ext4_type_by_mode[(S_IFMT >> S_SHIFT) + 1] = {
- [S_IFREG >> S_SHIFT]= EXT4_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXT4_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXT4_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXT4_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXT4_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXT4_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXT4_FT_SYMLINK,
-};
static inline void ext4_set_de_type(struct super_block *sb,
struct ext4_dir_entry_2 *de,
umode_t mode) {
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXT4_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXT4_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT4_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT4_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT4_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT4_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT4_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT4_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT4_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT4_FT_MAX != FT_MAX);
+
if (ext4_has_feature_filetype(sb))
- de->file_type = ext4_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ de->file_type = fs_umode_to_ftype(mode);
}
/* readpages.c */
--
2.17.2
[RFC][PATCH v2 04/10] ext2: use common file type conversion
Deduplicate the ext2 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not remove EXT2_FT_x enum from fs/ext2/ext2.h,
as these values are now used in compile-time checks added by
Phillip Potter to make sure they remain the same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/ext2/dir.c | 51 ++-
1 file changed, 22 insertions(+), 29 deletions(-)
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 3b8114def693..420d4b9e8980 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -252,33 +252,10 @@ ext2_validate_entry(char *base, unsigned offset, unsigned
mask)
return (char *)p - base;
}
-static unsigned char ext2_filetype_table[EXT2_FT_MAX] = {
- [EXT2_FT_UNKNOWN] = DT_UNKNOWN,
- [EXT2_FT_REG_FILE] = DT_REG,
- [EXT2_FT_DIR] = DT_DIR,
- [EXT2_FT_CHRDEV]= DT_CHR,
- [EXT2_FT_BLKDEV]= DT_BLK,
- [EXT2_FT_FIFO] = DT_FIFO,
- [EXT2_FT_SOCK] = DT_SOCK,
- [EXT2_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= EXT2_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXT2_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXT2_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXT2_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXT2_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXT2_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXT2_FT_SYMLINK,
-};
-
static inline void ext2_set_de_type(ext2_dirent *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
if (EXT2_HAS_INCOMPAT_FEATURE(inode->i_sb,
EXT2_FEATURE_INCOMPAT_FILETYPE))
- de->file_type = ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
else
de->file_type = 0;
}
@@ -293,14 +270,14 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
unsigned long n = pos >> PAGE_SHIFT;
unsigned long npages = dir_pages(inode);
unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
- unsigned char *types = NULL;
bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
+ bool has_filetype;
if (pos > inode->i_size - EXT2_DIR_REC_LEN(1))
return 0;
- if (EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
- types = ext2_filetype_table;
+ has_filetype =
+ EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE);
for ( ; n < npages; n++, offset = 0) {
char *kaddr, *limit;
@@ -335,8 +312,24 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
if (de->inode) {
unsigned char d_type = DT_UNKNOWN;
- if (types && de->file_type < EXT2_FT_MAX)
- d_type = types[de->file_type];
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXT2_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX);
+
+ if (has_filetype)
+ d_type = fs_dtype(de->file_type);
if (!dir_emit(ctx, de->name, de->name_len,
le32_to_cpu(de->inode),
--
2.17.2
[RFC][PATCH v2 06/10] ext4: use common file type conversion
Deduplicate the ext4 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not replace the EXT4_FT_x defines from
fs/ext4/ext4.h, as these values are now used in compile-time
checks added by Phillip Potter to make sure they remain the
same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/ext4/ext4.h | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index caff935fbeb8..4cb77a2cacdf 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2361,16 +2361,13 @@ static inline void ext4_update_dx_flag(struct inode
*inode)
if (!ext4_has_feature_dir_index(inode->i_sb))
ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
}
-static const unsigned char ext4_filetype_table[] = {
- DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
-};
static inline unsigned char get_dtype(struct super_block *sb, int filetype)
{
- if (!ext4_has_feature_filetype(sb) || filetype >= EXT4_FT_MAX)
+ if (!ext4_has_feature_filetype(sb))
return DT_UNKNOWN;
- return ext4_filetype_table[filetype];
+ return fs_dtype(filetype);
}
extern int ext4_check_all_de(struct inode *dir, struct buffer_head *bh,
void *buf, int buf_size);
@@ -3055,22 +3052,28 @@ extern void initialize_dirent_tail(struct
ext4_dir_entry_tail *t,
extern int ext4_handle_dirty_dirent_node(handle_t *handle,
struct inode *inode,
struct buffer_head *bh);
-#define S_SHIFT 12
-static const unsigned char ext4_type_by_mode[(S_IFMT >> S_SHIFT) + 1] = {
- [S_IFREG >> S_SHIFT]= EXT4_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXT4_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXT4_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXT4_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXT4_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXT4_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXT4_FT_SYMLINK,
-};
static inline void ext4_set_de_type(struct super_block *sb,
struct ext4_dir_entry_2 *de,
umode_t mode) {
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXT4_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXT4_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT4_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT4_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT4_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT4_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT4_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT4_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT4_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT4_FT_MAX != FT_MAX);
+
if (ext4_has_feature_filetype(sb))
- de->file_type = ext4_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ de->file_type = fs_umode_to_ftype(mode);
}
/* readpages.c */
--
2.17.2
[RFC][PATCH v2 04/10] ext2: use common file type conversion
Deduplicate the ext2 file type conversion implementation.
Original patch by Amir Goldstein.
v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not remove EXT2_FT_x enum from fs/ext2/ext2.h,
as these values are now used in compile-time checks added by
Phillip Potter to make sure they remain the same as FT_x values
v1:
- Initial implementation
Signed-off-by: Phillip Potter
---
fs/ext2/dir.c | 51 ++-
1 file changed, 22 insertions(+), 29 deletions(-)
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 3b8114def693..420d4b9e8980 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -252,33 +252,10 @@ ext2_validate_entry(char *base, unsigned offset, unsigned
mask)
return (char *)p - base;
}
-static unsigned char ext2_filetype_table[EXT2_FT_MAX] = {
- [EXT2_FT_UNKNOWN] = DT_UNKNOWN,
- [EXT2_FT_REG_FILE] = DT_REG,
- [EXT2_FT_DIR] = DT_DIR,
- [EXT2_FT_CHRDEV]= DT_CHR,
- [EXT2_FT_BLKDEV]= DT_BLK,
- [EXT2_FT_FIFO] = DT_FIFO,
- [EXT2_FT_SOCK] = DT_SOCK,
- [EXT2_FT_SYMLINK] = DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
- [S_IFREG >> S_SHIFT]= EXT2_FT_REG_FILE,
- [S_IFDIR >> S_SHIFT]= EXT2_FT_DIR,
- [S_IFCHR >> S_SHIFT]= EXT2_FT_CHRDEV,
- [S_IFBLK >> S_SHIFT]= EXT2_FT_BLKDEV,
- [S_IFIFO >> S_SHIFT]= EXT2_FT_FIFO,
- [S_IFSOCK >> S_SHIFT] = EXT2_FT_SOCK,
- [S_IFLNK >> S_SHIFT]= EXT2_FT_SYMLINK,
-};
-
static inline void ext2_set_de_type(ext2_dirent *de, struct inode *inode)
{
- umode_t mode = inode->i_mode;
if (EXT2_HAS_INCOMPAT_FEATURE(inode->i_sb,
EXT2_FEATURE_INCOMPAT_FILETYPE))
- de->file_type = ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+ de->file_type = fs_umode_to_ftype(inode->i_mode);
else
de->file_type = 0;
}
@@ -293,14 +270,14 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
unsigned long n = pos >> PAGE_SHIFT;
unsigned long npages = dir_pages(inode);
unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
- unsigned char *types = NULL;
bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
+ bool has_filetype;
if (pos > inode->i_size - EXT2_DIR_REC_LEN(1))
return 0;
- if (EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
- types = ext2_filetype_table;
+ has_filetype =
+ EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE);
for ( ; n < npages; n++, offset = 0) {
char *kaddr, *limit;
@@ -335,8 +312,24 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
if (de->inode) {
unsigned char d_type = DT_UNKNOWN;
- if (types && de->file_type < EXT2_FT_MAX)
- d_type = types[de->file_type];
+ /*
+* compile-time asserts that generic FT_x types
+* still match EXT2_FT_x types - no need to list
+* for other functions as well as build will
+* fail either way
+*/
+ BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN);
+ BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE);
+ BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR);
+ BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV);
+ BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV);
+ BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO);
+ BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK);
+ BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK);
+ BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX);
+
+ if (has_filetype)
+ d_type = fs_dtype(de->file_type);
if (!dir_emit(ctx, de->name, de->name_len,
le32_to_cpu(de->inode),
--
2.17.2
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Sun, Oct 21, 2018 at 02:02:57PM +0300, Amir Goldstein wrote:
> Yes. If you are looking for a cleanup task, you can
> apply relevant patches from my series, starting with:
> https://patchwork.kernel.org/patch/9481237/
> (Leave the xfs patch [11/11] out)
>
> But besides verifying that patches still apply and build,
> you will need to address the concerns of fs maintainers.
> Take for example the btrfs patch:
> https://patchwork.kernel.org/patch/9480725/
>
> It says:
> + *
> + * Values 0..7 should match common file type values in file_type.h.
> */
> #define BTRFS_FT_UNKNOWN 0
> #define BTRFS_FT_REG_FILE 1
>
> But that is not enough.
> When converting code to use the generic defines FT_*, instead of
> filesystem defined we need to leave in the code build time assertions
> that will catch an attempt to change fs contancts in the future, e.g.:
>
> static inline u8 btrfs_inode_type(struct inode *inode)
> {
> - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT];
> + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN);
> + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE);
> ...
> + return fs_umode_to_ftype(inode->i_mode);
> }
>
> Same should be done for all relevant filesystems.
> Then you need to hope that fs maintainers will like this cleanup and
> want to take the patches ;-)
>
> Cheers,
> Amir.
Dear Amir,
I will give it a go and see how far I get :-)
Regards,
Phil
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Sun, Oct 21, 2018 at 02:02:57PM +0300, Amir Goldstein wrote:
> Yes. If you are looking for a cleanup task, you can
> apply relevant patches from my series, starting with:
> https://patchwork.kernel.org/patch/9481237/
> (Leave the xfs patch [11/11] out)
>
> But besides verifying that patches still apply and build,
> you will need to address the concerns of fs maintainers.
> Take for example the btrfs patch:
> https://patchwork.kernel.org/patch/9480725/
>
> It says:
> + *
> + * Values 0..7 should match common file type values in file_type.h.
> */
> #define BTRFS_FT_UNKNOWN 0
> #define BTRFS_FT_REG_FILE 1
>
> But that is not enough.
> When converting code to use the generic defines FT_*, instead of
> filesystem defined we need to leave in the code build time assertions
> that will catch an attempt to change fs contancts in the future, e.g.:
>
> static inline u8 btrfs_inode_type(struct inode *inode)
> {
> - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT];
> + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN);
> + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE);
> ...
> + return fs_umode_to_ftype(inode->i_mode);
> }
>
> Same should be done for all relevant filesystems.
> Then you need to hope that fs maintainers will like this cleanup and
> want to take the patches ;-)
>
> Cheers,
> Amir.
Dear Amir,
I will give it a go and see how far I get :-)
Regards,
Phil
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Sun, Oct 21, 2018 at 08:30:35AM +0300, Amir Goldstein wrote: > On Sun, Oct 21, 2018 at 1:27 AM Matthew Wilcox wrote: > > > > On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > > header and replace with simple assignment. For each case, S_IFx >> 12 > > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > > us the correct file type. It is expected that for *nix compatibility > > > reasons, the relation between S_IFx and DT_x will not change. For > > > cases where the mode is invalid, upper layer validation catches this > > > anyway, so this improves readability and arguably performance by > > > assigning (mode & S_IFMT) >> 12 directly without any jump table > > > or conditional logic. > > > > Shouldn't we also do this for other filesystems? A quick scan suggests > > someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, > > nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. > > > > I've tried to do that 2 years ago, not even for readability, but to > fix a lurking > bug in conversion table implementation that was copy from ext2 to > many other fs: > https://marc.info/?l=linux-fsdevel=148217829301701=2 > https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=b > > The push back from ext4/xfs maintainers was that while all fs use common > values to encode d_type in on disk format, those on-disk format values > are private to the fs. > > Eventually, the fix that got merged (only to xfs) did the opposite, it > converted > the conversion table lookup to a switch statement: > https://marc.info/?l=linux-fsdevel=14824386620=2 > > Some fs maintainers were happy about the initial version, but I did not > pursue pushing that cleanup: > https://marc.info/?l=linux-fsdevel=14824386620=2 > > Phillip, you are welcome to re-spin those patches if you like. > Note that the generic conversion helpers do not rely on upper layer > to catch invalid mode values. > > Cheers, > Amir. I only changed this code in the first place because of the comment in the code - I was looking for things to do basically, and this caught my eye because I play with FreeBSD now and then which of course uses UFS. I didn't consider any use to the other filesystems - by re-spin do you just mean fix up and make sure the kernel still builds etc? Regards, Phil
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Sun, Oct 21, 2018 at 08:30:35AM +0300, Amir Goldstein wrote: > On Sun, Oct 21, 2018 at 1:27 AM Matthew Wilcox wrote: > > > > On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > > header and replace with simple assignment. For each case, S_IFx >> 12 > > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > > us the correct file type. It is expected that for *nix compatibility > > > reasons, the relation between S_IFx and DT_x will not change. For > > > cases where the mode is invalid, upper layer validation catches this > > > anyway, so this improves readability and arguably performance by > > > assigning (mode & S_IFMT) >> 12 directly without any jump table > > > or conditional logic. > > > > Shouldn't we also do this for other filesystems? A quick scan suggests > > someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, > > nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. > > > > I've tried to do that 2 years ago, not even for readability, but to > fix a lurking > bug in conversion table implementation that was copy from ext2 to > many other fs: > https://marc.info/?l=linux-fsdevel=148217829301701=2 > https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=b > > The push back from ext4/xfs maintainers was that while all fs use common > values to encode d_type in on disk format, those on-disk format values > are private to the fs. > > Eventually, the fix that got merged (only to xfs) did the opposite, it > converted > the conversion table lookup to a switch statement: > https://marc.info/?l=linux-fsdevel=14824386620=2 > > Some fs maintainers were happy about the initial version, but I did not > pursue pushing that cleanup: > https://marc.info/?l=linux-fsdevel=14824386620=2 > > Phillip, you are welcome to re-spin those patches if you like. > Note that the generic conversion helpers do not rely on upper layer > to catch invalid mode values. > > Cheers, > Amir. I only changed this code in the first place because of the comment in the code - I was looking for things to do basically, and this caught my eye because I play with FreeBSD now and then which of course uses UFS. I didn't consider any use to the other filesystems - by re-spin do you just mean fix up and make sure the kernel still builds etc? Regards, Phil
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. It is expected that for *nix compatibility
reasons, the relation between S_IFx and DT_x will not change. For
cases where the mode is invalid, upper layer validation catches this
anyway, so this improves readability and arguably performance by
assigning (mode & S_IFMT) >> 12 directly without any jump table
or conditional logic.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.2
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. It is expected that for *nix compatibility
reasons, the relation between S_IFx and DT_x will not change. For
cases where the mode is invalid, upper layer validation catches this
anyway, so this improves readability and arguably performance by
assigning (mode & S_IFMT) >> 12 directly without any jump table
or conditional logic.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.2
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Thu, Oct 18, 2018 at 12:33:05AM +0100, Al Viro wrote: > They are. BSD folks had (sanely, IMO) put the 'type' bits of st_mode > into directory entry verbatim. Again, "symbolic constant" != "can be > expected to change"... If, e.g., some port decides to change S_IFIFO, > they'll have no end of fun accessing ext*, xfs, ufs, etc. since that > value is stored in the on-disk inode and in effect pinned down by > that. > > All S_... constants are universal and going to remain unchanged on > any Unices. Dear Al, Shall I resubmit without the compile-time checks in the latest patch then? Regards, Phil
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Thu, Oct 18, 2018 at 12:33:05AM +0100, Al Viro wrote: > They are. BSD folks had (sanely, IMO) put the 'type' bits of st_mode > into directory entry verbatim. Again, "symbolic constant" != "can be > expected to change"... If, e.g., some port decides to change S_IFIFO, > they'll have no end of fun accessing ext*, xfs, ufs, etc. since that > value is stored in the on-disk inode and in effect pinned down by > that. > > All S_... constants are universal and going to remain unchanged on > any Unices. Dear Al, Shall I resubmit without the compile-time checks in the latest patch then? Regards, Phil
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. This is verified at compile-time with
BUILD_BUG_ON macro calls. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 39 +++
1 file changed, 11 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..dcf86829edc2 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,16 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* Compile-time assertions that S_IFx >> S_SHIFT == DT_x */
+ BUILD_BUG_ON(S_IFIFO >> S_SHIFT != DT_FIFO);
+ BUILD_BUG_ON(S_IFCHR >> S_SHIFT != DT_CHR);
+ BUILD_BUG_ON(S_IFDIR >> S_SHIFT != DT_DIR);
+ BUILD_BUG_ON(S_IFBLK >> S_SHIFT != DT_BLK);
+ BUILD_BUG_ON(S_IFREG >> S_SHIFT != DT_REG);
+ BUILD_BUG_ON(S_IFLNK >> S_SHIFT != DT_LNK);
+ BUILD_BUG_ON(S_IFSOCK >> S_SHIFT != DT_SOCK);
+
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.2
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. This is verified at compile-time with
BUILD_BUG_ON macro calls. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 39 +++
1 file changed, 11 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..dcf86829edc2 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,16 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* Compile-time assertions that S_IFx >> S_SHIFT == DT_x */
+ BUILD_BUG_ON(S_IFIFO >> S_SHIFT != DT_FIFO);
+ BUILD_BUG_ON(S_IFCHR >> S_SHIFT != DT_CHR);
+ BUILD_BUG_ON(S_IFDIR >> S_SHIFT != DT_DIR);
+ BUILD_BUG_ON(S_IFBLK >> S_SHIFT != DT_BLK);
+ BUILD_BUG_ON(S_IFREG >> S_SHIFT != DT_REG);
+ BUILD_BUG_ON(S_IFLNK >> S_SHIFT != DT_LNK);
+ BUILD_BUG_ON(S_IFSOCK >> S_SHIFT != DT_SOCK);
+
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.2
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.0
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.0
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.0
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.0
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.0
[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h
header and replace with simple assignment. For each case, S_IFx >> 12
is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give
us the correct file type. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..7e0c0878b9f9 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -16,6 +16,7 @@
* some useful macros
*/
#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+#define S_SHIFT12
/*
* functions used for retyping
@@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct
ufs_dir_entry *de, int mode)
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
}
static inline u32
--
2.17.0
Re: [PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
On Tue, Oct 02, 2018 at 03:28:14AM +0100, Al Viro wrote:
> On Mon, Oct 01, 2018 at 04:33:10PM +0100, Phillip Potter wrote:
> > Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
> > table rather than a switch statement, as per the TODO comment.
>
> Brittle, that... Something like fs/ext2/dir.c approach (that is,
> #define S_SHIFT 12
> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
> [S_IFREG >> S_SHIFT]= EXT2_FT_REG_FILE,
> [S_IFDIR >> S_SHIFT]= EXT2_FT_DIR,
> [S_IFCHR >> S_SHIFT]= EXT2_FT_CHRDEV,
> [S_IFBLK >> S_SHIFT]= EXT2_FT_BLKDEV,
> [S_IFIFO >> S_SHIFT]= EXT2_FT_FIFO,
> [S_IFSOCK >> S_SHIFT] = EXT2_FT_SOCK,
> [S_IFLNK >> S_SHIFT]= EXT2_FT_SYMLINK,
> };
> in there) would be saner, IMO. Note that DT_UNKNOWN is zero, so the
> array elements lacking an explicit initializer will end up with that.
>
> What's more, the values are ->i_mode >> 12 or 0, depending upon the
> value being valid. And since the upper layers do validate the type,
> I'd consider simply using (inode->i_mode & S_IFMT) >> 12 there.
> Unlike ext2, ufs stores straight bits 12..15 there (ext2 uses an
> enum with sequential values instead; e.g. regular files are encoded
> as 1 there, not 8 as on ufs)...
Dear Al,
Thank you for taking time to offer your feedback, I really appreciate
it. I will rework this patch this evening and resubmit.
Regards,
Phil
Re: [PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
On Tue, Oct 02, 2018 at 03:28:14AM +0100, Al Viro wrote:
> On Mon, Oct 01, 2018 at 04:33:10PM +0100, Phillip Potter wrote:
> > Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
> > table rather than a switch statement, as per the TODO comment.
>
> Brittle, that... Something like fs/ext2/dir.c approach (that is,
> #define S_SHIFT 12
> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
> [S_IFREG >> S_SHIFT]= EXT2_FT_REG_FILE,
> [S_IFDIR >> S_SHIFT]= EXT2_FT_DIR,
> [S_IFCHR >> S_SHIFT]= EXT2_FT_CHRDEV,
> [S_IFBLK >> S_SHIFT]= EXT2_FT_BLKDEV,
> [S_IFIFO >> S_SHIFT]= EXT2_FT_FIFO,
> [S_IFSOCK >> S_SHIFT] = EXT2_FT_SOCK,
> [S_IFLNK >> S_SHIFT]= EXT2_FT_SYMLINK,
> };
> in there) would be saner, IMO. Note that DT_UNKNOWN is zero, so the
> array elements lacking an explicit initializer will end up with that.
>
> What's more, the values are ->i_mode >> 12 or 0, depending upon the
> value being valid. And since the upper layers do validate the type,
> I'd consider simply using (inode->i_mode & S_IFMT) >> 12 there.
> Unlike ext2, ufs stores straight bits 12..15 there (ext2 uses an
> enum with sequential values instead; e.g. regular files are encoded
> as 1 there, not 8 as on ufs)...
Dear Al,
Thank you for taking time to offer your feedback, I really appreciate
it. I will rework this patch this evening and resubmit.
Regards,
Phil
[PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
table rather than a switch statement, as per the TODO comment.
Signed-off-by: Phillip Potter
---
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1907be6d5808..1edf4c6454e3 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct
ufs_dir_entry *de, u16 value)
static inline void
ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
{
+ /* type lookup table, DT_UNKNOWN is default type if no case holds */
+ const int mode_table[] = {
+ DT_UNKNOWN,
+ DT_FIFO,/* mode & S_IFMT == S_IFIFO case */
+ DT_CHR, /* mode & S_IFMT == S_IFCHR case */
+ DT_UNKNOWN,
+ DT_DIR, /* mode & S_IFMT == S_IFDIR case */
+ DT_UNKNOWN,
+ DT_BLK, /* mode & S_IFMT == S_IFBLK case */
+ DT_UNKNOWN,
+ DT_REG, /* mode & S_IFMT == S_IFREG case */
+ DT_UNKNOWN,
+ DT_LNK, /* mode & S_IFMT == S_IFLNK case */
+ DT_UNKNOWN,
+ DT_SOCK,/* mode & S_IFMT == S_IFSOCK case */
+ DT_UNKNOWN,
+ DT_UNKNOWN,
+ DT_UNKNOWN
+ };
+
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* shift (mode & S_IFMT) right 12 bits to index into table */
+ de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
}
static inline u32
[PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
table rather than a switch statement, as per the TODO comment.
Signed-off-by: Phillip Potter
---
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1907be6d5808..1edf4c6454e3 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct
ufs_dir_entry *de, u16 value)
static inline void
ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
{
+ /* type lookup table, DT_UNKNOWN is default type if no case holds */
+ const int mode_table[] = {
+ DT_UNKNOWN,
+ DT_FIFO,/* mode & S_IFMT == S_IFIFO case */
+ DT_CHR, /* mode & S_IFMT == S_IFCHR case */
+ DT_UNKNOWN,
+ DT_DIR, /* mode & S_IFMT == S_IFDIR case */
+ DT_UNKNOWN,
+ DT_BLK, /* mode & S_IFMT == S_IFBLK case */
+ DT_UNKNOWN,
+ DT_REG, /* mode & S_IFMT == S_IFREG case */
+ DT_UNKNOWN,
+ DT_LNK, /* mode & S_IFMT == S_IFLNK case */
+ DT_UNKNOWN,
+ DT_SOCK,/* mode & S_IFMT == S_IFSOCK case */
+ DT_UNKNOWN,
+ DT_UNKNOWN,
+ DT_UNKNOWN
+ };
+
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* shift (mode & S_IFMT) right 12 bits to index into table */
+ de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
}
static inline u32
[PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
table rather than a switch statement, as per the TODO comment.
Signed-off-by: Phillip Potter <p...@philpotter.co.uk>
---
fs/ufs/util.h | 50 ++
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1907be6d5808..1edf4c6454e3 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct
ufs_dir_entry *de, u16 value)
static inline void
ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
{
+ /* type lookup table, DT_UNKNOWN is default type if no case holds */
+ const int mode_table[] = {
+ DT_UNKNOWN,
+ DT_FIFO,/* mode & S_IFMT == S_IFIFO case */
+ DT_CHR, /* mode & S_IFMT == S_IFCHR case */
+ DT_UNKNOWN,
+ DT_DIR, /* mode & S_IFMT == S_IFDIR case */
+ DT_UNKNOWN,
+ DT_BLK, /* mode & S_IFMT == S_IFBLK case */
+ DT_UNKNOWN,
+ DT_REG, /* mode & S_IFMT == S_IFREG case */
+ DT_UNKNOWN,
+ DT_LNK, /* mode & S_IFMT == S_IFLNK case */
+ DT_UNKNOWN,
+ DT_SOCK,/* mode & S_IFMT == S_IFSOCK case */
+ DT_UNKNOWN,
+ DT_UNKNOWN,
+ DT_UNKNOWN
+ };
+
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* shift (mode & S_IFMT) right 12 bits to index into table */
+ de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
}
static inline u32
--
2.14.3
[PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
table rather than a switch statement, as per the TODO comment.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 50 ++
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1907be6d5808..1edf4c6454e3 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct
ufs_dir_entry *de, u16 value)
static inline void
ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
{
+ /* type lookup table, DT_UNKNOWN is default type if no case holds */
+ const int mode_table[] = {
+ DT_UNKNOWN,
+ DT_FIFO,/* mode & S_IFMT == S_IFIFO case */
+ DT_CHR, /* mode & S_IFMT == S_IFCHR case */
+ DT_UNKNOWN,
+ DT_DIR, /* mode & S_IFMT == S_IFDIR case */
+ DT_UNKNOWN,
+ DT_BLK, /* mode & S_IFMT == S_IFBLK case */
+ DT_UNKNOWN,
+ DT_REG, /* mode & S_IFMT == S_IFREG case */
+ DT_UNKNOWN,
+ DT_LNK, /* mode & S_IFMT == S_IFLNK case */
+ DT_UNKNOWN,
+ DT_SOCK,/* mode & S_IFMT == S_IFSOCK case */
+ DT_UNKNOWN,
+ DT_UNKNOWN,
+ DT_UNKNOWN
+ };
+
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* shift (mode & S_IFMT) right 12 bits to index into table */
+ de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
}
static inline u32
--
2.14.3
[PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
table rather than a switch statement, as per the TODO comment.
Signed-off-by: Phillip Potter <p...@philpotter.co.uk>
---
fs/ufs/util.h | 50 ++
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1907be6d5808..1edf4c6454e3 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct
ufs_dir_entry *de, u16 value)
static inline void
ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
{
+ /* type lookup table, DT_UNKNOWN is default type if no case holds */
+ const int mode_table[] = {
+ DT_UNKNOWN,
+ DT_FIFO,/* mode & S_IFMT == S_IFIFO case */
+ DT_CHR, /* mode & S_IFMT == S_IFCHR case */
+ DT_UNKNOWN,
+ DT_DIR, /* mode & S_IFMT == S_IFDIR case */
+ DT_UNKNOWN,
+ DT_BLK, /* mode & S_IFMT == S_IFBLK case */
+ DT_UNKNOWN,
+ DT_REG, /* mode & S_IFMT == S_IFREG case */
+ DT_UNKNOWN,
+ DT_LNK, /* mode & S_IFMT == S_IFLNK case */
+ DT_UNKNOWN,
+ DT_SOCK,/* mode & S_IFMT == S_IFSOCK case */
+ DT_UNKNOWN,
+ DT_UNKNOWN,
+ DT_UNKNOWN
+ };
+
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* shift (mode & S_IFMT) right 12 bits to index into table */
+ de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
}
static inline u32
--
2.14.3
[PATCH] fs: ufs: Convert ufs_set_de_type to use lookup table
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup
table rather than a switch statement, as per the TODO comment.
Signed-off-by: Phillip Potter
---
fs/ufs/util.h | 50 ++
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1907be6d5808..1edf4c6454e3 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct
ufs_dir_entry *de, u16 value)
static inline void
ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
{
+ /* type lookup table, DT_UNKNOWN is default type if no case holds */
+ const int mode_table[] = {
+ DT_UNKNOWN,
+ DT_FIFO,/* mode & S_IFMT == S_IFIFO case */
+ DT_CHR, /* mode & S_IFMT == S_IFCHR case */
+ DT_UNKNOWN,
+ DT_DIR, /* mode & S_IFMT == S_IFDIR case */
+ DT_UNKNOWN,
+ DT_BLK, /* mode & S_IFMT == S_IFBLK case */
+ DT_UNKNOWN,
+ DT_REG, /* mode & S_IFMT == S_IFREG case */
+ DT_UNKNOWN,
+ DT_LNK, /* mode & S_IFMT == S_IFLNK case */
+ DT_UNKNOWN,
+ DT_SOCK,/* mode & S_IFMT == S_IFSOCK case */
+ DT_UNKNOWN,
+ DT_UNKNOWN,
+ DT_UNKNOWN
+ };
+
if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
return;
- /*
-* TODO turn this into a table lookup
-*/
- switch (mode & S_IFMT) {
- case S_IFSOCK:
- de->d_u.d_44.d_type = DT_SOCK;
- break;
- case S_IFLNK:
- de->d_u.d_44.d_type = DT_LNK;
- break;
- case S_IFREG:
- de->d_u.d_44.d_type = DT_REG;
- break;
- case S_IFBLK:
- de->d_u.d_44.d_type = DT_BLK;
- break;
- case S_IFDIR:
- de->d_u.d_44.d_type = DT_DIR;
- break;
- case S_IFCHR:
- de->d_u.d_44.d_type = DT_CHR;
- break;
- case S_IFIFO:
- de->d_u.d_44.d_type = DT_FIFO;
- break;
- default:
- de->d_u.d_44.d_type = DT_UNKNOWN;
- }
+ /* shift (mode & S_IFMT) right 12 bits to index into table */
+ de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
}
static inline u32
--
2.14.3
[PATCH] fs: ufs: small comment fix
Fixes a small style error in comment of s_u2_dsize member of
struct ufs_sb_private_info definition in fs/ufs/ufs_fs.h
Signed-off-by: Phillip Potter <p...@philpotter.co.uk>
---
--- a/fs/ufs/ufs_fs.h 2017-04-16 21:00:18.0 +0100
+++ b/fs/ufs/ufs_fs.h 2017-04-20 10:19:32.272298138 +0100
@@ -736,7 +736,7 @@ struct ufs_sb_private_info {
__u32 s_size; /* number of blocks (fragments) in fs */
__u32 s_dsize;/* number of data blocks in fs */
__u64 s_u2_size; /* ufs2: number of blocks (fragments) in fs */
- __u64 s_u2_dsize; /*ufs2: number of data blocks in fs */
+ __u64 s_u2_dsize; /* ufs2: number of data blocks in fs */
__u32 s_ncg; /* number of cylinder groups */
__u32 s_bsize;/* size of basic blocks */
__u32 s_fsize;/* size of fragments */
[PATCH] fs: ufs: small comment fix
Fixes a small style error in comment of s_u2_dsize member of
struct ufs_sb_private_info definition in fs/ufs/ufs_fs.h
Signed-off-by: Phillip Potter
---
--- a/fs/ufs/ufs_fs.h 2017-04-16 21:00:18.0 +0100
+++ b/fs/ufs/ufs_fs.h 2017-04-20 10:19:32.272298138 +0100
@@ -736,7 +736,7 @@ struct ufs_sb_private_info {
__u32 s_size; /* number of blocks (fragments) in fs */
__u32 s_dsize;/* number of data blocks in fs */
__u64 s_u2_size; /* ufs2: number of blocks (fragments) in fs */
- __u64 s_u2_dsize; /*ufs2: number of data blocks in fs */
+ __u64 s_u2_dsize; /* ufs2: number of data blocks in fs */
__u32 s_ncg; /* number of cylinder groups */
__u32 s_bsize;/* size of basic blocks */
__u32 s_fsize;/* size of fragments */
