| Message ID | 1426724311-9343-5-git-send-email-somlo@cmu.edu |
|---|---|
| State | New |
| Headers | show |
On 03/19/15 01:18, Gabriel L. Somlo wrote: > Exit with an error (instead of simply logging a trace event) > whenever the same fw_cfg file name is added multiple times via > one of the fw_cfg_add_file[_callback]() host-side API calls. > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > --- > hw/nvram/fw_cfg.c | 11 ++++++----- > trace-events | 1 - > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 659de4c..a5fd512 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > index = be32_to_cpu(s->files->count); > assert(index < FW_CFG_FILE_SLOTS); > > - fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index, > - callback, callback_opaque, data, len); > - > pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name), > filename); > for (i = 0; i < index; i++) { > if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) { > - trace_fw_cfg_add_file_dupe(s, s->files->f[index].name); > - return; > + error_report("duplicate fw_cfg file name: %s", > + s->files->f[index].name); > + exit(1); > } > } > > + fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index, > + callback, callback_opaque, data, len); > + > s->files->f[index].size = cpu_to_be32(len); > s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index); > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > diff --git a/trace-events b/trace-events > index 1275b70..a340c5a 100644 > --- a/trace-events > +++ b/trace-events > @@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x > # hw/nvram/fw_cfg.c > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > fw_cfg_read(void *s, uint8_t ret) "%p = %d" > -fw_cfg_add_file_dupe(void *s, char *name) "%p %s" > fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" > > # hw/block/hd-geometry.c > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
On 03/19/15 01:18, Gabriel L. Somlo wrote: > Exit with an error (instead of simply logging a trace event) > whenever the same fw_cfg file name is added multiple times via > one of the fw_cfg_add_file[_callback]() host-side API calls. > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > --- > hw/nvram/fw_cfg.c | 11 ++++++----- > trace-events | 1 - > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 659de4c..a5fd512 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > index = be32_to_cpu(s->files->count); > assert(index < FW_CFG_FILE_SLOTS); > > - fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index, > - callback, callback_opaque, data, len); > - > pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name), > filename); > for (i = 0; i < index; i++) { > if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) { > - trace_fw_cfg_add_file_dupe(s, s->files->f[index].name); > - return; > + error_report("duplicate fw_cfg file name: %s", > + s->files->f[index].name); > + exit(1); > } > } > > + fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index, > + callback, callback_opaque, data, len); > + > s->files->f[index].size = cpu_to_be32(len); > s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index); > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > diff --git a/trace-events b/trace-events > index 1275b70..a340c5a 100644 > --- a/trace-events > +++ b/trace-events > @@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x > # hw/nvram/fw_cfg.c > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > fw_cfg_read(void *s, uint8_t ret) "%p = %d" > -fw_cfg_add_file_dupe(void *s, char *name) "%p %s" > fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" > > # hw/block/hd-geometry.c > Here's an idea I had this morning. This series gives equal rank to fw_cfg file names that originate internally and those that come from the user, via the command line. That means that whenever qemu developers want to introduce a new fw_cfg file, they can never be sure that the new name will not conflict with something a user has already been passing in via the command line, for whatever purposes. (Because, well, that's the goal of this patchset, to empower the user to pass in fw_cfg files independently of qemu developers.) This looks brittle. How about: (a) advising users in the docs txt *and in the manual* to use some kind of fw_cfg file name prefix, like "usr/" or "opt/", and then steering clear of such prefixes in qemu, as far as developers are concerned. Or, (b) automatically prepending "opt/" or "usr/" to all fw_cfg file names that come via -fw_cfg (equiv. via [fw_cfg] in the config file), and, for developers, steering clear of those prefixes in qemu's source. The C standard and the POSIX standard define lists of identifier prefixes (well, patterns) that are reserved for various uses. If a program violates that, it might not compile on some platform, or with the next release of the compiler on the same platform etc. I think we should posit something like this. Personally I vote (a). Document it, but don't enforce it. (Assuming that a user-specified fw_cfg file gains traction, and becomes popular to the point that qemu wants to expose it itself, then qemu can just generate the same file with (eg.) an "etc/" prefix. And then firmware (or other guest code) can start looking for the file under both prefixes, and give priority to... well, that's another policy question; but we're talking mechanism thus far. :)) Thoughts about (a) vs. (b) vs. neither? Thanks Laszlo
On Fri, Mar 20, 2015 at 07:51:06AM +0100, Laszlo Ersek wrote: > Here's an idea I had this morning. > > This series gives equal rank to fw_cfg file names that originate > internally and those that come from the user, via the command line. > > That means that whenever qemu developers want to introduce a new fw_cfg > file, they can never be sure that the new name will not conflict with > something a user has already been passing in via the command line, for > whatever purposes. (Because, well, that's the goal of this patchset, to > empower the user to pass in fw_cfg files independently of qemu developers.) > > This looks brittle. How about: > > (a) advising users in the docs txt *and in the manual* to use some kind > of fw_cfg file name prefix, like "usr/" or "opt/", and then steering > clear of such prefixes in qemu, as far as developers are concerned. Or, > > (b) automatically prepending "opt/" or "usr/" to all fw_cfg file names > that come via -fw_cfg (equiv. via [fw_cfg] in the config file), and, for > developers, steering clear of those prefixes in qemu's source. > > The C standard and the POSIX standard define lists of identifier > prefixes (well, patterns) that are reserved for various uses. If a > program violates that, it might not compile on some platform, or with > the next release of the compiler on the same platform etc. I think we > should posit something like this. > > Personally I vote (a). Document it, but don't enforce it. > > (Assuming that a user-specified fw_cfg file gains traction, and becomes > popular to the point that qemu wants to expose it itself, then qemu can > just generate the same file with (eg.) an "etc/" prefix. And then > firmware (or other guest code) can start looking for the file under both > prefixes, and give priority to... well, that's another policy question; > but we're talking mechanism thus far. :)) > > Thoughts about (a) vs. (b) vs. neither? You're basically saying it'd be nice to keep user-provided commandline blobs in a separate *namespace* from those inserted programmatically by QEMU, and I definitely agree! My inner control freak's gut reaction is to vote (b), but I'm sure theres lots of facets of the issue I haven't considered, so I could easily be talked out of that selection :) Either way, this would go in with patch 5/5 (where the command line option is being added), so everything up to that point (including generating an error on dupe fwcfg file names) should probably stay the way it is... Thanks much, --Gabriel
On 03/20/15 15:34, Gabriel L. Somlo wrote: > On Fri, Mar 20, 2015 at 07:51:06AM +0100, Laszlo Ersek wrote: >> Here's an idea I had this morning. >> >> This series gives equal rank to fw_cfg file names that originate >> internally and those that come from the user, via the command line. >> >> That means that whenever qemu developers want to introduce a new >> fw_cfg file, they can never be sure that the new name will not >> conflict with something a user has already been passing in via the >> command line, for whatever purposes. (Because, well, that's the goal >> of this patchset, to empower the user to pass in fw_cfg files >> independently of qemu developers.) >> >> This looks brittle. How about: >> >> (a) advising users in the docs txt *and in the manual* to use some >> kind of fw_cfg file name prefix, like "usr/" or "opt/", and then >> steering clear of such prefixes in qemu, as far as developers are >> concerned. Or, >> >> (b) automatically prepending "opt/" or "usr/" to all fw_cfg file >> names that come via -fw_cfg (equiv. via [fw_cfg] in the config file), >> and, for developers, steering clear of those prefixes in qemu's >> source. >> >> The C standard and the POSIX standard define lists of identifier >> prefixes (well, patterns) that are reserved for various uses. If a >> program violates that, it might not compile on some platform, or with >> the next release of the compiler on the same platform etc. I think we >> should posit something like this. >> >> Personally I vote (a). Document it, but don't enforce it. >> >> (Assuming that a user-specified fw_cfg file gains traction, and >> becomes popular to the point that qemu wants to expose it itself, >> then qemu can just generate the same file with (eg.) an "etc/" >> prefix. And then firmware (or other guest code) can start looking for >> the file under both prefixes, and give priority to... well, that's >> another policy question; but we're talking mechanism thus far. :)) >> >> Thoughts about (a) vs. (b) vs. neither? > > You're basically saying it'd be nice to keep user-provided commandline > blobs in a separate *namespace* from those inserted programmatically > by QEMU, and I definitely agree! Right. Dunno why I didn't say "namespace"; otherwise I keep dropping that term twice a day. :) > My inner control freak's gut reaction is to vote (b), but I'm sure > theres lots of facets of the issue I haven't considered, so I could > easily be talked out of that selection :) Well, if you implement (b), I certainly won't protest. > Either way, this would go in with patch 5/5 (where the command line > option is being added), so everything up to that point (including > generating an error on dupe fwcfg file names) should probably stay > the way it is... Oh yes, sure. I guess I followed up here only because this would be the patch to catch the conflict. My R-b's stand. Thanks Laszlo
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 659de4c..a5fd512 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, index = be32_to_cpu(s->files->count); assert(index < FW_CFG_FILE_SLOTS); - fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index, - callback, callback_opaque, data, len); - pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name), filename); for (i = 0; i < index; i++) { if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) { - trace_fw_cfg_add_file_dupe(s, s->files->f[index].name); - return; + error_report("duplicate fw_cfg file name: %s", + s->files->f[index].name); + exit(1); } } + fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index, + callback, callback_opaque, data, len); + s->files->f[index].size = cpu_to_be32(len); s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index); trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); diff --git a/trace-events b/trace-events index 1275b70..a340c5a 100644 --- a/trace-events +++ b/trace-events @@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x # hw/nvram/fw_cfg.c fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" fw_cfg_read(void *s, uint8_t ret) "%p = %d" -fw_cfg_add_file_dupe(void *s, char *name) "%p %s" fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" # hw/block/hd-geometry.c
Exit with an error (instead of simply logging a trace event) whenever the same fw_cfg file name is added multiple times via one of the fw_cfg_add_file[_callback]() host-side API calls. Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- hw/nvram/fw_cfg.c | 11 ++++++----- trace-events | 1 - 2 files changed, 6 insertions(+), 6 deletions(-)