From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754050AbbL2XAr (ORCPT ); Tue, 29 Dec 2015 18:00:47 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:48807 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753156AbbL2XAp (ORCPT ); Tue, 29 Dec 2015 18:00:45 -0500 Date: Tue, 29 Dec 2015 15:00:30 -0800 From: Joel Becker To: Christoph Hellwig Cc: akpm@linux-foundation.org, nab@linux-iscsi.org, pantelis.antoniou@konsulko.com, k.opasiak@samsung.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] configfs: implement binary attributes Message-ID: <20151229230029.GC32415@noexit.roam.corp.google.com> Mail-Followup-To: Christoph Hellwig , akpm@linux-foundation.org, nab@linux-iscsi.org, pantelis.antoniou@konsulko.com, k.opasiak@samsung.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org References: <1450968671-13699-1-git-send-email-hch@lst.de> <1450968671-13699-2-git-send-email-hch@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450968671-13699-2-git-send-email-hch@lst.de> X-Burt-Line: Trees are cool. X-Red-Smith: Ninety feet between bases is perhaps as close as man has ever come to perfection. User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 24, 2015 at 03:51:10PM +0100, Christoph Hellwig wrote: > From: Pantelis Antoniou > > ConfigFS lacked binary attributes up until now. This patch > introduces support for binary attributes in a somewhat similar > manner of sysfs binary attributes albeit with changes that > fit the configfs usage model. > > Problems that configfs binary attributes fix are everything that > requires a binary blob as part of the configuration of a resource, > such as bitstream loading for FPGAs, DTBs for dynamically created > devices etc. Overall, I really like this addition. > @@ -423,7 +429,9 @@ static int configfs_attach_attr(struct configfs_dirent * sd, struct dentry * den > spin_unlock(&configfs_dirent_lock); > > error = configfs_create(dentry, (attr->ca_mode & S_IALLUGO) | S_IFREG, > - configfs_init_file); > + (sd->s_type & CONFIGFS_ITEM_ATTR) ? > + configfs_init_file : > + configfs_init_bin_file); BIN_ATTRs are the more uncommon type, at least for now. I would think this code should check for special cases and fall back to ITEM_ATTR. So reverse it. (sd->s_type & CONFIGFS_ITEM_BIN_ATTR) ? configfs_init_bin_file : configfs_init_file > +static ssize_t > +configfs_read_bin_file(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct configfs_buffer *buffer = file->private_data; > + struct dentry *dentry = file->f_path.dentry; > + struct config_item *item = to_item(dentry->d_parent); > + struct configfs_bin_attribute *bin_attr = to_bin_attr(dentry); > + ssize_t retval = 0; > + ssize_t len = min_t(size_t, count, PAGE_SIZE); > + > + mutex_lock(&buffer->mutex); > + > + /* we don't support switching read/write modes */ > + if (buffer->write_in_progress) { > + retval = -EINVAL; > + goto out; > + } These are valid arguments, it's just competing with another operation. Wouldn't something like EINPROGRESS or ETXTBSY make more sense and be more informative? The same for configfs_write_bin_file(). Joel -- "Soap and education are not as sudden as a massacre, but they are more deadly in the long run." - Mark Twain http://www.jlbec.org/ jlbec@evilplan.org