| Message ID | 1525754479-12177-21-git-send-email-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 07, 2018 at 09:41:18PM -0700, Allison Henderson wrote: > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ > repair/attr_repair.c | 16 ++------------ Separate patches for separate utilities, please. > repair/phase6.c | 47 +++++++++++++++++++++++++---------------- > 3 files changed, 70 insertions(+), 52 deletions(-) > > diff --git a/mkfs/proto.c b/mkfs/proto.c > index 67c228a..3e4fd33 100644 > --- a/mkfs/proto.c > +++ b/mkfs/proto.c > @@ -19,6 +19,7 @@ > #include "libxfs.h" > #include <sys/stat.h> > #include "xfs_multidisk.h" > +#include "xfs_parent.h" > > /* > * Prototypes for internal functions. > @@ -318,23 +319,25 @@ newregfile( > > static void > newdirent( > - xfs_mount_t *mp, > - xfs_trans_t *tp, > - xfs_inode_t *pip, > - struct xfs_name *name, > - xfs_ino_t inum, > - xfs_fsblock_t *first, > - struct xfs_defer_ops *dfops) > + xfs_mount_t *mp, While you're changing these lines, get rid of the _t usage when appropriate. struct xfs_trans *tp, Here and elsewhere in the patch. > + xfs_trans_t *tp, > + xfs_inode_t *pip, > + struct xfs_name *name, > + struct xfs_inode *ip, > + xfs_fsblock_t *first, > + struct xfs_defer_ops *dfops, > + xfs_dir2_dataptr_t *offset) > { > - int error; > - int rsv; > + int error; > + int rsv; > > rsv = XFS_DIRENTER_SPACE_RES(mp, name->len); > > - error = -libxfs_dir_createname(tp, pip, name, inum, first, dfops, rsv, > - NULL); > + error = -libxfs_dir_createname(tp, pip, name, ip->i_ino, first, dfops, rsv, > + offset); > if (error) > fail(_("directory createname error"), error); > + > } > > static void > @@ -387,6 +390,7 @@ parseproto( > cred_t creds; > char *value; > struct xfs_name xname; > + xfs_dir2_dataptr_t offset; > > memset(&creds, 0, sizeof(creds)); > mstr = getstr(pp); > @@ -470,7 +474,7 @@ parseproto( > free(buf); > libxfs_trans_ijoin(tp, pip, 0); > xname.type = XFS_DIR3_FT_REG_FILE; > - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); > break; > > case IF_RESERVED: /* pre-allocated space only */ > @@ -493,7 +497,7 @@ parseproto( > libxfs_trans_ijoin(tp, pip, 0); > > xname.type = XFS_DIR3_FT_REG_FILE; > - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); > libxfs_trans_log_inode(tp, ip, flags); > > libxfs_defer_ijoin(&dfops, ip); > @@ -516,7 +520,7 @@ parseproto( > } > libxfs_trans_ijoin(tp, pip, 0); > xname.type = XFS_DIR3_FT_BLKDEV; > - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); > flags |= XFS_ILOG_DEV; > break; > > @@ -530,7 +534,7 @@ parseproto( > fail(_("Inode allocation failed"), error); > libxfs_trans_ijoin(tp, pip, 0); > xname.type = XFS_DIR3_FT_CHRDEV; > - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); > flags |= XFS_ILOG_DEV; > break; > > @@ -542,7 +546,7 @@ parseproto( > fail(_("Inode allocation failed"), error); > libxfs_trans_ijoin(tp, pip, 0); > xname.type = XFS_DIR3_FT_FIFO; > - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); > break; > case IF_SYMLINK: > buf = getstr(pp); > @@ -555,7 +559,7 @@ parseproto( > flags |= newfile(tp, ip, &dfops, &first, 1, 1, buf, len); > libxfs_trans_ijoin(tp, pip, 0); > xname.type = XFS_DIR3_FT_SYMLINK; > - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); > break; > case IF_DIRECTORY: > tp = getres(mp, 0); > @@ -572,8 +576,8 @@ parseproto( > } else { > libxfs_trans_ijoin(tp, pip, 0); > xname.type = XFS_DIR3_FT_DIR; > - newdirent(mp, tp, pip, &xname, ip->i_ino, > - &first, &dfops); > + newdirent(mp, tp, pip, &xname, ip, > + &first, &dfops, &offset); > inc_nlink(VFS_I(pip)); > libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE); > } > @@ -612,8 +616,23 @@ parseproto( > fail(_("Error encountered creating file from prototype file"), > error); > } > - libxfs_trans_commit(tp); > + libxfs_trans_commit(tp); > + > + if (xfs_sb_version_hasparent(&mp->m_sb)) { > + error = xfs_parent_add(tp, pip, ip, &xname, offset, &first, &dfops); Definitely can't keep using tp after committing the transaction. > + if (error) > + fail(_("Error creating parent pointer"), error); > + > + libxfs_trans_log_inode(tp, ip, flags); > + libxfs_defer_ijoin(&dfops, ip); > + error = -libxfs_defer_finish(&tp, &dfops); > + if (error) > + fail(_("Directory creation failed"), error); > + libxfs_trans_commit(tp); Also, if you add libxfs_trans_commit calls they probably ought to have return values checked in case some day libxfs_trans_commit starts returning error codes. > + } > + > IRELE(ip); > + > } > > void > diff --git a/repair/attr_repair.c b/repair/attr_repair.c > index 8b1b8a7..e5ff3b0 100644 > --- a/repair/attr_repair.c > +++ b/repair/attr_repair.c > @@ -305,16 +305,6 @@ process_shortform_attr( > } > } > > - /* namecheck checks for / and null terminated for file names. > - * attributes names currently follow the same rules. > - */ > - if (namecheck((char *)¤tentry->nameval[0], Why remove the namecheck calls? It's only the ATTR_PARENT attributes where we want to skip the null-in-name checks, right? > - currententry->namelen)) { > - do_warn( > - _("entry contains illegal character in shortform attribute name\n")); > - junkit = 1; > - } > - > if (currententry->flags & XFS_ATTR_INCOMPLETE) { > do_warn( > _("entry has INCOMPLETE flag on in shortform attribute\n")); > @@ -470,8 +460,7 @@ process_leaf_attr_local( > xfs_attr_leaf_name_local_t *local; > > local = xfs_attr3_leaf_name_local(leaf, i); > - if (local->namelen == 0 || namecheck((char *)&local->nameval[0], > - local->namelen)) { > + if (local->namelen == 0) { > do_warn( > _("attribute entry %d in attr block %u, inode %" PRIu64 " has bad name (namelen = %d)\n"), > i, da_bno, ino, local->namelen); > @@ -525,8 +514,7 @@ process_leaf_attr_remote( > > remotep = xfs_attr3_leaf_name_remote(leaf, i); > > - if (remotep->namelen == 0 || namecheck((char *)&remotep->name[0], > - remotep->namelen) || > + if (remotep->namelen == 0 || > be32_to_cpu(entry->hashval) != > libxfs_da_hashname((unsigned char *)&remotep->name[0], > remotep->namelen) || > diff --git a/repair/phase6.c b/repair/phase6.c > index 4fedb35..b861b3b 100644 > --- a/repair/phase6.c > +++ b/repair/phase6.c > @@ -29,6 +29,7 @@ > #include "dinode.h" > #include "progress.h" > #include "versions.h" > +#include "xfs_parent.h" > > static struct cred zerocr; > static struct fsxattr zerofsx; > @@ -962,19 +963,20 @@ mk_root_dir(xfs_mount_t *mp) > static xfs_ino_t > mk_orphanage(xfs_mount_t *mp) > { > - xfs_ino_t ino; > - xfs_trans_t *tp; > - xfs_inode_t *ip; > - xfs_inode_t *pip; > - xfs_fsblock_t first; > - ino_tree_node_t *irec; > - int ino_offset = 0; > - int i; > - int error; > + xfs_ino_t ino; > + xfs_trans_t *tp; > + xfs_inode_t *ip; > + xfs_inode_t *pip; > + xfs_fsblock_t first; > + ino_tree_node_t *irec; > + int ino_offset = 0; > + int i; > + int error; > struct xfs_defer_ops dfops; > - const int mode = 0755; > - int nres; > - struct xfs_name xname; > + const int mode = 0755; > + int nres; > + struct xfs_name xname; > + xfs_dir2_dataptr_t offset; > > /* > * check for an existing lost+found first, if it exists, return > @@ -1061,7 +1063,7 @@ mk_orphanage(xfs_mount_t *mp) > * create the actual entry > */ > error = -libxfs_dir_createname(tp, pip, &xname, ip->i_ino, &first, > - &dfops, nres, NULL); > + &dfops, nres, &offset); > if (error) > do_error( > _("can't make %s, createname error %d\n"), > @@ -1089,6 +1091,14 @@ mk_orphanage(xfs_mount_t *mp) > ORPHANAGE, error); > } > > + libxfs_trans_commit(tp); > + if (xfs_sb_version_hasparent(&mp->m_sb)) { > + error = xfs_parent_add(tp, pip, ip, &xname, offset, When you want to use libxfs functions from outside of libxfs (namely within mkfs/repair/whatever) the convention is to add a #define in libxfs_api_defs.h: #define libxfs_parent_add xfs_parent_add And then in the utility program the usage should be: error = -libxfs_parent_add(...); Because libxfs functions return negative numbers for errors (kernel style) but the rest of the C world expects positive error codes. In theory xfs/437 should complain about this (though for all I know it could be busted). --D > + &first, &dfops); > + if (error) > + do_error(_("Error creating parent pointer: %d\n"), > + error); > + } > > libxfs_trans_commit(tp); > IRELE(ip); > @@ -1120,6 +1130,7 @@ mv_orphanage( > ino_tree_node_t *irec; > int ino_offset = 0; > struct xfs_name xname; > + xfs_dir2_dataptr_t offset; > > xname.name = fname; > xname.len = snprintf((char *)fname, sizeof(fname), "%llu", > @@ -1170,7 +1181,7 @@ mv_orphanage( > > libxfs_defer_init(&dfops, &first); > err = -libxfs_dir_createname(tp, orphanage_ip, &xname, > - ino, &first, &dfops, nres, NULL); > + ino, &first, &dfops, nres, &offset); > if (err) > do_error( > _("name create failed in %s (%d), filesystem may be out of space\n"), > @@ -1183,7 +1194,7 @@ mv_orphanage( > libxfs_trans_log_inode(tp, orphanage_ip, XFS_ILOG_CORE); > > err = -libxfs_dir_createname(tp, ino_p, &xfs_name_dotdot, > - orphanage_ino, &first, &dfops, nres, NULL); > + orphanage_ino, &first, &dfops, nres, &offset); > if (err) > do_error( > _("creation of .. entry failed (%d), filesystem may be out of space\n"), > @@ -1214,7 +1225,7 @@ mv_orphanage( > libxfs_defer_init(&dfops, &first); > > err = -libxfs_dir_createname(tp, orphanage_ip, &xname, > - ino, &first, &dfops, nres, NULL); > + ino, &first, &dfops, nres, &offset); > if (err) > do_error( > _("name create failed in %s (%d), filesystem may be out of space\n"), > @@ -1233,7 +1244,7 @@ mv_orphanage( > if (entry_ino_num != orphanage_ino) { > err = -libxfs_dir_replace(tp, ino_p, > &xfs_name_dotdot, orphanage_ino, > - &first, &dfops, nres, NULL); > + &first, &dfops, nres, &offset); > if (err) > do_error( > _("name replace op failed (%d), filesystem may be out of space\n"), > @@ -1270,7 +1281,7 @@ mv_orphanage( > > libxfs_defer_init(&dfops, &first); > err = -libxfs_dir_createname(tp, orphanage_ip, &xname, ino, > - &first, &dfops, nres, NULL); > + &first, &dfops, nres, &offset); > if (err) > do_error( > _("name create failed in %s (%d), filesystem may be out of space\n"), > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/08/2018 10:43 AM, Darrick J. Wong wrote: > On Mon, May 07, 2018 at 09:41:18PM -0700, Allison Henderson wrote: >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ >> repair/attr_repair.c | 16 ++------------ > Separate patches for separate utilities, please. > >> repair/phase6.c | 47 +++++++++++++++++++++++++---------------- >> 3 files changed, 70 insertions(+), 52 deletions(-) >> >> diff --git a/mkfs/proto.c b/mkfs/proto.c >> index 67c228a..3e4fd33 100644 >> --- a/mkfs/proto.c >> +++ b/mkfs/proto.c >> @@ -19,6 +19,7 @@ >> #include "libxfs.h" >> #include <sys/stat.h> >> #include "xfs_multidisk.h" >> +#include "xfs_parent.h" >> >> /* >> * Prototypes for internal functions. >> @@ -318,23 +319,25 @@ newregfile( >> >> static void >> newdirent( >> - xfs_mount_t *mp, >> - xfs_trans_t *tp, >> - xfs_inode_t *pip, >> - struct xfs_name *name, >> - xfs_ino_t inum, >> - xfs_fsblock_t *first, >> - struct xfs_defer_ops *dfops) >> + xfs_mount_t *mp, > While you're changing these lines, get rid of the _t usage when > appropriate. > > struct xfs_trans *tp, > > Here and elsewhere in the patch. Sure, will do > >> + xfs_trans_t *tp, >> + xfs_inode_t *pip, >> + struct xfs_name *name, >> + struct xfs_inode *ip, >> + xfs_fsblock_t *first, >> + struct xfs_defer_ops *dfops, >> + xfs_dir2_dataptr_t *offset) >> { >> - int error; >> - int rsv; >> + int error; >> + int rsv; >> >> rsv = XFS_DIRENTER_SPACE_RES(mp, name->len); >> >> - error = -libxfs_dir_createname(tp, pip, name, inum, first, dfops, rsv, >> - NULL); >> + error = -libxfs_dir_createname(tp, pip, name, ip->i_ino, first, dfops, rsv, >> + offset); >> if (error) >> fail(_("directory createname error"), error); >> + >> } >> >> static void >> @@ -387,6 +390,7 @@ parseproto( >> cred_t creds; >> char *value; >> struct xfs_name xname; >> + xfs_dir2_dataptr_t offset; >> >> memset(&creds, 0, sizeof(creds)); >> mstr = getstr(pp); >> @@ -470,7 +474,7 @@ parseproto( >> free(buf); >> libxfs_trans_ijoin(tp, pip, 0); >> xname.type = XFS_DIR3_FT_REG_FILE; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); >> break; >> >> case IF_RESERVED: /* pre-allocated space only */ >> @@ -493,7 +497,7 @@ parseproto( >> libxfs_trans_ijoin(tp, pip, 0); >> >> xname.type = XFS_DIR3_FT_REG_FILE; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); >> libxfs_trans_log_inode(tp, ip, flags); >> >> libxfs_defer_ijoin(&dfops, ip); >> @@ -516,7 +520,7 @@ parseproto( >> } >> libxfs_trans_ijoin(tp, pip, 0); >> xname.type = XFS_DIR3_FT_BLKDEV; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); >> flags |= XFS_ILOG_DEV; >> break; >> >> @@ -530,7 +534,7 @@ parseproto( >> fail(_("Inode allocation failed"), error); >> libxfs_trans_ijoin(tp, pip, 0); >> xname.type = XFS_DIR3_FT_CHRDEV; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); >> flags |= XFS_ILOG_DEV; >> break; >> >> @@ -542,7 +546,7 @@ parseproto( >> fail(_("Inode allocation failed"), error); >> libxfs_trans_ijoin(tp, pip, 0); >> xname.type = XFS_DIR3_FT_FIFO; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); >> break; >> case IF_SYMLINK: >> buf = getstr(pp); >> @@ -555,7 +559,7 @@ parseproto( >> flags |= newfile(tp, ip, &dfops, &first, 1, 1, buf, len); >> libxfs_trans_ijoin(tp, pip, 0); >> xname.type = XFS_DIR3_FT_SYMLINK; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); >> break; >> case IF_DIRECTORY: >> tp = getres(mp, 0); >> @@ -572,8 +576,8 @@ parseproto( >> } else { >> libxfs_trans_ijoin(tp, pip, 0); >> xname.type = XFS_DIR3_FT_DIR; >> - newdirent(mp, tp, pip, &xname, ip->i_ino, >> - &first, &dfops); >> + newdirent(mp, tp, pip, &xname, ip, >> + &first, &dfops, &offset); >> inc_nlink(VFS_I(pip)); >> libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE); >> } >> @@ -612,8 +616,23 @@ parseproto( >> fail(_("Error encountered creating file from prototype file"), >> error); >> } >> - libxfs_trans_commit(tp); >> + libxfs_trans_commit(tp); >> + >> + if (xfs_sb_version_hasparent(&mp->m_sb)) { >> + error = xfs_parent_add(tp, pip, ip, &xname, offset, &first, &dfops); > Definitely can't keep using tp after committing the transaction. > >> + if (error) >> + fail(_("Error creating parent pointer"), error); >> + >> + libxfs_trans_log_inode(tp, ip, flags); >> + libxfs_defer_ijoin(&dfops, ip); >> + error = -libxfs_defer_finish(&tp, &dfops); >> + if (error) >> + fail(_("Directory creation failed"), error); >> + libxfs_trans_commit(tp); > Also, if you add libxfs_trans_commit calls they probably ought to have > return values checked in case some day libxfs_trans_commit starts > returning error codes. Alrighty, will fix >> + } >> + >> IRELE(ip); >> + >> } >> >> void >> diff --git a/repair/attr_repair.c b/repair/attr_repair.c >> index 8b1b8a7..e5ff3b0 100644 >> --- a/repair/attr_repair.c >> +++ b/repair/attr_repair.c >> @@ -305,16 +305,6 @@ process_shortform_attr( >> } >> } >> >> - /* namecheck checks for / and null terminated for file names. >> - * attributes names currently follow the same rules. >> - */ >> - if (namecheck((char *)¤tentry->nameval[0], > Why remove the namecheck calls? It's only the ATTR_PARENT attributes > where we want to skip the null-in-name checks, right? Oh, I had forgotten about this. I'll add something to make it exclude only pptrs > >> - currententry->namelen)) { >> - do_warn( >> - _("entry contains illegal character in shortform attribute name\n")); >> - junkit = 1; >> - } >> - >> if (currententry->flags & XFS_ATTR_INCOMPLETE) { >> do_warn( >> _("entry has INCOMPLETE flag on in shortform attribute\n")); >> @@ -470,8 +460,7 @@ process_leaf_attr_local( >> xfs_attr_leaf_name_local_t *local; >> >> local = xfs_attr3_leaf_name_local(leaf, i); >> - if (local->namelen == 0 || namecheck((char *)&local->nameval[0], >> - local->namelen)) { >> + if (local->namelen == 0) { >> do_warn( >> _("attribute entry %d in attr block %u, inode %" PRIu64 " has bad name (namelen = %d)\n"), >> i, da_bno, ino, local->namelen); >> @@ -525,8 +514,7 @@ process_leaf_attr_remote( >> >> remotep = xfs_attr3_leaf_name_remote(leaf, i); >> >> - if (remotep->namelen == 0 || namecheck((char *)&remotep->name[0], >> - remotep->namelen) || >> + if (remotep->namelen == 0 || >> be32_to_cpu(entry->hashval) != >> libxfs_da_hashname((unsigned char *)&remotep->name[0], >> remotep->namelen) || >> diff --git a/repair/phase6.c b/repair/phase6.c >> index 4fedb35..b861b3b 100644 >> --- a/repair/phase6.c >> +++ b/repair/phase6.c >> @@ -29,6 +29,7 @@ >> #include "dinode.h" >> #include "progress.h" >> #include "versions.h" >> +#include "xfs_parent.h" >> >> static struct cred zerocr; >> static struct fsxattr zerofsx; >> @@ -962,19 +963,20 @@ mk_root_dir(xfs_mount_t *mp) >> static xfs_ino_t >> mk_orphanage(xfs_mount_t *mp) >> { >> - xfs_ino_t ino; >> - xfs_trans_t *tp; >> - xfs_inode_t *ip; >> - xfs_inode_t *pip; >> - xfs_fsblock_t first; >> - ino_tree_node_t *irec; >> - int ino_offset = 0; >> - int i; >> - int error; >> + xfs_ino_t ino; >> + xfs_trans_t *tp; >> + xfs_inode_t *ip; >> + xfs_inode_t *pip; >> + xfs_fsblock_t first; >> + ino_tree_node_t *irec; >> + int ino_offset = 0; >> + int i; >> + int error; >> struct xfs_defer_ops dfops; >> - const int mode = 0755; >> - int nres; >> - struct xfs_name xname; >> + const int mode = 0755; >> + int nres; >> + struct xfs_name xname; >> + xfs_dir2_dataptr_t offset; >> >> /* >> * check for an existing lost+found first, if it exists, return >> @@ -1061,7 +1063,7 @@ mk_orphanage(xfs_mount_t *mp) >> * create the actual entry >> */ >> error = -libxfs_dir_createname(tp, pip, &xname, ip->i_ino, &first, >> - &dfops, nres, NULL); >> + &dfops, nres, &offset); >> if (error) >> do_error( >> _("can't make %s, createname error %d\n"), >> @@ -1089,6 +1091,14 @@ mk_orphanage(xfs_mount_t *mp) >> ORPHANAGE, error); >> } >> >> + libxfs_trans_commit(tp); >> + if (xfs_sb_version_hasparent(&mp->m_sb)) { >> + error = xfs_parent_add(tp, pip, ip, &xname, offset, > When you want to use libxfs functions from outside of libxfs (namely > within mkfs/repair/whatever) the convention is to add a #define in > libxfs_api_defs.h: > > #define libxfs_parent_add xfs_parent_add > > And then in the utility program the usage should be: > > error = -libxfs_parent_add(...); > > Because libxfs functions return negative numbers for errors (kernel > style) but the rest of the C world expects positive error codes. > > In theory xfs/437 should complain about this (though for all I know it > could be busted). > > --D I see, I will get libxfs_api_defs updated then. Thx for the review! Allison >> + &first, &dfops); >> + if (error) >> + do_error(_("Error creating parent pointer: %d\n"), >> + error); >> + } >> >> libxfs_trans_commit(tp); >> IRELE(ip); >> @@ -1120,6 +1130,7 @@ mv_orphanage( >> ino_tree_node_t *irec; >> int ino_offset = 0; >> struct xfs_name xname; >> + xfs_dir2_dataptr_t offset; >> >> xname.name = fname; >> xname.len = snprintf((char *)fname, sizeof(fname), "%llu", >> @@ -1170,7 +1181,7 @@ mv_orphanage( >> >> libxfs_defer_init(&dfops, &first); >> err = -libxfs_dir_createname(tp, orphanage_ip, &xname, >> - ino, &first, &dfops, nres, NULL); >> + ino, &first, &dfops, nres, &offset); >> if (err) >> do_error( >> _("name create failed in %s (%d), filesystem may be out of space\n"), >> @@ -1183,7 +1194,7 @@ mv_orphanage( >> libxfs_trans_log_inode(tp, orphanage_ip, XFS_ILOG_CORE); >> >> err = -libxfs_dir_createname(tp, ino_p, &xfs_name_dotdot, >> - orphanage_ino, &first, &dfops, nres, NULL); >> + orphanage_ino, &first, &dfops, nres, &offset); >> if (err) >> do_error( >> _("creation of .. entry failed (%d), filesystem may be out of space\n"), >> @@ -1214,7 +1225,7 @@ mv_orphanage( >> libxfs_defer_init(&dfops, &first); >> >> err = -libxfs_dir_createname(tp, orphanage_ip, &xname, >> - ino, &first, &dfops, nres, NULL); >> + ino, &first, &dfops, nres, &offset); >> if (err) >> do_error( >> _("name create failed in %s (%d), filesystem may be out of space\n"), >> @@ -1233,7 +1244,7 @@ mv_orphanage( >> if (entry_ino_num != orphanage_ino) { >> err = -libxfs_dir_replace(tp, ino_p, >> &xfs_name_dotdot, orphanage_ino, >> - &first, &dfops, nres, NULL); >> + &first, &dfops, nres, &offset); >> if (err) >> do_error( >> _("name replace op failed (%d), filesystem may be out of space\n"), >> @@ -1270,7 +1281,7 @@ mv_orphanage( >> >> libxfs_defer_init(&dfops, &first); >> err = -libxfs_dir_createname(tp, orphanage_ip, &xname, ino, >> - &first, &dfops, nres, NULL); >> + &first, &dfops, nres, &offset); >> if (err) >> do_error( >> _("name create failed in %s (%d), filesystem may be out of space\n"), >> -- >> 2.7.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/8/18 12:43 PM, Darrick J. Wong wrote: > On Mon, May 07, 2018 at 09:41:18PM -0700, Allison Henderson wrote: >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ >> repair/attr_repair.c | 16 ++------------ >> repair/phase6.c | 47 +++++++++++++++++++++++++---------------- > Separate patches for separate utilities, please. Or at least patch per functional change; it's not at all clear to me how protofile creation has anything to do with xfs_repair, and there's nothing in the commit log to help me figure it out. :) (I think it doesn't have anything to do w/ protofile creation, and the repair patches are fixing up orphanage entry creation; the attr_repair changes seem like some third thing that I don't grok yet, sorry - removing namecheck() for some reason?). -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/08/2018 01:39 PM, Eric Sandeen wrote: > > > On 5/8/18 12:43 PM, Darrick J. Wong wrote: >> On Mon, May 07, 2018 at 09:41:18PM -0700, Allison Henderson wrote: >>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>> --- >>> mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ >>> repair/attr_repair.c | 16 ++------------ >>> repair/phase6.c | 47 +++++++++++++++++++++++++---------------- > >> Separate patches for separate utilities, please. > > Or at least patch per functional change; it's not at all clear to me how protofile > creation has anything to do with xfs_repair, and there's nothing in the commit > log to help me figure it out. :) > > (I think it doesn't have anything to do w/ protofile creation, and the repair > patches are fixing up orphanage entry creation; the attr_repair changes seem > like some third thing that I don't grok yet, sorry - removing namecheck() for > some reason?). > > -Eric Ok, it took me a minute to remember why I did this too. We had ended up adding a parameter to libxfs_dir_createname to get the offset needed for creating the parent pointer in the protofile, and I ended up following the compiler errors back around the repair code that uses the same function calls as well. Maybe I should make a separate patch just for the offset like the kernel space set does, and then have two smaller patches for the protofile and repair code. Allison > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIDaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=RAhvaxRO4Y3BuEWfKCiKHMUeNpZj53frHeTCh1R64dU&s=KTNiitB11SrpPwMvBXEQzon_G1JLFHAd69coCAKx8_Y&e= > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/8/18 4:14 PM, Allison Henderson wrote: > > > On 05/08/2018 01:39 PM, Eric Sandeen wrote: >> >> >> On 5/8/18 12:43 PM, Darrick J. Wong wrote: >>> On Mon, May 07, 2018 at 09:41:18PM -0700, Allison Henderson wrote: >>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>>> --- >>>> mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ >>>> repair/attr_repair.c | 16 ++------------ >>>> repair/phase6.c | 47 +++++++++++++++++++++++++---------------- >> >>> Separate patches for separate utilities, please. >> >> Or at least patch per functional change; it's not at all clear to me how protofile >> creation has anything to do with xfs_repair, and there's nothing in the commit >> log to help me figure it out. :) >> >> (I think it doesn't have anything to do w/ protofile creation, and the repair >> patches are fixing up orphanage entry creation; the attr_repair changes seem >> like some third thing that I don't grok yet, sorry - removing namecheck() for >> some reason?). >> >> -Eric > > Ok, it took me a minute to remember why I did this too. We had ended up adding a parameter to libxfs_dir_createname to get the offset needed for creating the parent pointer in the protofile, and I ended up following the compiler errors back around the repair code that uses the same function calls as well. Except that this patch isn't fixing the parameter count, it's changing it from NULL, right? @@ -1170,7 +1181,7 @@ mv_orphanage( libxfs_defer_init(&dfops, &first); err = -libxfs_dir_createname(tp, orphanage_ip, &xname, - ino, &first, &dfops, nres, NULL); + ino, &first, &dfops, nres, &offset); ...I wonder if there's much hope for doing this series in a bisectable way. :) > Maybe I should make a separate patch just for the offset like the kernel space set does, and then have two smaller patches for the protofile and repair code. That might be good. Any hint about what the attr_repair.c changes are? Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/08/2018 02:17 PM, Eric Sandeen wrote: > > > On 5/8/18 4:14 PM, Allison Henderson wrote: >> >> >> On 05/08/2018 01:39 PM, Eric Sandeen wrote: >>> >>> >>> On 5/8/18 12:43 PM, Darrick J. Wong wrote: >>>> On Mon, May 07, 2018 at 09:41:18PM -0700, Allison Henderson wrote: >>>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>>>> --- >>>>> mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ >>>>> repair/attr_repair.c | 16 ++------------ >>>>> repair/phase6.c | 47 +++++++++++++++++++++++++---------------- >>> >>>> Separate patches for separate utilities, please. >>> >>> Or at least patch per functional change; it's not at all clear to me how protofile >>> creation has anything to do with xfs_repair, and there's nothing in the commit >>> log to help me figure it out. :) >>> >>> (I think it doesn't have anything to do w/ protofile creation, and the repair >>> patches are fixing up orphanage entry creation; the attr_repair changes seem >>> like some third thing that I don't grok yet, sorry - removing namecheck() for >>> some reason?). >>> >>> -Eric >> >> Ok, it took me a minute to remember why I did this too. We had ended up adding a parameter to libxfs_dir_createname to get the offset needed for creating the parent pointer in the protofile, and I ended up following the compiler errors back around the repair code that uses the same function calls as well. > > Except that this patch isn't fixing the parameter count, it's changing it from NULL, right? > > @@ -1170,7 +1181,7 @@ mv_orphanage( > > libxfs_defer_init(&dfops, &first); > err = -libxfs_dir_createname(tp, orphanage_ip, &xname, > - ino, &first, &dfops, nres, NULL); > + ino, &first, &dfops, nres, &offset); > > > ...I wonder if there's much hope for doing this series in a bisectable way. :) > >> Maybe I should make a separate patch just for the offset like the kernel space set does, and then have two smaller patches for the protofile and repair code. > > That might be good. Any hint about what the attr_repair.c changes are? > > Thanks, > -Eric > Oh, you're right. I probably just tracked it down then. Reasoning that anything using it would likely need the pptr update to go along with it. It can still be a separate patch though. The changes in repair/attr_repair.c are because the parent pointer names are not strings, they are the binary {ino, gen, diroffset}. So it doesnt make sense to run a name check on them. I'll try to get something added to only skip the check for pptrs though. Allison -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/8/18 4:57 PM, Allison Henderson wrote: > On 05/08/2018 02:17 PM, Eric Sandeen wrote: ... >>> Ok, it took me a minute to remember why I did this too. We had ended up adding a parameter to libxfs_dir_createname to get the offset needed for creating the parent pointer in the protofile, and I ended up following the compiler errors back around the repair code that uses the same function calls as well. >> >> Except that this patch isn't fixing the parameter count, it's changing it from NULL, right? >> >> @@ -1170,7 +1181,7 @@ mv_orphanage( >> libxfs_defer_init(&dfops, &first); >> err = -libxfs_dir_createname(tp, orphanage_ip, &xname, >> - ino, &first, &dfops, nres, NULL); >> + ino, &first, &dfops, nres, &offset); >> >> >> ...I wonder if there's much hope for doing this series in a bisectable way. :) >> >>> Maybe I should make a separate patch just for the offset like the kernel space set does, and then have two smaller patches for the protofile and repair code. >> >> That might be good. Any hint about what the attr_repair.c changes are? >> >> Thanks, >> -Eric >> > Oh, you're right. I probably just tracked it down then. Reasoning that anything using it would likely need the pptr update to go along with it. It can still be a separate patch though. It's fine by me if the commit is something like "xfsprogs: update parent pointers in mkfs and repair" It's just confusing to have a terse commit log stating that it does one thing, and in fact it is doing 3 unrelated things. > The changes in repair/attr_repair.c are because the parent pointer names are not strings, they are the binary {ino, gen, diroffset}. So it doesnt make sense to run a name check on them. I'll try to get something added to only skip the check for pptrs though. Ah, that makes sense (and sounds necessary) - we can't skip the check on every attribute name just because it might possibly be a parent pointer. Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/mkfs/proto.c b/mkfs/proto.c index 67c228a..3e4fd33 100644 --- a/mkfs/proto.c +++ b/mkfs/proto.c @@ -19,6 +19,7 @@ #include "libxfs.h" #include <sys/stat.h> #include "xfs_multidisk.h" +#include "xfs_parent.h" /* * Prototypes for internal functions. @@ -318,23 +319,25 @@ newregfile( static void newdirent( - xfs_mount_t *mp, - xfs_trans_t *tp, - xfs_inode_t *pip, - struct xfs_name *name, - xfs_ino_t inum, - xfs_fsblock_t *first, - struct xfs_defer_ops *dfops) + xfs_mount_t *mp, + xfs_trans_t *tp, + xfs_inode_t *pip, + struct xfs_name *name, + struct xfs_inode *ip, + xfs_fsblock_t *first, + struct xfs_defer_ops *dfops, + xfs_dir2_dataptr_t *offset) { - int error; - int rsv; + int error; + int rsv; rsv = XFS_DIRENTER_SPACE_RES(mp, name->len); - error = -libxfs_dir_createname(tp, pip, name, inum, first, dfops, rsv, - NULL); + error = -libxfs_dir_createname(tp, pip, name, ip->i_ino, first, dfops, rsv, + offset); if (error) fail(_("directory createname error"), error); + } static void @@ -387,6 +390,7 @@ parseproto( cred_t creds; char *value; struct xfs_name xname; + xfs_dir2_dataptr_t offset; memset(&creds, 0, sizeof(creds)); mstr = getstr(pp); @@ -470,7 +474,7 @@ parseproto( free(buf); libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_REG_FILE; - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); break; case IF_RESERVED: /* pre-allocated space only */ @@ -493,7 +497,7 @@ parseproto( libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_REG_FILE; - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); libxfs_trans_log_inode(tp, ip, flags); libxfs_defer_ijoin(&dfops, ip); @@ -516,7 +520,7 @@ parseproto( } libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_BLKDEV; - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); flags |= XFS_ILOG_DEV; break; @@ -530,7 +534,7 @@ parseproto( fail(_("Inode allocation failed"), error); libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_CHRDEV; - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); flags |= XFS_ILOG_DEV; break; @@ -542,7 +546,7 @@ parseproto( fail(_("Inode allocation failed"), error); libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_FIFO; - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); break; case IF_SYMLINK: buf = getstr(pp); @@ -555,7 +559,7 @@ parseproto( flags |= newfile(tp, ip, &dfops, &first, 1, 1, buf, len); libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_SYMLINK; - newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, &first, &dfops, &offset); break; case IF_DIRECTORY: tp = getres(mp, 0); @@ -572,8 +576,8 @@ parseproto( } else { libxfs_trans_ijoin(tp, pip, 0); xname.type = XFS_DIR3_FT_DIR; - newdirent(mp, tp, pip, &xname, ip->i_ino, - &first, &dfops); + newdirent(mp, tp, pip, &xname, ip, + &first, &dfops, &offset); inc_nlink(VFS_I(pip)); libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE); } @@ -612,8 +616,23 @@ parseproto( fail(_("Error encountered creating file from prototype file"), error); } - libxfs_trans_commit(tp); + libxfs_trans_commit(tp); + + if (xfs_sb_version_hasparent(&mp->m_sb)) { + error = xfs_parent_add(tp, pip, ip, &xname, offset, &first, &dfops); + if (error) + fail(_("Error creating parent pointer"), error); + + libxfs_trans_log_inode(tp, ip, flags); + libxfs_defer_ijoin(&dfops, ip); + error = -libxfs_defer_finish(&tp, &dfops); + if (error) + fail(_("Directory creation failed"), error); + libxfs_trans_commit(tp); + } + IRELE(ip); + } void diff --git a/repair/attr_repair.c b/repair/attr_repair.c index 8b1b8a7..e5ff3b0 100644 --- a/repair/attr_repair.c +++ b/repair/attr_repair.c @@ -305,16 +305,6 @@ process_shortform_attr( } } - /* namecheck checks for / and null terminated for file names. - * attributes names currently follow the same rules. - */ - if (namecheck((char *)¤tentry->nameval[0], - currententry->namelen)) { - do_warn( - _("entry contains illegal character in shortform attribute name\n")); - junkit = 1; - } - if (currententry->flags & XFS_ATTR_INCOMPLETE) { do_warn( _("entry has INCOMPLETE flag on in shortform attribute\n")); @@ -470,8 +460,7 @@ process_leaf_attr_local( xfs_attr_leaf_name_local_t *local; local = xfs_attr3_leaf_name_local(leaf, i); - if (local->namelen == 0 || namecheck((char *)&local->nameval[0], - local->namelen)) { + if (local->namelen == 0) { do_warn( _("attribute entry %d in attr block %u, inode %" PRIu64 " has bad name (namelen = %d)\n"), i, da_bno, ino, local->namelen); @@ -525,8 +514,7 @@ process_leaf_attr_remote( remotep = xfs_attr3_leaf_name_remote(leaf, i); - if (remotep->namelen == 0 || namecheck((char *)&remotep->name[0], - remotep->namelen) || + if (remotep->namelen == 0 || be32_to_cpu(entry->hashval) != libxfs_da_hashname((unsigned char *)&remotep->name[0], remotep->namelen) || diff --git a/repair/phase6.c b/repair/phase6.c index 4fedb35..b861b3b 100644 --- a/repair/phase6.c +++ b/repair/phase6.c @@ -29,6 +29,7 @@ #include "dinode.h" #include "progress.h" #include "versions.h" +#include "xfs_parent.h" static struct cred zerocr; static struct fsxattr zerofsx; @@ -962,19 +963,20 @@ mk_root_dir(xfs_mount_t *mp) static xfs_ino_t mk_orphanage(xfs_mount_t *mp) { - xfs_ino_t ino; - xfs_trans_t *tp; - xfs_inode_t *ip; - xfs_inode_t *pip; - xfs_fsblock_t first; - ino_tree_node_t *irec; - int ino_offset = 0; - int i; - int error; + xfs_ino_t ino; + xfs_trans_t *tp; + xfs_inode_t *ip; + xfs_inode_t *pip; + xfs_fsblock_t first; + ino_tree_node_t *irec; + int ino_offset = 0; + int i; + int error; struct xfs_defer_ops dfops; - const int mode = 0755; - int nres; - struct xfs_name xname; + const int mode = 0755; + int nres; + struct xfs_name xname; + xfs_dir2_dataptr_t offset; /* * check for an existing lost+found first, if it exists, return @@ -1061,7 +1063,7 @@ mk_orphanage(xfs_mount_t *mp) * create the actual entry */ error = -libxfs_dir_createname(tp, pip, &xname, ip->i_ino, &first, - &dfops, nres, NULL); + &dfops, nres, &offset); if (error) do_error( _("can't make %s, createname error %d\n"), @@ -1089,6 +1091,14 @@ mk_orphanage(xfs_mount_t *mp) ORPHANAGE, error); } + libxfs_trans_commit(tp); + if (xfs_sb_version_hasparent(&mp->m_sb)) { + error = xfs_parent_add(tp, pip, ip, &xname, offset, + &first, &dfops); + if (error) + do_error(_("Error creating parent pointer: %d\n"), + error); + } libxfs_trans_commit(tp); IRELE(ip); @@ -1120,6 +1130,7 @@ mv_orphanage( ino_tree_node_t *irec; int ino_offset = 0; struct xfs_name xname; + xfs_dir2_dataptr_t offset; xname.name = fname; xname.len = snprintf((char *)fname, sizeof(fname), "%llu", @@ -1170,7 +1181,7 @@ mv_orphanage( libxfs_defer_init(&dfops, &first); err = -libxfs_dir_createname(tp, orphanage_ip, &xname, - ino, &first, &dfops, nres, NULL); + ino, &first, &dfops, nres, &offset); if (err) do_error( _("name create failed in %s (%d), filesystem may be out of space\n"), @@ -1183,7 +1194,7 @@ mv_orphanage( libxfs_trans_log_inode(tp, orphanage_ip, XFS_ILOG_CORE); err = -libxfs_dir_createname(tp, ino_p, &xfs_name_dotdot, - orphanage_ino, &first, &dfops, nres, NULL); + orphanage_ino, &first, &dfops, nres, &offset); if (err) do_error( _("creation of .. entry failed (%d), filesystem may be out of space\n"), @@ -1214,7 +1225,7 @@ mv_orphanage( libxfs_defer_init(&dfops, &first); err = -libxfs_dir_createname(tp, orphanage_ip, &xname, - ino, &first, &dfops, nres, NULL); + ino, &first, &dfops, nres, &offset); if (err) do_error( _("name create failed in %s (%d), filesystem may be out of space\n"), @@ -1233,7 +1244,7 @@ mv_orphanage( if (entry_ino_num != orphanage_ino) { err = -libxfs_dir_replace(tp, ino_p, &xfs_name_dotdot, orphanage_ino, - &first, &dfops, nres, NULL); + &first, &dfops, nres, &offset); if (err) do_error( _("name replace op failed (%d), filesystem may be out of space\n"), @@ -1270,7 +1281,7 @@ mv_orphanage( libxfs_defer_init(&dfops, &first); err = -libxfs_dir_createname(tp, orphanage_ip, &xname, ino, - &first, &dfops, nres, NULL); + &first, &dfops, nres, &offset); if (err) do_error( _("name create failed in %s (%d), filesystem may be out of space\n"),
Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- mkfs/proto.c | 59 ++++++++++++++++++++++++++++++++++------------------ repair/attr_repair.c | 16 ++------------ repair/phase6.c | 47 +++++++++++++++++++++++++---------------- 3 files changed, 70 insertions(+), 52 deletions(-)