* Vivek Goyal (vgo...@redhat.com) wrote:
> On Wed, Oct 14, 2020 at 07:02:05PM +0100, Dr. David Alan Gilbert (git) wrote:
>
> [..]
> > +/*
> > + * Exit; process attribute unmodified if matched.
> > + * An empty key applies to all.
> > + */
> > +#define XATTR_MAP_FLAG_END_OK (1 << 0)
> > +/*
> > + * The attribute is unwanted;
> > + * EPERM on write hidden on read.
> > + */
> > +#define XATTR_MAP_FLAG_END_BAD (1 << 1)
> > +/*
> > + * For attr that start with 'key' prepend 'prepend'
> > + * 'key' maybe empty to prepend for all attrs
> > + * key is defined from set/remove point of view.
> > + * Automatically reversed on read
> > + */
> > +#define XATTR_MAP_FLAG_PREFIX (1 << 2)
> > +/* Apply rule to get/set/remove */
> > +#define XATTR_MAP_FLAG_CLIENT (1 << 16)
> > +/* Apply rule to list */
> > +#define XATTR_MAP_FLAG_SERVER (1 << 17)
> > +/* Apply rule to all */
> > +#define XATTR_MAP_FLAG_ALL (XATTR_MAP_FLAG_SERVER |
> > XATTR_MAP_FLAG_CLIENT)
> > +
> > +/* Last rule in the XATTR_MAP */
> > +#define XATTR_MAP_FLAG_LAST (1 << 30)
>
> I see that you are using bit positions for flags. Not clear why you
> used bit 0,1,2 and then jumped to 16,17 and then to 30. May be you
> are doing some sort of reservation of bits. Will be nice to explain
> that a bit so that next person modifying it can use bits from
> correct pool.
I've added a 'types' and a 'scopes' comment pair to hopefully make it
clear how I split it up.
> > +
> > +static XattrMapEntry *add_xattrmap_entry(XattrMapEntry *orig_map,
> > + size_t *nentries,
> > + const XattrMapEntry *new_entry)
> > +{
> > + XattrMapEntry *res = g_realloc_n(orig_map, ++*nentries,
> > + sizeof(XattrMapEntry));
> > + res[*nentries - 1] = *new_entry;
> > +
> > + return res;
> > +}
> > +
> > +static void free_xattrmap(XattrMapEntry *map)
> > +{
> > + XattrMapEntry *curr = map;
> > +
> > + if (!map) {
> > + return;
> > + };
>
> ; after } is not needed.
Gone.
> > +
> > + do {
> > + g_free(curr->key);
> > + g_free(curr->prepend);
> > + } while (!(curr++->flags & XATTR_MAP_FLAG_LAST));
> > +
> > + g_free(map);
> > +}
> > +
> > +static XattrMapEntry *parse_xattrmap(struct lo_data *lo)
> > +{
> > + XattrMapEntry *res = NULL;
> > + XattrMapEntry tmp_entry;
> > + size_t nentries = 0;
>
> If you are calculating number of entries (nentries), may be this could
> be stored in lo_data so that can be later used to free entries or loop
> through rules etc.
Done; and that removes the need for _LAST.
> > + const char *map = lo->xattrmap;
> > + const char *tmp;
> > +
> > + while (*map) {
> > + char sep;
> > +
> > + if (isspace(*map)) {
> > + map++;
> > + continue;
> > + }
> > + /* The separator is the first non-space of the rule */
> > + sep = *map++;
> > + if (!sep) {
> > + break;
> > + }
>
> When can sep be NULL? In that case while loop will not even continue.
The end of the rule list.
> > +
> > + /* Start of 'type' */
> > + if (strstart(map, "prefix", &map)) {
> > + tmp_entry.flags |= XATTR_MAP_FLAG_PREFIX;
> > + } else if (strstart(map, "ok", &map)) {
> > + tmp_entry.flags |= XATTR_MAP_FLAG_END_OK;
> > + } else if (strstart(map, "bad", &map)) {
> > + tmp_entry.flags |= XATTR_MAP_FLAG_END_BAD;
> > + } else {
> > + fuse_log(FUSE_LOG_ERR,
> > + "%s: Unexpected type;"
> > + "Expecting 'prefix', 'ok', or 'bad' in rule %zu\n",
> > + __func__, nentries);
> > + exit(1);
> > + }
> > +
> > + if (*map++ != sep) {
> > + fuse_log(FUSE_LOG_ERR,
> > + "%s: Missing '%c' at end of type field of rule %zu\n",
> > + __func__, sep, nentries);
> > + exit(1);
> > + }
> > +
> > + /* Start of 'scope' */
> > + if (strstart(map, "client", &map)) {
> > + tmp_entry.flags |= XATTR_MAP_FLAG_CLIENT;
> > + } else if (strstart(map, "server", &map)) {
> > + tmp_entry.flags |= XATTR_MAP_FLAG_SERVER;
> > + } else if (strstart(map, "all", &map)) {
> > + tmp_entry.flags |= XATTR_MAP_FLAG_ALL;
> > + } else {
> > + fuse_log(FUSE_LOG_ERR,
> > + "%s: Unexpected scope;"
> > + " Expecting 'client', 'server', or 'all', in rule
> > %zu\n",
> > + __func__, nentries);
> > + exit(1);
> > + }
> > +
> > + if (*map++ != sep) {
> > + fuse_log(FUSE_LOG_ERR,
> > + "%s: Expecting '%c' found '%c'"
> > + " after scope in rule %zu\n",
> > + __func__, sep, *map, nentries);
> > + exit(1);
> > + }
> > +
> > + /* At start of 'key' field */
> > + tmp = strchr(map, sep);
> > + if (!tmp) {
> > + fuse_log(FUSE_LOG_ERR,
> > + "%s: Missing '%c' at end of key field of rule %zu",
> > + __func__, sep, nentries);
> > + exit(1);
> > + }
> > + tmp_entry.key = g_strndup(map, tmp - map);
> > + map = tmp + 1;
> > +
> > + /* At start of 'prepend' field */
> > + tmp = strchr(map, sep);
> > + if (!tmp) {
> > + fuse_log(FUSE_LOG_ERR,
> > + "%s: Missing '%c' at end of prepend field of rule
> > %zu",
> > + __func__, sep, nentries);
> > + exit(1);
> > + }
> > + tmp_entry.prepend = g_strndup(map, tmp - map);
> > + map = tmp + 1;
> > +
> > + lo->xattr_map_list = add_xattrmap_entry(lo->xattr_map_list,
> > &nentries,
> > + &tmp_entry);
> > + /* End of rule - go around again for another rule */
> > + }
> > +
> > + if (!nentries) {
> > + fuse_log(FUSE_LOG_ERR, "Empty xattr map\n");
> > + exit(1);
> > + }
> > +
> > + /* Add a terminator to error in cases the user hasn't specified */
> > + tmp_entry.flags = XATTR_MAP_FLAG_ALL | XATTR_MAP_FLAG_END_BAD |
> > + XATTR_MAP_FLAG_LAST;
> > + tmp_entry.key = g_strdup("");
> > + tmp_entry.prepend = g_strdup("");
> > + lo->xattr_map_list = add_xattrmap_entry(lo->xattr_map_list, &nentries,
> > + &tmp_entry);
>
> Not sure why this default rule is needed when user has not specified one.
>
> This seems to be equivalent of ":bad:all:::". Will this not block all
> the xattrs which have not been caught by previous rules. And user
> probably did not want it.
I might be able to get rid of that now; my preference is to tell the
users they should be explicit about what happens.
Dave
> Thanks
> Vivek
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
