[Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Peter Lieven]

recent libnfs versions support logging debug messages. Add
support for it in qemu through an URL parameter.

Example:
 qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2

Signed-off-by: Peter Lieven <pl@kamp.de>
---
v4->v5: add a comment in the code why we limit the debug level [Stefan]
v3->v4: revert to the initial version, but limit max debug level
v2->v3: use a per-drive option instead of a global one. [Stefan]
v1->v2: reworked patch to accept the debug level as a cmdline
        parameter instead of an URI parameter [Stefan]

 block/nfs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/nfs.c b/block/nfs.c
index fd79f89..ab1e267 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -36,6 +36,7 @@
 #include <nfsc/libnfs.h>
 
 #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576
+#define QEMU_NFS_MAX_DEBUG_LEVEL 2
 
 typedef struct NFSClient {
     struct nfs_context *context;
@@ -334,6 +335,17 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
             }
             nfs_set_readahead(client->context, val);
 #endif
+#ifdef LIBNFS_FEATURE_DEBUG
+        } else if (!strcmp(qp->p[i].name, "debug")) {
+            /* limit the maximum debug level to avoid potential flooding
+             * of our log files. */
+            if (val > QEMU_NFS_MAX_DEBUG_LEVEL) {
+                error_report("NFS Warning: Limiting NFS debug level"
+                             " to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
+                val = QEMU_NFS_MAX_DEBUG_LEVEL;
+            }
+            nfs_set_debug(client->context, val);
+#endif
         } else {
             error_setg(errp, "Unknown NFS parameter name: %s",
                        qp->p[i].name);
-- 
1.9.1

Re: [Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Fam Zheng]

On Mon, 11/09 08:09, Peter Lieven wrote:
> recent libnfs versions support logging debug messages. Add
> support for it in qemu through an URL parameter.
> 
> Example:
>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> v4->v5: add a comment in the code why we limit the debug level [Stefan]
> v3->v4: revert to the initial version, but limit max debug level
> v2->v3: use a per-drive option instead of a global one. [Stefan]
> v1->v2: reworked patch to accept the debug level as a cmdline
>         parameter instead of an URI parameter [Stefan]
> 
>  block/nfs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index fd79f89..ab1e267 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -36,6 +36,7 @@
>  #include <nfsc/libnfs.h>
>  
>  #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576
> +#define QEMU_NFS_MAX_DEBUG_LEVEL 2
>  
>  typedef struct NFSClient {
>      struct nfs_context *context;
> @@ -334,6 +335,17 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>              }
>              nfs_set_readahead(client->context, val);
>  #endif
> +#ifdef LIBNFS_FEATURE_DEBUG
> +        } else if (!strcmp(qp->p[i].name, "debug")) {
> +            /* limit the maximum debug level to avoid potential flooding
> +             * of our log files. */
> +            if (val > QEMU_NFS_MAX_DEBUG_LEVEL) {
> +                error_report("NFS Warning: Limiting NFS debug level"
> +                             " to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
> +                val = QEMU_NFS_MAX_DEBUG_LEVEL;
> +            }
> +            nfs_set_debug(client->context, val);
> +#endif

It would be slightly nicer to do:

        } else if (!strcmp(qp->p[i].name, "debug")) {
#ifdef LIBNFS_FEATURE_DEBUG
            /* limit the maximum debug level to avoid potential flooding
             * of our log files. */
            if (val > QEMU_NFS_MAX_DEBUG_LEVEL) {
                error_report("NFS Warning: Limiting NFS debug level"
                             " to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
                val = QEMU_NFS_MAX_DEBUG_LEVEL;
            }
            nfs_set_debug(client->context, val);
#else
            error_report("Specifying debug level is only supported by libnfs"
                         " version >= XXX");
#endif

So that you know which one out of QEMU and libnfs to upgrade when the option is
refused. :)

But it's your call. Either way:

Reviewed-by: Fam Zheng <famz@redhat.com>

>          } else {
>              error_setg(errp, "Unknown NFS parameter name: %s",
>                         qp->p[i].name);
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
> recent libnfs versions support logging debug messages. Add
> support for it in qemu through an URL parameter.
> 
> Example:
>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> v4->v5: add a comment in the code why we limit the debug level [Stefan]
> v3->v4: revert to the initial version, but limit max debug level
> v2->v3: use a per-drive option instead of a global one. [Stefan]
> v1->v2: reworked patch to accept the debug level as a cmdline
>         parameter instead of an URI parameter [Stefan]
> 
>  block/nfs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Hi Peter,
Please use my official maintainer email address <stefanha@redhat.com>
when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
hard freeze deadline.

Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
patch didn't make it.  My block-next branch will be opening on Monday
and I'll merge this patch there for QEMU 2.6.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Peter Lieven]

Am 13.11.2015 um 10:45 schrieb Stefan Hajnoczi:
> On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
>> recent libnfs versions support logging debug messages. Add
>> support for it in qemu through an URL parameter.
>>
>> Example:
>>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> v4->v5: add a comment in the code why we limit the debug level [Stefan]
>> v3->v4: revert to the initial version, but limit max debug level
>> v2->v3: use a per-drive option instead of a global one. [Stefan]
>> v1->v2: reworked patch to accept the debug level as a cmdline
>>         parameter instead of an URI parameter [Stefan]
>>
>>  block/nfs.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
> Hi Peter,
> Please use my official maintainer email address <stefanha@redhat.com>
> when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
> hard freeze deadline.

Okay. I think I used that email because you replayed to earlier versions
of this Patch from your Gmail address.

>
> Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
> patch didn't make it.  My block-next branch will be opening on Monday
> and I'll merge this patch there for QEMU 2.6.

Thats not critical in this case. I think same applies for the ATAPI stuff
we worked on? There you used also your GMail account. Or can this
be treated as a bugfix?

Peter

Re: [Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

On Fri, Nov 13, 2015 at 10:52:49AM +0100, Peter Lieven wrote:
> Am 13.11.2015 um 10:45 schrieb Stefan Hajnoczi:
> > On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
> >> recent libnfs versions support logging debug messages. Add
> >> support for it in qemu through an URL parameter.
> >>
> >> Example:
> >>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
> >>
> >> Signed-off-by: Peter Lieven <pl@kamp.de>
> >> ---
> >> v4->v5: add a comment in the code why we limit the debug level [Stefan]
> >> v3->v4: revert to the initial version, but limit max debug level
> >> v2->v3: use a per-drive option instead of a global one. [Stefan]
> >> v1->v2: reworked patch to accept the debug level as a cmdline
> >>         parameter instead of an URI parameter [Stefan]
> >>
> >>  block/nfs.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> > Hi Peter,
> > Please use my official maintainer email address <stefanha@redhat.com>
> > when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
> > hard freeze deadline.
> 
> Okay. I think I used that email because you replayed to earlier versions
> of this Patch from your Gmail address.
> 
> >
> > Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
> > patch didn't make it.  My block-next branch will be opening on Monday
> > and I'll merge this patch there for QEMU 2.6.
> 
> Thats not critical in this case. I think same applies for the ATAPI stuff
> we worked on? There you used also your GMail account. Or can this
> be treated as a bugfix?

The ATAPI patches go through John Snow.  It's up to him whether they are
considered a bug fix suitable for 2.5 or too risky.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[John Snow]

On 11/15/2015 10:25 PM, Stefan Hajnoczi wrote:
> On Fri, Nov 13, 2015 at 10:52:49AM +0100, Peter Lieven wrote:
>> Am 13.11.2015 um 10:45 schrieb Stefan Hajnoczi:
>>> On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
>>>> recent libnfs versions support logging debug messages. Add
>>>> support for it in qemu through an URL parameter.
>>>>
>>>> Example:
>>>>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>> v4->v5: add a comment in the code why we limit the debug level [Stefan]
>>>> v3->v4: revert to the initial version, but limit max debug level
>>>> v2->v3: use a per-drive option instead of a global one. [Stefan]
>>>> v1->v2: reworked patch to accept the debug level as a cmdline
>>>>         parameter instead of an URI parameter [Stefan]
>>>>
>>>>  block/nfs.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>> Hi Peter,
>>> Please use my official maintainer email address <stefanha@redhat.com>
>>> when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
>>> hard freeze deadline.
>>
>> Okay. I think I used that email because you replayed to earlier versions
>> of this Patch from your Gmail address.
>>
>>>
>>> Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
>>> patch didn't make it.  My block-next branch will be opening on Monday
>>> and I'll merge this patch there for QEMU 2.6.
>>
>> Thats not critical in this case. I think same applies for the ATAPI stuff
>> we worked on? There you used also your GMail account. Or can this
>> be treated as a bugfix?
> 
> The ATAPI patches go through John Snow.  It's up to him whether they are
> considered a bug fix suitable for 2.5 or too risky.
> 
> Stefan
> 

I've tested them pretty thoroughly; I think they're safe (ATAPI-wise)
provided the buffered DMA helpers look sane (They look sane to me.) The
modifications to the giant atapi-return-code blob are well understood at
this point and are definitely fine.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH V5] block/nfs: add support for setting debug level

[Jeff Cody]

On Fri, Nov 13, 2015 at 05:45:09PM +0800, Stefan Hajnoczi wrote:
> On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
> > recent libnfs versions support logging debug messages. Add
> > support for it in qemu through an URL parameter.
> > 
> > Example:
> >  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
> > 
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> > v4->v5: add a comment in the code why we limit the debug level [Stefan]
> > v3->v4: revert to the initial version, but limit max debug level
> > v2->v3: use a per-drive option instead of a global one. [Stefan]
> > v1->v2: reworked patch to accept the debug level as a cmdline
> >         parameter instead of an URI parameter [Stefan]
> > 
> >  block/nfs.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> 
> Hi Peter,
> Please use my official maintainer email address <stefanha@redhat.com>
> when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
> hard freeze deadline.
> 
> Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
> patch didn't make it.  My block-next branch will be opening on Monday
> and I'll merge this patch there for QEMU 2.6.
>

Actually, this should go in through my branch, right?  I didn't see it
before the hard freeze either (I was not on the cc list).  If you want
to queue it that is fine, otherwise I'll add it to my 2.6 queue.

Re: [Qemu-devel] [Qemu-block] [PATCH V5] block/nfs: add support for setting debug level

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On Fri, Nov 13, 2015 at 01:19:55PM -0500, Jeff Cody wrote:
> On Fri, Nov 13, 2015 at 05:45:09PM +0800, Stefan Hajnoczi wrote:
> > On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
> > > recent libnfs versions support logging debug messages. Add
> > > support for it in qemu through an URL parameter.
> > > 
> > > Example:
> > >  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
> > > 
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > > v4->v5: add a comment in the code why we limit the debug level [Stefan]
> > > v3->v4: revert to the initial version, but limit max debug level
> > > v2->v3: use a per-drive option instead of a global one. [Stefan]
> > > v1->v2: reworked patch to accept the debug level as a cmdline
> > >         parameter instead of an URI parameter [Stefan]
> > > 
> > >  block/nfs.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > 
> > Hi Peter,
> > Please use my official maintainer email address <stefanha@redhat.com>
> > when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
> > hard freeze deadline.
> > 
> > Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
> > patch didn't make it.  My block-next branch will be opening on Monday
> > and I'll merge this patch there for QEMU 2.6.
> >
> 
> Actually, this should go in through my branch, right?  I didn't see it
> before the hard freeze either (I was not on the cc list).  If you want
> to queue it that is fine, otherwise I'll add it to my 2.6 queue.

Yes:

$ scripts/get_maintainer.pl -f block/nfs.c
Jeff Cody <jcody@redhat.com> (maintainer:NFS)
Peter Lieven <pl@kamp.de> (maintainer:NFS)
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
qemu-block@nongnu.org (open list:NFS)

I've been involved with the review of this series for a while and
overlooked it.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH V5] block/nfs: add support for setting debug level

[Peter Lieven]

Am 13.11.2015 um 10:45 schrieb Stefan Hajnoczi:
> On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
>> recent libnfs versions support logging debug messages. Add
>> support for it in qemu through an URL parameter.
>>
>> Example:
>>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> v4->v5: add a comment in the code why we limit the debug level [Stefan]
>> v3->v4: revert to the initial version, but limit max debug level
>> v2->v3: use a per-drive option instead of a global one. [Stefan]
>> v1->v2: reworked patch to accept the debug level as a cmdline
>>         parameter instead of an URI parameter [Stefan]
>>
>>  block/nfs.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
> Hi Peter,
> Please use my official maintainer email address <stefanha@redhat.com>
> when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
> hard freeze deadline.
>
> Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
> patch didn't make it.  My block-next branch will be opening on Monday
> and I'll merge this patch there for QEMU 2.6.

Hi Stefan,

can you pick up this one for 2.6 now?

Thanks,
Peter

Re: [Qemu-devel] [Qemu-block] [PATCH V5] block/nfs: add support for setting debug level

[Jeff Cody]

On Mon, Jan 11, 2016 at 09:51:39AM +0100, Peter Lieven wrote:
> Am 13.11.2015 um 10:45 schrieb Stefan Hajnoczi:
> > On Mon, Nov 09, 2015 at 08:09:33AM +0100, Peter Lieven wrote:
> >> recent libnfs versions support logging debug messages. Add
> >> support for it in qemu through an URL parameter.
> >>
> >> Example:
> >>  qemu -cdrom nfs://127.0.0.1/iso/my.iso?debug=2
> >>
> >> Signed-off-by: Peter Lieven <pl@kamp.de>
> >> ---
> >> v4->v5: add a comment in the code why we limit the debug level [Stefan]
> >> v3->v4: revert to the initial version, but limit max debug level
> >> v2->v3: use a per-drive option instead of a global one. [Stefan]
> >> v1->v2: reworked patch to accept the debug level as a cmdline
> >>         parameter instead of an URI parameter [Stefan]
> >>
> >>  block/nfs.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> > Hi Peter,
> > Please use my official maintainer email address <stefanha@redhat.com>
> > when CCing me.  I didn't spot the mail to GMail until after the QEMU 2.5
> > hard freeze deadline.
> >
> > Only bug fixes are being merged for QEMU 2.5 now.  I'm sorry that this
> > patch didn't make it.  My block-next branch will be opening on Monday
> > and I'll merge this patch there for QEMU 2.6.
> 
> Hi Stefan,
> 
> can you pick up this one for 2.6 now?
> 
> Thanks,
> Peter
> 
>

Hi Peter,

I've applied it to my branch, for 2.6:

git://github.com/codyprime/qemu-kvm-jtc.git block


Thanks,
Jeff