[Qemu-devel] [PATCH 1/2] vl.c: call bdrv_init_with_whitelist() before cmdline parsing

[Qemu-devel] [PATCH 1/2] vl.c: call bdrv_init_with_whitelist() before cmdline parsing

[Peter Lieven]

commit 4d454574 "qemu-option: move standard option definitions
out of qemu-config.c" broke support for commandline option
groups that where registered during bdrv_init(). In particular
support for -iscsi options was broken since that commit.

Fix by moving the bdrv_init_with_whitelist() before command
line argument parsing.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 vl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index aeed7f4..315d43d 100644
--- a/vl.c
+++ b/vl.c
@@ -2935,6 +2935,8 @@ int main(int argc, char **argv, char **envp)
 
     nb_numa_nodes = 0;
     nb_nics = 0;
+    
+    bdrv_init_with_whitelist();
 
     autostart= 1;
 
@@ -4194,8 +4196,6 @@ int main(int argc, char **argv, char **envp)
 
     cpu_exec_init_all();
 
-    bdrv_init_with_whitelist();
-
     blk_mig_init();
 
     /* open the virtual block devices */
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] vl.c: throw an error if iscsi is not supported

[Peter Lieven]

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(-)

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;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/2] vl.c: throw an error if iscsi is not supported

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH 1/2] vl.c: call bdrv_init_with_whitelist() before cmdline parsing

[Stefan Hajnoczi]

On Thu, Mar 21, 2013 at 01:07:10PM +0100, Peter Lieven wrote:
> commit 4d454574 "qemu-option: move standard option definitions
> out of qemu-config.c" broke support for commandline option
> groups that where registered during bdrv_init(). In particular
> support for -iscsi options was broken since that commit.
> 
> Fix by moving the bdrv_init_with_whitelist() before command
> line argument parsing.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  vl.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

> diff --git a/vl.c b/vl.c
> index aeed7f4..315d43d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2935,6 +2935,8 @@ int main(int argc, char **argv, char **envp)
>  
>      nb_numa_nodes = 0;
>      nb_nics = 0;
> +    

I removed the trailing whitespace here.  git-am(1) warns about it.

Re: [Qemu-devel] [PATCH 2/2] vl.c: throw an error if iscsi is not supported

[Peter Lieven]

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

Re: [Qemu-devel] [PATCH 2/2] vl.c: throw an error if iscsi is not supported

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 2/2] vl.c: throw an error if iscsi is not supported

[Stefan Hajnoczi]

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