Patchwork [2/2] vl.c: throw an error if iscsi is not supported

login
register
mail settings
Submitter Peter Lieven
Date March 21, 2013, 12:07 p.m.
Message ID <1363867631-5859-2-git-send-email-pl@kamp.de>
Download mbox | patch
Permalink /patch/229649/
State New
Headers show

Comments

Peter Lieven - March 21, 2013, 12:07 p.m.
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(-)
Stefan Hajnoczi - March 25, 2013, 2 p.m.
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
Peter Lieven - March 25, 2013, 2:10 p.m.
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
Peter Maydell - March 25, 2013, 2:34 p.m.
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
Stefan Hajnoczi - March 26, 2013, 9:26 a.m.
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

Patch

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;