From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Vivek Goyal <vgoyal@redhat.com> Cc: dinechin@redhat.com, virtio-fs@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Subject: Re: [PATCH v3 1/5] tools/virtiofsd: xattr name mappings: Add option Date: Fri, 23 Oct 2020 16:46:58 +0100 [thread overview] Message-ID: <20201023154658.GG3038@work-vm> (raw) In-Reply-To: <20201022145242.GB512900@redhat.com> * Vivek Goyal (vgoyal@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 / dgilbert@redhat.com / Manchester, UK
WARNING: multiple messages have this Message-ID (diff)
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Vivek Goyal <vgoyal@redhat.com> Cc: dinechin@redhat.com, virtio-fs@redhat.com, qemu-devel@nongnu.org Subject: Re: [Virtio-fs] [PATCH v3 1/5] tools/virtiofsd: xattr name mappings: Add option Date: Fri, 23 Oct 2020 16:46:58 +0100 [thread overview] Message-ID: <20201023154658.GG3038@work-vm> (raw) In-Reply-To: <20201022145242.GB512900@redhat.com> * Vivek Goyal (vgoyal@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 / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2020-10-23 16:32 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-14 18:02 [PATCH v3 0/5] virtiofsd xattr name mappings Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [Virtio-fs] " Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [PATCH v3 1/5] tools/virtiofsd: xattr name mappings: Add option Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [Virtio-fs] " Dr. David Alan Gilbert (git) 2020-10-20 9:07 ` Stefan Hajnoczi 2020-10-20 9:07 ` [Virtio-fs] " Stefan Hajnoczi 2020-10-21 17:13 ` Dr. David Alan Gilbert 2020-10-21 17:13 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-20 14:04 ` Vivek Goyal 2020-10-20 14:04 ` [Virtio-fs] " Vivek Goyal 2020-10-22 14:52 ` Vivek Goyal 2020-10-22 14:52 ` [Virtio-fs] " Vivek Goyal 2020-10-23 15:46 ` Dr. David Alan Gilbert [this message] 2020-10-23 15:46 ` Dr. David Alan Gilbert 2020-10-14 18:02 ` [PATCH v3 2/5] tools/virtiofsd: xattr name mappings: Map client xattr names Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [Virtio-fs] " Dr. David Alan Gilbert (git) 2020-10-20 9:16 ` Stefan Hajnoczi 2020-10-20 9:16 ` [Virtio-fs] " Stefan Hajnoczi 2020-10-22 15:28 ` Vivek Goyal 2020-10-22 15:28 ` [Virtio-fs] " Vivek Goyal 2020-10-23 15:04 ` Dr. David Alan Gilbert 2020-10-23 15:04 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-14 18:02 ` [PATCH v3 3/5] tools/virtiofsd: xattr name mappings: Map server " Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [Virtio-fs] " Dr. David Alan Gilbert (git) 2020-10-15 23:57 ` [Virtio-fs] Puzzle about rootflags, restorecon "operation not supported" Harry G. Coin 2020-10-20 9:54 ` Stefan Hajnoczi 2020-10-20 12:57 ` Miklos Szeredi 2020-10-20 14:55 ` Harry G. Coin 2020-10-20 9:52 ` [PATCH v3 3/5] tools/virtiofsd: xattr name mappings: Map server xattr names Stefan Hajnoczi 2020-10-20 9:52 ` [Virtio-fs] " Stefan Hajnoczi 2020-10-22 16:16 ` Vivek Goyal 2020-10-22 16:16 ` [Virtio-fs] " Vivek Goyal 2020-10-23 14:49 ` Dr. David Alan Gilbert 2020-10-23 14:49 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-14 18:02 ` [PATCH v3 4/5] tools/virtiofsd: xattr name mapping examples Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [Virtio-fs] " Dr. David Alan Gilbert (git) 2020-10-20 9:56 ` Stefan Hajnoczi 2020-10-20 9:56 ` [Virtio-fs] " Stefan Hajnoczi 2020-10-20 14:40 ` Vivek Goyal 2020-10-20 14:40 ` [Virtio-fs] " Vivek Goyal 2020-10-20 15:34 ` Dr. David Alan Gilbert 2020-10-20 15:34 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-20 17:56 ` Vivek Goyal 2020-10-20 17:56 ` [Virtio-fs] " Vivek Goyal 2020-10-20 19:02 ` Dr. David Alan Gilbert 2020-10-20 19:02 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-21 13:44 ` Vivek Goyal 2020-10-21 13:44 ` [Virtio-fs] " Vivek Goyal 2020-10-21 17:39 ` Dr. David Alan Gilbert 2020-10-21 17:39 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-14 18:02 ` [PATCH v3 5/5] tools/virtiofsd: xattr name mappings: Simple 'map' Dr. David Alan Gilbert (git) 2020-10-14 18:02 ` [Virtio-fs] " Dr. David Alan Gilbert (git) 2020-10-20 10:09 ` Stefan Hajnoczi 2020-10-20 10:09 ` [Virtio-fs] " Stefan Hajnoczi 2020-10-20 11:35 ` Dr. David Alan Gilbert 2020-10-20 11:35 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-10-22 13:42 ` Vivek Goyal 2020-10-22 13:42 ` [Virtio-fs] " Vivek Goyal 2020-10-23 13:05 ` Dr. David Alan Gilbert 2020-10-23 13:05 ` [Virtio-fs] " Dr. David Alan Gilbert
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201023154658.GG3038@work-vm \ --to=dgilbert@redhat.com \ --cc=dinechin@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=vgoyal@redhat.com \ --cc=virtio-fs@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.