Hi Brian
> > {
> > xfs_trans_t *tp = NULL;
> > xfs_mount_t *mp = src_dp->i_mount;
> > int new_parent; /* moving to a new dir */
> > int src_is_directory; /* src_name is a directory */
> > + int tgt_is_directory;
>
> There's an unused variable warning for this.
Thanks for catching this!!!
>
> > int error;
> > xfs_bmap_free_t free_list;
> > xfs_fsblock_t first_block;
> > @@ -2706,6 +2708,7 @@ xfs_rename(
> > cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> > spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
> > error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
> > +
> > if (error == -ENOSPC) {
> > spaceres = 0;
> > error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
> > @@ -2756,143 +2759,155 @@ xfs_rename(
> > }
> >
> > /*
> > - * Set up the target.
> > - */
> > - if (target_ip == NULL) {
> > + * Handle RENAME_EXCHANGE flags
> > + */
> > + if (flags & RENAME_EXCHANGE) {
> > + error = xfs_cross_rename(tp, src_dp, src_name, src_ip,
> > target_dp,
> > + target_name, target_ip, &free_list,
> > + &first_block, spaceres);
> > + if (error)
> > + goto abort_return;
{
>
> The factoring of xfs_rename() looks much better, but we could probably
> avoid all the extra indentation in the else branch that follows by using
> a label. It does look like that indentation creates a bunch of 80+ char
> lines in xfs_rename(). E.g., something like:
>
> if (flags & RENAME_EXCHANGE) {
> ...
> goto complete_tp;
> }
> ...
> complete_tp:
> ...
> return xfs_trans_commit(...);
>
> ... and then kill the else above and leave the rest of xfs_rename() as
> is.
I don't think it's a bad idea at all. I particularly don't like the use of
labels for flow control like this, in this case though, it's a simple
"skip this bunch of code", and I'd not object to use it, it would make the
code more clear IMHO too.
>
> > /*
> > - * If there's no space reservation, check the entry will
> > - * fit before actually inserting it.
> > + * Set up the target.
> > */
> > - if (!spaceres) {
> > - error = xfs_dir_canenter(tp, target_dp, target_name);
> > - if (error)
> > + if (target_ip == NULL) {
> > + /*
> > + * If there's no space reservation, check the entry will
> > + * fit before actually inserting it.
> > + */
> > + if (!spaceres) {
> > + error = xfs_dir_canenter(tp, target_dp, target_name);
> > + if (error)
> > + goto error_return;
> > + }
> > + /*
> > + * If target does not exist and the rename crosses
> > + * directories, adjust the target directory link count
> > + * to account for the ".." reference from the new entry.
> > + */
> > + error = xfs_dir_createname(tp, target_dp, target_name,
> > + src_ip->i_ino,
> > &first_block,
> > + &free_list, spaceres);
> > + if (error == -ENOSPC)
> > goto error_return;
> > - }
> > - /*
> > - * If target does not exist and the rename crosses
> > - * directories, adjust the target directory link count
> > - * to account for the ".." reference from the new entry.
> > - */
> > - error = xfs_dir_createname(tp, target_dp, target_name,
> > - src_ip->i_ino, &first_block,
> > - &free_list, spaceres);
> > - if (error == -ENOSPC)
> > - goto error_return;
> > - if (error)
> > - goto abort_return;
> > + if (error)
> > + goto abort_return;
> >
> > - xfs_trans_ichgtime(tp, target_dp,
> > - XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > + xfs_trans_ichgtime(tp, target_dp,
> > + XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
> >
> > - if (new_parent && src_is_directory) {
> > - error = xfs_bumplink(tp, target_dp);
> > + if (new_parent && src_is_directory) {
> > + error = xfs_bumplink(tp, target_dp);
.
.
.
> > +/* xfs_cross_rename()
> > + *
> > + * responsible to handle RENAME_EXCHANGE flag
> > + * in renameat2() sytemcall
> > + */
> > +int
> > +xfs_cross_rename(
>
> xfs_cross_rename() is only used by xfs_rename() so it can be defined
> static. Somewhat of a nit, but I'd also expect to see it implemented
> above xfs_rename() as it is a helper for the latter. That could be a
> matter of personal taste, however. Otherwise, just stick a declaration
> at the top of the file.
>
I agree, as you said, although it's a matter of taste, making it static and
moving it above xfs_rename() will save us from some extra code.
.
.
.
> > + /*
> > + * Update ".." entry to match the new parents
> > + *
> > + * In case we are crossing different file types between different
> > + * parents, we must update parent's link count to match the ".."
> > + * entry of the new child (or the removal of it).
> > + */
>
> The logic of the subsequent block looks Ok to me, but I would probably
> split up the comment to be more specific. Something like the following
> just as an example:
>
> /*
> * If we're renaming one or more directories across different parents,
> * update the respective ".." entries (and link counts) to match the new
> * parents.
> */
Can't argue here, I'm not a native english speaker, so, it should be much more
readable in this way than mine :)
.
.
.
> > + if (new_parent) {
> > + xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
>
> Long line.
>
I think there is no reason to break this line. We should avoid 80+ columns per
line, but it's not a must, break this line will hurt readability of the code in
exchange of just a few chars more than 80. I don't think it's worth.
.
.
.
> > +out_abort:
>
> The name out_abort doesn't really mean much here. We don't abort
> anything. Perhaps just 'out?'
>
Agree, it doesn't mean an abort anymore, remains of the integration of the old
cross_rename version, 'out' is much better here.
.
.
.
> > @@ -340,7 +340,12 @@ int xfs_link(struct xfs_inode *tdp, struct
> > xfs_inode *sip,
> > int xfs_rename(struct xfs_inode *src_dp, struct xfs_name
> > *src_name,
> > struct xfs_inode *src_ip, struct xfs_inode
> > *target_dp,
> > struct xfs_name *target_name,
> > - struct xfs_inode *target_ip);
> > + struct xfs_inode *target_ip, unsigned int flags);
> > +int xfs_cross_rename(struct xfs_trans *tp, struct xfs_inode
> > *src_dp,
> > + struct xfs_name *src_name, struct xfs_inode
> > *src_ip,
> > + struct xfs_inode *target_dp, struct xfs_name
> > *target_name,
> > + struct xfs_inode *target_ip,struct
> > xfs_bmap_free *free_list,
> > + xfs_fsblock_t *first_block, int spaceres);
>
> There's no need for this once xfs_cross_rename() is static.
>
> Brian
>
Will vanish on the next version as soon as I make it static
Thanks the review Brian
--
Carlos
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
