| Message ID | 1363867631-5859-2-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 21, 2013 at 01:07:11PM +0100, Peter Lieven wrote: > this patch adds a check for qemu_find_opts("iscsi") returning > NULL instead of blindly passing the result to qemu_opts_parse(). > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > vl.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) Why is this change necessary? Stefan
Am 25.03.2013 um 15:00 schrieb Stefan Hajnoczi <stefanha@gmail.com>: > On Thu, Mar 21, 2013 at 01:07:11PM +0100, Peter Lieven wrote: >> this patch adds a check for qemu_find_opts("iscsi") returning >> NULL instead of blindly passing the result to qemu_opts_parse(). >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> vl.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) > > Why is this change necessary? a) just to make sure we check the result of qemu_find_opts for NULL in case a bug stops registering the opts again b) to have an error output if libiscsi is not compiled in and someone passes an iscsi flag (analogue to the error if you supply -spice XXX) Peter > > Stefan
On 25 March 2013 14:10, Peter Lieven <pl@kamp.de> wrote: > Am 25.03.2013 um 15:00 schrieb Stefan Hajnoczi <stefanha@gmail.com>: >> On Thu, Mar 21, 2013 at 01:07:11PM +0100, Peter Lieven wrote: >>> this patch adds a check for qemu_find_opts("iscsi") returning >>> NULL instead of blindly passing the result to qemu_opts_parse(). >>> >>> Signed-off-by: Peter Lieven <pl@kamp.de> >>> --- >>> vl.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> Why is this change necessary? > > a) just to make sure we check the result of qemu_find_opts for NULL in case > a bug stops registering the opts again It's OK to crash for "can't happen" cases IMHO. > b) to have an error output if libiscsi is not compiled in and someone passes > an iscsi flag (analogue to the error if you supply -spice XXX) I don't have a strong opinion here but it would be good to be consistent. At the moment (as well as iscsi) SLIRP, TPM and mem_prealloc options all just vanish if qemu wasn't configured with them supported. [Various other things like SDL and Windows specific options do remain to produce an error.] So maybe we should move all these options to "always exist but may produce an error". -- PMM
On Mon, Mar 25, 2013 at 02:34:01PM +0000, Peter Maydell wrote: > On 25 March 2013 14:10, Peter Lieven <pl@kamp.de> wrote: > > Am 25.03.2013 um 15:00 schrieb Stefan Hajnoczi <stefanha@gmail.com>: > >> On Thu, Mar 21, 2013 at 01:07:11PM +0100, Peter Lieven wrote: > >>> this patch adds a check for qemu_find_opts("iscsi") returning > >>> NULL instead of blindly passing the result to qemu_opts_parse(). > >>> > >>> Signed-off-by: Peter Lieven <pl@kamp.de> > >>> --- > >>> vl.c | 9 ++++++--- > >>> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> Why is this change necessary? > > > > a) just to make sure we check the result of qemu_find_opts for NULL in case > > a bug stops registering the opts again > > It's OK to crash for "can't happen" cases IMHO. > > > b) to have an error output if libiscsi is not compiled in and someone passes > > an iscsi flag (analogue to the error if you supply -spice XXX) > > I don't have a strong opinion here but it would be good to be > consistent. At the moment (as well as iscsi) SLIRP, TPM and > mem_prealloc options all just vanish if qemu wasn't configured > with them supported. [Various other things like SDL and Windows > specific options do remain to produce an error.] So maybe we > should move all these options to "always exist but may produce > an error". Agreed. Either this patch should be dropped because it's not strictly necessary. Or we should be consistent and clean up all of vl.c:main(). There's not much point of just changing -iscsi. Stefan
diff --git a/vl.c b/vl.c index 315d43d..69babbf 100644 --- a/vl.c +++ b/vl.c @@ -3224,14 +3224,17 @@ int main(int argc, char **argv, char **envp) exit(1); } break; -#ifdef CONFIG_LIBISCSI case QEMU_OPTION_iscsi: - opts = qemu_opts_parse(qemu_find_opts("iscsi"), optarg, 0); + olist = qemu_find_opts("iscsi"); + if (!olist) { + fprintf(stderr, "iscsi is not supported by this qemu build.\n"); + exit(1); + } + opts = qemu_opts_parse(olist, optarg, 0); if (!opts) { exit(1); } break; -#endif #ifdef CONFIG_SLIRP case QEMU_OPTION_tftp: legacy_tftp_prefix = optarg;
this patch adds a check for qemu_find_opts("iscsi") returning NULL instead of blindly passing the result to qemu_opts_parse(). Signed-off-by: Peter Lieven <pl@kamp.de> --- vl.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)