Re: [PATCH 4/4] xfs: consolidate sysfs ops (dbg, stats, log)

[Brian Foster <bfoster@redhat.com>]

On Tue, Sep 08, 2015 at 10:09:32AM -0500, Bill O'Donnell wrote:
>     As a part of the series to move xfs global stats from procfs to sysfs,
>     this patch consolidates the sysfs ops functions and removes redundancy.
> 
>     Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/xfs_stats.c |   2 +-
>  fs/xfs/xfs_sysfs.c | 147 ++++++++++++++++++-----------------------------------
>  2 files changed, 51 insertions(+), 98 deletions(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 05d5227..dc6ca67 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -92,7 +92,7 @@ int xfs_stats_format(char *buf)
>  		0);
>  #endif
>  
> -return len;
> +	return len;

Looks like this snuck in from a fix to a previous patch.

>  }
>  
>  void xfs_stats_clearall(void)
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index a094e20..6c44e95 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -54,6 +54,53 @@ struct kobj_type xfs_mp_ktype = {
>  	.release = xfs_sysfs_release,
>  };
>  
> +static inline struct xlog *
> +to_xlog(struct kobject *kobject)
> +{
> +	struct xfs_kobj *kobj = to_kobj(kobject);
> +	return container_of(kobj, struct xlog, l_kobj);
> +}
> +
> +STATIC ssize_t
> +xfs_sysfs_object_show(
> +	struct kobject		*kobject,
> +	struct attribute	*attr,
> +	char			*buf)
> +{
> +	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> +	struct kobj_type *ktype = get_ktype(kobject);
> +
> +	if (ktype == &xfs_log_ktype) {
> +		struct xlog *log = to_xlog(kobject);
> +		return xfs_attr->show ? xfs_attr->show(buf, log) : 0;
> +	}
> +	else
> +		return xfs_attr->show ? xfs_attr->show(buf, NULL) : 0;

Could we just set a void pointer to the object and not duplicate the
function call? The attribute handler parameter is a void * anyways.
E.g.:

	void *data = NULL;
	...
	if (...)
		data = to_xlog(kobject);
	return xfs_attr->show ? xfs_attr->show(buf, data) : 0;

(and in the store path as well). Otherwise, looks like a nice cleanup to
me.

Brian

P.S., It's good practice to include a brief, but running changelog in
the cover letter to call out precisely what changed from the previous
version(s) to help out reviewers. For example:

v4:
- Fixed up whitespace in patch N.
- Added patch 4.
...
v3:
...


> +}
> +
> +STATIC ssize_t
> +xfs_sysfs_object_store(
> +	struct kobject		*kobject,
> +	struct attribute	*attr,
> +	const char		*buf,
> +	size_t			count)
> +{
> +	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> +	struct kobj_type *ktype = get_ktype(kobject);
> +
> +	if (ktype == &xfs_log_ktype) {
> +		struct xlog *log = to_xlog(kobject);
> +		return xfs_attr->store ? xfs_attr->store(buf, count, log) : 0;
> +	}
> +	else
> +		return xfs_attr->store ? xfs_attr->store(buf, count, NULL) : 0;
> +}
> +
> +static struct sysfs_ops xfs_sysfs_ops = {
> +	.show = xfs_sysfs_object_show,
> +	.store = xfs_sysfs_object_store,
> +};
> +
>  #ifdef DEBUG
>  /* debug */
>  
> @@ -92,43 +139,14 @@ static struct attribute *xfs_dbg_attrs[] = {
>  	NULL,
>  };
>  
> -STATIC ssize_t
> -xfs_dbg_show(
> -	struct kobject		*kobject,
> -	struct attribute	*attr,
> -	char			*buf)
> -{
> -	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> -
> -	return xfs_attr->show ? xfs_attr->show(buf, NULL) : 0;
> -}
> -
> -STATIC ssize_t
> -xfs_dbg_store(
> -	struct kobject		*kobject,
> -	struct attribute	*attr,
> -	const char		*buf,
> -	size_t			count)
> -{
> -	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> -
> -	return xfs_attr->store ? xfs_attr->store(buf, count, NULL) : 0;
> -}
> -
> -static struct sysfs_ops xfs_dbg_ops = {
> -	.show = xfs_dbg_show,
> -	.store = xfs_dbg_store,
> -};
> -
>  struct kobj_type xfs_dbg_ktype = {
>  	.release = xfs_sysfs_release,
> -	.sysfs_ops = &xfs_dbg_ops,
> +	.sysfs_ops = &xfs_sysfs_ops,
>  	.default_attrs = xfs_dbg_attrs,
>  };
>  
>  #endif /* DEBUG */
>  
> -
>  /* stats */
>  
>  STATIC ssize_t
> @@ -166,37 +184,9 @@ static struct attribute *xfs_stats_attrs[] = {
>  	NULL,
>  };
>  
> -STATIC ssize_t
> -xfs_stats_show(
> -	struct kobject		*kobject,
> -	struct attribute	*attr,
> -	char			*buf)
> -{
> -	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> -
> -	return xfs_attr->show ? xfs_attr->show(buf, NULL) : 0;
> -}
> -
> -STATIC ssize_t
> -xfs_stats_store(
> -	struct kobject		*kobject,
> -	struct attribute	*attr,
> -	const char		*buf,
> -	size_t			count)
> -{
> -	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> -
> -	return xfs_attr->store ? xfs_attr->store(buf, count, NULL) : 0;
> -}
> -
> -static struct sysfs_ops xfs_stats_ops = {
> -	.show = xfs_stats_show,
> -	.store = xfs_stats_store,
> -};
> -
>  struct kobj_type xfs_stats_ktype = {
>  	.release = xfs_sysfs_release,
> -	.sysfs_ops = &xfs_stats_ops,
> +	.sysfs_ops = &xfs_sysfs_ops,
>  	.default_attrs = xfs_stats_attrs,
>  };
>  
> @@ -270,45 +260,8 @@ static struct attribute *xfs_log_attrs[] = {
>  	NULL,
>  };
>  
> -static inline struct xlog *
> -to_xlog(struct kobject *kobject)
> -{
> -	struct xfs_kobj *kobj = to_kobj(kobject);
> -	return container_of(kobj, struct xlog, l_kobj);
> -}
> -
> -STATIC ssize_t
> -xfs_log_show(
> -	struct kobject		*kobject,
> -	struct attribute	*attr,
> -	char			*buf)
> -{
> -	struct xlog *log = to_xlog(kobject);
> -	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> -
> -	return xfs_attr->show ? xfs_attr->show(buf, log) : 0;
> -}
> -
> -STATIC ssize_t
> -xfs_log_store(
> -	struct kobject		*kobject,
> -	struct attribute	*attr,
> -	const char		*buf,
> -	size_t			count)
> -{
> -	struct xlog *log = to_xlog(kobject);
> -	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
> -
> -	return xfs_attr->store ? xfs_attr->store(buf, count, log) : 0;
> -}
> -
> -static struct sysfs_ops xfs_log_ops = {
> -	.show = xfs_log_show,
> -	.store = xfs_log_store,
> -};
> -
>  struct kobj_type xfs_log_ktype = {
>  	.release = xfs_sysfs_release,
> -	.sysfs_ops = &xfs_log_ops,
> +	.sysfs_ops = &xfs_sysfs_ops,
>  	.default_attrs = xfs_log_attrs,
>  };
> -- 
> 2.4.3
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs