| Message ID | 20200509170125.952508-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/8] libxfs: use tabs instead of spaces in div_u64 | expand |
On Sat, May 09, 2020 at 07:01:21PM +0200, Christoph Hellwig wrote: > Don't use local variables for information that is set in the da_args > structure. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Seems like a good cleanup, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > db/attrset.c | 67 ++++++++++++++++++++++------------------------------ > 1 file changed, 28 insertions(+), 39 deletions(-) > > diff --git a/db/attrset.c b/db/attrset.c > index 1ff2eb85..0a464983 100644 > --- a/db/attrset.c > +++ b/db/attrset.c > @@ -67,10 +67,9 @@ attr_set_f( > int argc, > char **argv) > { > - struct xfs_inode *ip = NULL; > - struct xfs_da_args args = { NULL }; > - char *name, *value, *sp; > - int c, valuelen = 0; > + struct xfs_da_args args = { }; > + char *sp; > + int c; > > if (cur_typ == NULL) { > dbprintf(_("no current type\n")); > @@ -111,8 +110,9 @@ attr_set_f( > > /* value length */ > case 'v': > - valuelen = (int)strtol(optarg, &sp, 0); > - if (*sp != '\0' || valuelen < 0 || valuelen > 64*1024) { > + args.valuelen = strtol(optarg, &sp, 0); > + if (*sp != '\0' || > + args.valuelen < 0 || args.valuelen > 64 * 1024) { > dbprintf(_("bad attr_set valuelen %s\n"), optarg); > return 0; > } > @@ -129,35 +129,29 @@ attr_set_f( > return 0; > } > > - name = argv[optind]; > + args.name = (const unsigned char *)argv[optind]; > + args.namelen = strlen(argv[optind]); > > - if (valuelen) { > - value = (char *)memalign(getpagesize(), valuelen); > - if (!value) { > - dbprintf(_("cannot allocate buffer (%d)\n"), valuelen); > + if (args.valuelen) { > + args.value = memalign(getpagesize(), args.valuelen); > + if (!args.value) { > + dbprintf(_("cannot allocate buffer (%d)\n"), > + args.valuelen); > goto out; > } > - memset(value, 'v', valuelen); > - } else { > - value = NULL; > + memset(args.value, 'v', args.valuelen); > } > > - if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip, > + if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp, > &xfs_default_ifork_ops)) { > dbprintf(_("failed to iget inode %llu\n"), > (unsigned long long)iocur_top->ino); > goto out; > } > > - args.dp = ip; > - args.name = (unsigned char *)name; > - args.namelen = strlen(name); > - args.value = value; > - args.valuelen = valuelen; > - > if (libxfs_attr_set(&args)) { > dbprintf(_("failed to set attr %s on inode %llu\n"), > - name, (unsigned long long)iocur_top->ino); > + args.name, (unsigned long long)iocur_top->ino); > goto out; > } > > @@ -166,10 +160,10 @@ attr_set_f( > > out: > mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR; > - if (ip) > - libxfs_irele(ip); > - if (value) > - free(value); > + if (args.dp) > + libxfs_irele(args.dp); > + if (args.value) > + free(args.value); > return 0; > } > > @@ -178,9 +172,7 @@ attr_remove_f( > int argc, > char **argv) > { > - struct xfs_inode *ip = NULL; > - struct xfs_da_args args = { NULL }; > - char *name; > + struct xfs_da_args args = { }; > int c; > > if (cur_typ == NULL) { > @@ -223,23 +215,20 @@ attr_remove_f( > return 0; > } > > - name = argv[optind]; > + args.name = (const unsigned char *)argv[optind]; > + args.namelen = strlen(argv[optind]); > > - if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip, > + if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp, > &xfs_default_ifork_ops)) { > dbprintf(_("failed to iget inode %llu\n"), > (unsigned long long)iocur_top->ino); > goto out; > } > > - args.dp = ip; > - args.name = (unsigned char *)name; > - args.namelen = strlen(name); > - args.value = NULL; > - > if (libxfs_attr_set(&args)) { > dbprintf(_("failed to remove attr %s from inode %llu\n"), > - name, (unsigned long long)iocur_top->ino); > + (unsigned char *)args.name, > + (unsigned long long)iocur_top->ino); > goto out; > } > > @@ -248,7 +237,7 @@ attr_remove_f( > > out: > mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR; > - if (ip) > - libxfs_irele(ip); > + if (args.dp) > + libxfs_irele(args.dp); > return 0; > } > -- > 2.26.2 >
On 5/9/20 12:01 PM, Christoph Hellwig wrote: > Don't use local variables for information that is set in the da_args > structure. I'm on the fence about this one; Darrick had missed setting a couple of necessary structure members, so I actually see some value in assigning them all right before we call into libxfs_attr_set .... it makes it very clear what's being sent in to libxfs_attr_set. -Eric > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > db/attrset.c | 67 ++++++++++++++++++++++------------------------------ > 1 file changed, 28 insertions(+), 39 deletions(-) > > diff --git a/db/attrset.c b/db/attrset.c > index 1ff2eb85..0a464983 100644 > --- a/db/attrset.c > +++ b/db/attrset.c > @@ -67,10 +67,9 @@ attr_set_f( > int argc, > char **argv) > { > - struct xfs_inode *ip = NULL; > - struct xfs_da_args args = { NULL }; > - char *name, *value, *sp; > - int c, valuelen = 0; > + struct xfs_da_args args = { }; > + char *sp; > + int c; > > if (cur_typ == NULL) { > dbprintf(_("no current type\n")); > @@ -111,8 +110,9 @@ attr_set_f( > > /* value length */ > case 'v': > - valuelen = (int)strtol(optarg, &sp, 0); > - if (*sp != '\0' || valuelen < 0 || valuelen > 64*1024) { > + args.valuelen = strtol(optarg, &sp, 0); > + if (*sp != '\0' || > + args.valuelen < 0 || args.valuelen > 64 * 1024) { > dbprintf(_("bad attr_set valuelen %s\n"), optarg); > return 0; > } > @@ -129,35 +129,29 @@ attr_set_f( > return 0; > } > > - name = argv[optind]; > + args.name = (const unsigned char *)argv[optind]; > + args.namelen = strlen(argv[optind]); > > - if (valuelen) { > - value = (char *)memalign(getpagesize(), valuelen); > - if (!value) { > - dbprintf(_("cannot allocate buffer (%d)\n"), valuelen); > + if (args.valuelen) { > + args.value = memalign(getpagesize(), args.valuelen); > + if (!args.value) { > + dbprintf(_("cannot allocate buffer (%d)\n"), > + args.valuelen); > goto out; > } > - memset(value, 'v', valuelen); > - } else { > - value = NULL; > + memset(args.value, 'v', args.valuelen); > } > > - if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip, > + if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp, > &xfs_default_ifork_ops)) { > dbprintf(_("failed to iget inode %llu\n"), > (unsigned long long)iocur_top->ino); > goto out; > } > > - args.dp = ip; > - args.name = (unsigned char *)name; > - args.namelen = strlen(name); > - args.value = value; > - args.valuelen = valuelen; > - > if (libxfs_attr_set(&args)) { > dbprintf(_("failed to set attr %s on inode %llu\n"), > - name, (unsigned long long)iocur_top->ino); > + args.name, (unsigned long long)iocur_top->ino); > goto out; > } > > @@ -166,10 +160,10 @@ attr_set_f( > > out: > mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR; > - if (ip) > - libxfs_irele(ip); > - if (value) > - free(value); > + if (args.dp) > + libxfs_irele(args.dp); > + if (args.value) > + free(args.value); > return 0; > } > > @@ -178,9 +172,7 @@ attr_remove_f( > int argc, > char **argv) > { > - struct xfs_inode *ip = NULL; > - struct xfs_da_args args = { NULL }; > - char *name; > + struct xfs_da_args args = { }; > int c; > > if (cur_typ == NULL) { > @@ -223,23 +215,20 @@ attr_remove_f( > return 0; > } > > - name = argv[optind]; > + args.name = (const unsigned char *)argv[optind]; > + args.namelen = strlen(argv[optind]); > > - if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip, > + if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp, > &xfs_default_ifork_ops)) { > dbprintf(_("failed to iget inode %llu\n"), > (unsigned long long)iocur_top->ino); > goto out; > } > > - args.dp = ip; > - args.name = (unsigned char *)name; > - args.namelen = strlen(name); > - args.value = NULL; > - > if (libxfs_attr_set(&args)) { > dbprintf(_("failed to remove attr %s from inode %llu\n"), > - name, (unsigned long long)iocur_top->ino); > + (unsigned char *)args.name, > + (unsigned long long)iocur_top->ino); > goto out; > } > > @@ -248,7 +237,7 @@ attr_remove_f( > > out: > mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR; > - if (ip) > - libxfs_irele(ip); > + if (args.dp) > + libxfs_irele(args.dp); > return 0; > } >
On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote: > On 5/9/20 12:01 PM, Christoph Hellwig wrote: > > Don't use local variables for information that is set in the da_args > > structure. > > I'm on the fence about this one; Darrick had missed setting a couple > of necessary structure members, so I actually see some value in assigning them > all right before we call into libxfs_attr_set .... it makes it very clear what's > being sent in to libxfs_attr_set. But using additional local variables doesn't help with initialing the fields, it actually makes it easier to miss, which I guess is what happened. I find the code much easier to verify without the extra variables.
On 5/10/20 2:11 AM, Christoph Hellwig wrote: > On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote: >> On 5/9/20 12:01 PM, Christoph Hellwig wrote: >>> Don't use local variables for information that is set in the da_args >>> structure. >> >> I'm on the fence about this one; Darrick had missed setting a couple >> of necessary structure members, so I actually see some value in assigning them >> all right before we call into libxfs_attr_set .... it makes it very clear what's >> being sent in to libxfs_attr_set. > > But using additional local variables doesn't help with initialing > the fields, it actually makes it easier to miss, which I guess is > what happened. I find the code much easier to verify without the > extra variables. They seem a bit extraneous, but my problem is I can't keep track of how much of the args structure is actually filled out when it's spread out over dozens of lines .... *shrug* I dunno. Maybe darrick can cast the tie-breaking vote. ;)
On Sun, May 10, 2020 at 09:09:14AM -0500, Eric Sandeen wrote: > On 5/10/20 2:11 AM, Christoph Hellwig wrote: > > On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote: > >> On 5/9/20 12:01 PM, Christoph Hellwig wrote: > >>> Don't use local variables for information that is set in the da_args > >>> structure. > >> > >> I'm on the fence about this one; Darrick had missed setting a couple > >> of necessary structure members, so I actually see some value in assigning them > >> all right before we call into libxfs_attr_set .... it makes it very clear what's > >> being sent in to libxfs_attr_set. > > > > But using additional local variables doesn't help with initialing > > the fields, it actually makes it easier to miss, which I guess is > > what happened. I find the code much easier to verify without the > > extra variables. > > They seem a bit extraneous, but my problem is I can't keep track of how much > of the args structure is actually filled out when it's spread out over dozens > of lines .... > > *shrug* I dunno. Maybe darrick can cast the tie-breaking vote. ;) I mean... I /did/ already RVB this one... :) --D
On 5/10/20 10:41 PM, Darrick J. Wong wrote: > On Sun, May 10, 2020 at 09:09:14AM -0500, Eric Sandeen wrote: >> On 5/10/20 2:11 AM, Christoph Hellwig wrote: >>> On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote: >>>> On 5/9/20 12:01 PM, Christoph Hellwig wrote: >>>>> Don't use local variables for information that is set in the da_args >>>>> structure. >>>> >>>> I'm on the fence about this one; Darrick had missed setting a couple >>>> of necessary structure members, so I actually see some value in assigning them >>>> all right before we call into libxfs_attr_set .... it makes it very clear what's >>>> being sent in to libxfs_attr_set. >>> >>> But using additional local variables doesn't help with initialing >>> the fields, it actually makes it easier to miss, which I guess is >>> what happened. I find the code much easier to verify without the >>> extra variables. >> >> They seem a bit extraneous, but my problem is I can't keep track of how much >> of the args structure is actually filled out when it's spread out over dozens >> of lines .... >> >> *shrug* I dunno. Maybe darrick can cast the tie-breaking vote. ;) > > I mean... I /did/ already RVB this one... :) Before I raised the question, but *shrug* ok, I guess nobody else sees it my way so I'll merge it as is, not worth haggling over any further. thanks for all the cleanups, Christoph. -Eric
diff --git a/db/attrset.c b/db/attrset.c index 1ff2eb85..0a464983 100644 --- a/db/attrset.c +++ b/db/attrset.c @@ -67,10 +67,9 @@ attr_set_f( int argc, char **argv) { - struct xfs_inode *ip = NULL; - struct xfs_da_args args = { NULL }; - char *name, *value, *sp; - int c, valuelen = 0; + struct xfs_da_args args = { }; + char *sp; + int c; if (cur_typ == NULL) { dbprintf(_("no current type\n")); @@ -111,8 +110,9 @@ attr_set_f( /* value length */ case 'v': - valuelen = (int)strtol(optarg, &sp, 0); - if (*sp != '\0' || valuelen < 0 || valuelen > 64*1024) { + args.valuelen = strtol(optarg, &sp, 0); + if (*sp != '\0' || + args.valuelen < 0 || args.valuelen > 64 * 1024) { dbprintf(_("bad attr_set valuelen %s\n"), optarg); return 0; } @@ -129,35 +129,29 @@ attr_set_f( return 0; } - name = argv[optind]; + args.name = (const unsigned char *)argv[optind]; + args.namelen = strlen(argv[optind]); - if (valuelen) { - value = (char *)memalign(getpagesize(), valuelen); - if (!value) { - dbprintf(_("cannot allocate buffer (%d)\n"), valuelen); + if (args.valuelen) { + args.value = memalign(getpagesize(), args.valuelen); + if (!args.value) { + dbprintf(_("cannot allocate buffer (%d)\n"), + args.valuelen); goto out; } - memset(value, 'v', valuelen); - } else { - value = NULL; + memset(args.value, 'v', args.valuelen); } - if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip, + if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp, &xfs_default_ifork_ops)) { dbprintf(_("failed to iget inode %llu\n"), (unsigned long long)iocur_top->ino); goto out; } - args.dp = ip; - args.name = (unsigned char *)name; - args.namelen = strlen(name); - args.value = value; - args.valuelen = valuelen; - if (libxfs_attr_set(&args)) { dbprintf(_("failed to set attr %s on inode %llu\n"), - name, (unsigned long long)iocur_top->ino); + args.name, (unsigned long long)iocur_top->ino); goto out; } @@ -166,10 +160,10 @@ attr_set_f( out: mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR; - if (ip) - libxfs_irele(ip); - if (value) - free(value); + if (args.dp) + libxfs_irele(args.dp); + if (args.value) + free(args.value); return 0; } @@ -178,9 +172,7 @@ attr_remove_f( int argc, char **argv) { - struct xfs_inode *ip = NULL; - struct xfs_da_args args = { NULL }; - char *name; + struct xfs_da_args args = { }; int c; if (cur_typ == NULL) { @@ -223,23 +215,20 @@ attr_remove_f( return 0; } - name = argv[optind]; + args.name = (const unsigned char *)argv[optind]; + args.namelen = strlen(argv[optind]); - if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip, + if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp, &xfs_default_ifork_ops)) { dbprintf(_("failed to iget inode %llu\n"), (unsigned long long)iocur_top->ino); goto out; } - args.dp = ip; - args.name = (unsigned char *)name; - args.namelen = strlen(name); - args.value = NULL; - if (libxfs_attr_set(&args)) { dbprintf(_("failed to remove attr %s from inode %llu\n"), - name, (unsigned long long)iocur_top->ino); + (unsigned char *)args.name, + (unsigned long long)iocur_top->ino); goto out; } @@ -248,7 +237,7 @@ attr_remove_f( out: mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR; - if (ip) - libxfs_irele(ip); + if (args.dp) + libxfs_irele(args.dp); return 0; }
Don't use local variables for information that is set in the da_args structure. Signed-off-by: Christoph Hellwig <hch@lst.de> --- db/attrset.c | 67 ++++++++++++++++++++++------------------------------ 1 file changed, 28 insertions(+), 39 deletions(-)