| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAO9wTFgMCsS9L1PwpkDr48t9R4hO2UvFRwbPu2mMQMP0aqD+cQ@mail.gmail.com>
Date: Wed, 12 Mar 2025 23:58:37 +0530
From: Suchit K <suchitkarunakaran@...il.com>
To: Dave Kleikamp <dave.kleikamp@...cle.com>
Cc: jfs-discussion@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
skhan@...uxfoundation.org
Subject: Re: [PATCH] jfs: jfs_xtree: replace XT_GETPAGE macro with
xt_getpage() function
Hi Dave,
Thanks for the comments. I’m still a beginner so I wanted to share my
thoughts and check with you. Almost all references to the xt_getpage
function directly return the rc value obtained from it. I feel that
changing its return type to (xtpage_t *) might not be ideal, as it
would require modifying the code in multiple places. That said, if you
think changing the return value is the better approach, I’ll try to
change it. Also, I’ll update struct inode** ip to struct inode* ip.
Thanks once again.
On Tue, 11 Mar 2025 at 22:19, Dave Kleikamp <dave.kleikamp@...cle.com> wrote:
>
> On 3/2/25 1:15PM, Suchit Karunakaran wrote:
> > Replace legacy XT_GETPAGE macro with an inline function and update all
> > instances of XT_GETPAGE in jfs_xtree.c file to use the new function.
>
> I like the idea, but as long as we are changing this to a function, I'd
> like to simplify it to return the xtpage (xtpage_t *). A NULL return
> would indicate a failure.
>
> Also, the first argument should just be "struct inode *ip". That should
> eliminate your second patch.
> >
> > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@...il.com>
> > ---
> > fs/jfs/jfs_xtree.c | 86 ++++++++++++++++++++++++++++------------------
> > 1 file changed, 52 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/jfs/jfs_xtree.c b/fs/jfs/jfs_xtree.c
> > index 5ee618d17e77..fb736a06ea58 100644
> > --- a/fs/jfs/jfs_xtree.c
> > +++ b/fs/jfs/jfs_xtree.c
> > @@ -49,26 +49,6 @@
> >
> > #define XT_PAGE(IP, MP) BT_PAGE(IP, MP, xtpage_t, i_xtroot)
> >
> > -/* get page buffer for specified block address */
> > -/* ToDo: Replace this ugly macro with a function */
> > -#define XT_GETPAGE(IP, BN, MP, SIZE, P, RC) \
> > -do { \
> > - BT_GETPAGE(IP, BN, MP, xtpage_t, SIZE, P, RC, i_xtroot); \
> > - if (!(RC)) { \
> > - if ((le16_to_cpu((P)->header.nextindex) < XTENTRYSTART) || \
> > - (le16_to_cpu((P)->header.nextindex) > \
> > - le16_to_cpu((P)->header.maxentry)) || \
> > - (le16_to_cpu((P)->header.maxentry) > \
> > - (((BN) == 0) ? XTROOTMAXSLOT : PSIZE >> L2XTSLOTSIZE))) { \
> > - jfs_error((IP)->i_sb, \
> > - "XT_GETPAGE: xtree page corrupt\n"); \
> > - BT_PUTPAGE(MP); \
> > - MP = NULL; \
> > - RC = -EIO; \
> > - } \
> > - } \
> > -} while (0)
> > -
> > /* for consistency */
> > #define XT_PUTPAGE(MP) BT_PUTPAGE(MP)
> >
> > @@ -114,6 +94,44 @@ static int xtSplitPage(tid_t tid, struct inode *ip, struct xtsplit * split,
> > static int xtSplitRoot(tid_t tid, struct inode *ip,
> > struct xtsplit * split, struct metapage ** rmpp);
> >
> > +/*
> > + * xt_getpage()
> > + *
> > + * function: get the page buffer for a specified block address.
> > + *
> > + * parameters:
> > + * ip - pointer to the inode
> > + * bn - block number (s64) of the xtree page to be retrieved;
> > + * mp - pointer to a metapage pointer where the page buffer is returned;
> > + * size - size parameter to pass to BT_GETPAGE;
> > + * p - pointer to an xtpage_t pointer mapping the page's data.
> > + *
> > + * returns:
> > + * 0 on success, or -EIO if the page is corrupt or an error occurs.
> > + */
> > +
> > +static inline int xt_getpage(struct inode **ip, s64 bn, struct metapage **mp,
> > + unsigned int size, xtpage_t **p)
> > +{
> > + int rc;
> > +
> > + BT_GETPAGE(ip, bn, *mp, xtpage_t, size, *p, rc, i_xtroot);
> > +
> > + if (!rc) {
> > + if ((le16_to_cpu((*p)->header.nextindex) < XTENTRYSTART) ||
> > + (le16_to_cpu((*p)->header.nextindex) >
> > + le16_to_cpu((*p)->header.maxentry)) ||
> > + (le16_to_cpu((*p)->header.maxentry) >
> > + ((bn == 0) ? XTROOTMAXSLOT : PSIZE >> L2XTSLOTSIZE))) {
> > + jfs_error(ip->i_sb, "xt_getpage: xtree page corrupt\n");
> > + BT_PUTPAGE(*mp);
> > + *mp = NULL;
> > + rc = -EIO;
> > + }
> > + }
> > + return rc;
> > +}
> > +
> > /*
> > * xtLookup()
> > *
> > @@ -252,7 +270,7 @@ static int xtSearch(struct inode *ip, s64 xoff, s64 *nextp,
> > */
> > for (bn = 0;;) {
> > /* get/pin the page to search */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -807,7 +825,7 @@ xtSplitUp(tid_t tid,
> > * insert router entry in parent for new right child page <rp>
> > */
> > /* get/pin the parent page <sp> */
> > - XT_GETPAGE(ip, parent->bn, smp, PSIZE, sp, rc);
> > + rc = xt_getpage(ip, parent->bn, &smp, PSIZE, &sp);
> > if (rc) {
> > XT_PUTPAGE(rcmp);
> > return rc;
> > @@ -1062,7 +1080,7 @@ xtSplitPage(tid_t tid, struct inode *ip,
> > * update previous pointer of old next/right page of <sp>
> > */
> > if (nextbn != 0) {
> > - XT_GETPAGE(ip, nextbn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, nextbn, &mp, PSIZE, &p);
> > if (rc) {
> > XT_PUTPAGE(rmp);
> > goto clean_up;
> > @@ -1417,7 +1435,7 @@ int xtExtend(tid_t tid, /* transaction id */
> > return rc;
> >
> > /* get back old page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> > /*
> > @@ -1433,7 +1451,7 @@ int xtExtend(tid_t tid, /* transaction id */
> > XT_PUTPAGE(mp);
> >
> > /* get new child page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -1711,7 +1729,7 @@ int xtUpdate(tid_t tid, struct inode *ip, xad_t * nxad)
> > return rc;
> >
> > /* get back old page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> > /*
> > @@ -1727,7 +1745,7 @@ int xtUpdate(tid_t tid, struct inode *ip, xad_t * nxad)
> > XT_PUTPAGE(mp);
> >
> > /* get new child page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -1788,7 +1806,7 @@ int xtUpdate(tid_t tid, struct inode *ip, xad_t * nxad)
> > XT_PUTPAGE(mp);
> >
> > /* get new right page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -1864,7 +1882,7 @@ printf("xtUpdate.updateLeft.split p:0x%p\n", p);
> > return rc;
> >
> > /* get back old page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -1881,7 +1899,7 @@ printf("xtUpdate.updateLeft.split p:0x%p\n", p);
> > XT_PUTPAGE(mp);
> >
> > /* get new child page */
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -2268,7 +2286,7 @@ s64 xtTruncate(tid_t tid, struct inode *ip, s64 newsize, int flag)
> > * first access of each page:
> > */
> > getPage:
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -2506,7 +2524,7 @@ s64 xtTruncate(tid_t tid, struct inode *ip, s64 newsize, int flag)
> >
> > /* get back the parent page */
> > bn = parent->bn;
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -2791,7 +2809,7 @@ s64 xtTruncate_pmap(tid_t tid, struct inode *ip, s64 committed_size)
> > * first access of each page:
> > */
> > getPage:
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
> > @@ -2836,7 +2854,7 @@ s64 xtTruncate_pmap(tid_t tid, struct inode *ip, s64 committed_size)
> >
> > /* get back the parent page */
> > bn = parent->bn;
> > - XT_GETPAGE(ip, bn, mp, PSIZE, p, rc);
> > + rc = xt_getpage(ip, bn, &mp, PSIZE, &p);
> > if (rc)
> > return rc;
> >
>
Powered by blists - more mailing lists