Xen Developers, FWIW, in trying to compile mini-os on Xen 4.1.2, I get a variable set but not used warning for variable ''message'' in function init_kbdfront. (Looks like another one just like it, in function init_fbfront, from the same file.) My source won''t compile with these warnings. I could not find a patch for this in xen-4.1-testing.hg Xenbits. It looks like a conditional printk got left out after the label ''abort_transaction''. I don''t understand fbfront.c well enough to suggest a patch. Its low priority for me, because I am going to stick some plausible code in there for now. Sincerely, John ---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942
On 08/02/2012 14:15, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:> On Mon, 2012-02-06 at 19:05 +0000, John McDermott CIV wrote: >> Xen Developers, >> >> FWIW, in trying to compile mini-os on Xen 4.1.2, I get a variable set >> but not used warning for variable ''message'' in function init_kbdfront. >> (Looks like another one just like it, in function init_fbfront, from >> the same file.) My source won''t compile with these warnings. I could >> not find a patch for this in xen-4.1-testing.hg Xenbits. > > This new, enabled by default, compiler warning is very annoying, even > though it happens to be correct. Which distro has done it? (or maybe > it''s just new enough gcc which is to blame).We should be disablign this warning in Config.mk, where we add -Wno-unused-but-set-variable to CFLAGS. Perhaps mini-os needs to do the same, if it is creating its own CFLAGS? Note that we add it in Config.mk via $(call cc-option-add ...) because older gcc versions do not have this warning. -- Keir>> It looks like a conditional printk got left out after the label >> ''abort_transaction''. I don''t understand fbfront.c well enough to >> suggest a patch. Its low priority for me, because I am going to stick >> some plausible code in there for now. > > I think an unconditional printk including the %s and message is all > which is required here. If you''ve got some plausible code you may as > well post it. > > Thanks, > Ian. > >> >> Sincerely, >> >> John >> ---- >> What is the formal meaning of the one-line program >> #include "/dev/tty" >> >> J.P. McDermott building 12 >> Code 5542 john.mcdermott@nrl.navy.mil >> Naval Research Laboratory voice: +1 202.404.8301 >> Washington, DC 20375, US fax: +1 202.404.7942 >> >> >> >> >> >> >> >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> lists.xensource.com/xen-devel > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > lists.xensource.com/xen-devel
On Mon, 2012-02-06 at 19:05 +0000, John McDermott CIV wrote:> Xen Developers, > > FWIW, in trying to compile mini-os on Xen 4.1.2, I get a variable set > but not used warning for variable ''message'' in function init_kbdfront. > (Looks like another one just like it, in function init_fbfront, from > the same file.) My source won''t compile with these warnings. I could > not find a patch for this in xen-4.1-testing.hg Xenbits.This new, enabled by default, compiler warning is very annoying, even though it happens to be correct. Which distro has done it? (or maybe it''s just new enough gcc which is to blame).> It looks like a conditional printk got left out after the label > ''abort_transaction''. I don''t understand fbfront.c well enough to > suggest a patch. Its low priority for me, because I am going to stick > some plausible code in there for now.I think an unconditional printk including the %s and message is all which is required here. If you''ve got some plausible code you may as well post it. Thanks, Ian.> > Sincerely, > > John > ---- > What is the formal meaning of the one-line program > #include "/dev/tty" > > J.P. McDermott building 12 > Code 5542 john.mcdermott@nrl.navy.mil > Naval Research Laboratory voice: +1 202.404.8301 > Washington, DC 20375, US fax: +1 202.404.7942 > > > > > > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > lists.xensource.com/xen-devel
Ian, Agreed; this warning is very annoying. The problem is in several files, like a design pattern issue. So far it has popped up in fbfront.c, blkfront.c, and netfront.c, but the code is nice clean code, so the problem is very regular. We are running Fedora 16 and Xen 4.1.2. ---- [mc@xenpro3 ~]$ gcc --version gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-1) Copyright (C) 2011 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ---- Here is an example of what I have been doing to get rid of the warning: ---- [mc@xenpro3 xenon]$ hg diff extras/mini-os/fbfront.c diff -r 8cfe801d065d extras/mini-os/fbfront.c --- a/extras/mini-os/fbfront.c Mon Feb 06 08:37:59 2012 -0500 +++ b/extras/mini-os/fbfront.c Wed Feb 08 09:52:06 2012 -0500 @@ -142,6 +142,9 @@ abort_transaction: free(err); err = xenbus_transaction_end(xbt, 1, &retry); + if (message) { + printk("Abort transaction %s", message); + } goto error; done: @@ -503,6 +506,9 @@ abort_transaction: free(err); err = xenbus_transaction_end(xbt, 1, &retry); + if (message) { + printk("Abort transaction %s", message); + } goto error; done: [mc@xenpro3 xenon]$ ---- On Feb 8, 2012, at 9:15 AM, Ian Campbell wrote:> On Mon, 2012-02-06 at 19:05 +0000, John McDermott CIV wrote: >> Xen Developers, >> >> FWIW, in trying to compile mini-os on Xen 4.1.2, I get a variable set >> but not used warning for variable ''message'' in function init_kbdfront. >> (Looks like another one just like it, in function init_fbfront, from >> the same file.) My source won''t compile with these warnings. I could >> not find a patch for this in xen-4.1-testing.hg Xenbits. > > This new, enabled by default, compiler warning is very annoying, even > though it happens to be correct. Which distro has done it? (or maybe > it''s just new enough gcc which is to blame). > >> It looks like a conditional printk got left out after the label >> ''abort_transaction''. I don''t understand fbfront.c well enough to >> suggest a patch. Its low priority for me, because I am going to stick >> some plausible code in there for now. > > I think an unconditional printk including the %s and message is all > which is required here. If you''ve got some plausible code you may as > well post it. > > Thanks, > Ian. > >> >> Sincerely, >> >> John >> ---- >> What is the formal meaning of the one-line program >> #include "/dev/tty" >> >> J.P. McDermott building 12 >> Code 5542 john.mcdermott@nrl.navy.mil >> Naval Research Laboratory voice: +1 202.404.8301 >> Washington, DC 20375, US fax: +1 202.404.7942 >> >> >> >> >> >> >> >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> lists.xensource.com/xen-devel >---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942
On Wed, 2012-02-08 at 07:19 +0000, Keir Fraser wrote:> On 08/02/2012 14:15, "Ian Campbell" <Ian.Campbell@citrix.com> wrote: > > > On Mon, 2012-02-06 at 19:05 +0000, John McDermott CIV wrote: > >> Xen Developers, > >> > >> FWIW, in trying to compile mini-os on Xen 4.1.2, I get a variable set > >> but not used warning for variable ''message'' in function init_kbdfront. > >> (Looks like another one just like it, in function init_fbfront, from > >> the same file.) My source won''t compile with these warnings. I could > >> not find a patch for this in xen-4.1-testing.hg Xenbits. > > > > This new, enabled by default, compiler warning is very annoying, even > > though it happens to be correct. Which distro has done it? (or maybe > > it''s just new enough gcc which is to blame). > > We should be disablign this warning in Config.mk, where we add > -Wno-unused-but-set-variable to CFLAGS. Perhaps mini-os needs to do the > same, if it is creating its own CFLAGS? Note that we add it in Config.mk via > $(call cc-option-add ...) because older gcc versions do not have this > warning.I imagine that would look like the following, John does this work for you? On the other hand although this compiler warning is annoying these particular ones do seem to be real issues worth fixing, if only to improve the error reporting. Ian. # HG changeset patch # User Ian Campbell <ian.campbell@citrix.com> # Date 1328778190 0 # Node ID 72db20f4f6389649680e13d8ad6d96b5ed9d576f # Parent 7f2a3ca9d749c65336aca04617bb0a038445fb49 mini-os: Add -Wno-unused-but-set-variable when compiling mini-oss too. Disabled for the main Xen build in 23368:0f670f5146c8 but mini-os has it''s own CFLAGS. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff -r 7f2a3ca9d749 -r 72db20f4f638 extras/mini-os/minios.mk --- a/extras/mini-os/minios.mk Thu Feb 09 09:03:10 2012 +0000 +++ b/extras/mini-os/minios.mk Thu Feb 09 09:03:10 2012 +0000 @@ -9,6 +9,7 @@ debug = y DEF_CFLAGS += -fno-builtin -Wall -Werror -Wredundant-decls -Wno-format -Wno-redundant-decls DEF_CFLAGS += $(call cc-option,$(CC),-fno-stack-protector,) DEF_CFLAGS += $(call cc-option,$(CC),-fgnu89-inline) +DEF_CFLAGS += $(call cc-option,$(CC),-Wno-unused-but-set-variable) DEF_CFLAGS += -Wstrict-prototypes -Wnested-externs -Wpointer-arith -Winline DEF_CPPFLAGS += -D__XEN_INTERFACE_VERSION__=$(XEN_INTERFACE_VERSION)
On Wed, 2012-02-08 at 15:01 +0000, John McDermott CIV wrote:> Ian, > > Agreed; this warning is very annoying. The problem is in several > files, like a design pattern issue. So far it has popped up in > fbfront.c, blkfront.c, and netfront.c, but the code is nice clean > code, so the problem is very regular. We are running Fedora 16 and Xen > 4.1.2. > > ---- > > [mc@xenpro3 ~]$ gcc --version > gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-1) > Copyright (C) 2011 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > ---- > > Here is an example of what I have been doing to get rid of the warning: > > ---- > [mc@xenpro3 xenon]$ hg diff extras/mini-os/fbfront.c > diff -r 8cfe801d065d extras/mini-os/fbfront.c > --- a/extras/mini-os/fbfront.c Mon Feb 06 08:37:59 2012 -0500 > +++ b/extras/mini-os/fbfront.c Wed Feb 08 09:52:06 2012 -0500 > @@ -142,6 +142,9 @@ > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + if (message) { > + printk("Abort transaction %s", message); > + }I''d be tempted to make these unconditional and then fixup any resultant "variable message is used but not initialised" type warnings since I expect we should always have a reason for getting to this particular point so it would be a bug not to set message at that time. Would you mind resending this with a Signed-off-by as per wiki.xen.org/wiki/SubmittingXenPatches ? From what you say above I guess you also have similar fixes to the other drivers, I think it would be fine to bundle all those similar fixes into a single patch. Thanks, Ian.> goto error; > > done: > @@ -503,6 +506,9 @@ > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + if (message) { > + printk("Abort transaction %s", message); > + } > goto error; > > > done: > [mc@xenpro3 xenon]$ > ---- > > On Feb 8, 2012, at 9:15 AM, Ian Campbell wrote: > > > On Mon, 2012-02-06 at 19:05 +0000, John McDermott CIV wrote: > >> Xen Developers, > >> > >> FWIW, in trying to compile mini-os on Xen 4.1.2, I get a variable set > >> but not used warning for variable ''message'' in function init_kbdfront. > >> (Looks like another one just like it, in function init_fbfront, from > >> the same file.) My source won''t compile with these warnings. I could > >> not find a patch for this in xen-4.1-testing.hg Xenbits. > > > > This new, enabled by default, compiler warning is very annoying, even > > though it happens to be correct. Which distro has done it? (or maybe > > it''s just new enough gcc which is to blame). > > > >> It looks like a conditional printk got left out after the label > >> ''abort_transaction''. I don''t understand fbfront.c well enough to > >> suggest a patch. Its low priority for me, because I am going to stick > >> some plausible code in there for now. > > > > I think an unconditional printk including the %s and message is all > > which is required here. If you''ve got some plausible code you may as > > well post it. > > > > Thanks, > > Ian. > > > >> > >> Sincerely, > >> > >> John > >> ---- > >> What is the formal meaning of the one-line program > >> #include "/dev/tty" > >> > >> J.P. McDermott building 12 > >> Code 5542 john.mcdermott@nrl.navy.mil > >> Naval Research Laboratory voice: +1 202.404.8301 > >> Washington, DC 20375, US fax: +1 202.404.7942 > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> _______________________________________________ > >> Xen-devel mailing list > >> Xen-devel@lists.xensource.com > >> lists.xensource.com/xen-devel > > > > ---- > What is the formal meaning of the one-line program > #include "/dev/tty" > > J.P. McDermott building 12 > Code 5542 john.mcdermott@nrl.navy.mil > Naval Research Laboratory voice: +1 202.404.8301 > Washington, DC 20375, US fax: +1 202.404.7942 > > > > > > > > > >
Ian, Here is the first patch. There are other unused variable issues not so easily fixed, so I am putting them in another patch that should come out later today, FWIW. The changeset id in this patch is bogus. It is based on our own Mercurial repo, not Xenbits. The code is from the RedHat source RPM xen-4.1.2-2.fc16.src.rpm ---- ---- Sincerely, John On Feb 9, 2012, at 4:10 AM, Ian Campbell wrote:> > Would you mind resending this with a Signed-off-by as per > wiki.xen.org/wiki/SubmittingXenPatches ? From what you say above > I guess you also have similar fixes to the other drivers, I think it > would be fine to bundle all those similar fixes into a single patch. > > Thanks, > Ian.---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com lists.xensource.com/xen-devel
On Thu, 2012-02-09 at 15:28 +0000, John McDermott CIV wrote:> mini-os: stop compiler complaint about unused variables > > gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-1) complains about unused > variables > in mini-os drivers > > Signed-off-by: John McDermott <john.mcdermott@nrl.navy.mil>Acked-by: Ian Campbell <ian.campbell@citrix.com> Although perhaps "$FOO abort transaction", for $FOO in blkfront,netfront etc might be more helpful if the message actually happens?> > diff -r 3faf3f1c25fc extras/mini-os/blkfront.c > --- a/extras/mini-os/blkfront.c Mon Jan 09 08:56:52 2012 -0500 > +++ b/extras/mini-os/blkfront.c Thu Feb 09 09:47:34 2012 -0500 > @@ -171,6 +171,7 @@ again: > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + printk("Abort transaction %s\n", message); > goto error; > > done: > diff -r 3faf3f1c25fc extras/mini-os/console/xencons_ring.c > --- a/extras/mini-os/console/xencons_ring.c Mon Jan 09 08:56:52 > 2012 -0500 > +++ b/extras/mini-os/console/xencons_ring.c Thu Feb 09 09:47:34 > 2012 -0500 > @@ -317,6 +317,7 @@ again: > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + printk("Abort transaction %s\n", message); > goto error; > > done: > diff -r 3faf3f1c25fc extras/mini-os/fbfront.c > --- a/extras/mini-os/fbfront.c Mon Jan 09 08:56:52 2012 -0500 > +++ b/extras/mini-os/fbfront.c Thu Feb 09 09:47:34 2012 -0500 > @@ -142,6 +142,7 @@ again: > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + printk("Abort transaction %s\n", message); > goto error; > > done: > @@ -503,6 +504,7 @@ again: > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + printk("Abort transaction %s\n", message); > goto error; > > done: > diff -r 3faf3f1c25fc extras/mini-os/netfront.c > --- a/extras/mini-os/netfront.c Mon Jan 09 08:56:52 2012 -0500 > +++ b/extras/mini-os/netfront.c Thu Feb 09 09:47:34 2012 -0500 > @@ -421,6 +421,7 @@ again: > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + printk("Abort transaction %s\n", message); > goto error; > > done: > diff -r 3faf3f1c25fc extras/mini-os/pcifront.c > --- a/extras/mini-os/pcifront.c Mon Jan 09 08:56:52 2012 -0500 > +++ b/extras/mini-os/pcifront.c Thu Feb 09 09:47:34 2012 -0500 > @@ -222,6 +222,7 @@ again: > abort_transaction: > free(err); > err = xenbus_transaction_end(xbt, 1, &retry); > + printk("Abort transaction %s\n", message); > goto error; > > done: > >
John McDermott CIV writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"):> Here is the first patch. There are other unused variable issues not so easily fixed, so I am putting them in another patch that should come out later today, FWIW.Great, thanks. Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>> The changeset id in this patch is bogus. It is based on our own Mercurial repo, not Xenbits. The code is from the RedHat source RPM xen-4.1.2-2.fc16.src.rpmNoted, ta. Ian.
Keir Fraser writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"):> We should be disablign this warning in Config.mk, where we add > -Wno-unused-but-set-variable to CFLAGS. Perhaps mini-os needs to do the > same, if it is creating its own CFLAGS? Note that we add it in Config.mk via > $(call cc-option-add ...) because older gcc versions do not have this > warning.Personally in this case I like the new warning. Shall we at least wait with disabling it until we get an actual false positive ? Ian.
On Thu, 2012-02-09 at 16:08 +0000, Ian Jackson wrote:> Keir Fraser writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"): > > We should be disablign this warning in Config.mk, where we add > > -Wno-unused-but-set-variable to CFLAGS. Perhaps mini-os needs to do the > > same, if it is creating its own CFLAGS? Note that we add it in Config.mk via > > $(call cc-option-add ...) because older gcc versions do not have this > > warning. > > Personally in this case I like the new warning. Shall we at least > wait with disabling it until we get an actual false positive ?Yes, lets. Ian
Ian, There is also an unused variable warning in xenbus/xenbus.c. I think the compiler should not be warning about this, as the reasonable definition of the local DEBUG macro is the cause? Anyways, here is an ugly fix that shows where the problem is raised. I tried the old trick of defining the variables as volatile, but the new gcc won''t let it go by. It won''t hurt my feelings if you don''t use this patch. Sincerely, John On Feb 9, 2012, at 11:09 AM, Ian Campbell wrote:> On Thu, 2012-02-09 at 16:08 +0000, Ian Jackson wrote: >> Keir Fraser writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"): >>> We should be disablign this warning in Config.mk, where we add >>> -Wno-unused-but-set-variable to CFLAGS. Perhaps mini-os needs to do the >>> same, if it is creating its own CFLAGS? Note that we add it in Config.mk via >>> $(call cc-option-add ...) because older gcc versions do not have this >>> warning. >> >> Personally in this case I like the new warning. Shall we at least >> wait with disabling it until we get an actual false positive ? > > Yes, lets. > > Ian---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com lists.xensource.com/xen-devel
John McDermott CIV writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"):> There is also an unused variable warning in xenbus/xenbus.c. I think the compiler should not be warning about this, as the reasonable definition of the local DEBUG macro is the cause? Anyways, here is an ugly fix that shows where the problem is raised. I tried the old trick of defining the variables as volatile, but the new gcc won''t let it go by. It won''t hurt my feelings if you don''t use this patch.Urgh, that "fix" really is very ugly as you say. I think I''m sold on disable the warning. Ian.
Ian, I would expect most use of mini-os would involve lots of locally hacking the source anyways (we do) and likely to raise this warning again. So it is probably the best choice to disable? Sincerely, John ---- On Feb 9, 2012, at 12:20 PM, Ian Jackson wrote:> John McDermott CIV writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"): >> There is also an unused variable warning in xenbus/xenbus.c. I think the compiler should not be warning about this, as the reasonable definition of the local DEBUG macro is the cause? Anyways, here is an ugly fix that shows where the problem is raised. I tried the old trick of defining the variables as volatile, but the new gcc won''t let it go by. It won''t hurt my feelings if you don''t use this patch. > > Urgh, that "fix" really is very ugly as you say. I think I''m sold on > disable the warning. > > Ian.---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942
On Thu, 2012-02-09 at 17:20 +0000, Ian Jackson wrote:> John McDermott CIV writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"): > > There is also an unused variable warning in xenbus/xenbus.c. I think the compiler should not be warning about this, as the reasonable definition of the local DEBUG macro is the cause? Anyways, here is an ugly fix that shows where the problem is raised. I tried the old trick of defining the variables as volatile, but the new gcc won''t let it go by. It won''t hurt my feelings if you don''t use this patch. > > Urgh, that "fix" really is very ugly as you say. I think I''m sold on > disable the warning.Please see <1328778306.6133.102.camel@zakaz.uk.xensource.com> for that patch. But: In this case:> @@ -327,13 +331,14 @@ static int allocate_xenbus_id(void) > /* Initialise xenbus. */ > void init_xenbus(void) > { > - int err; > + COND(int err;) > printk("Initialising xenbus\n"); > DEBUG("init_xenbus called.\n"); > xenstore_buf = mfn_to_virt(start_info.store_mfn); > create_thread("xenstore", xenbus_thread_func, NULL); > DEBUG("buf at %p.\n", xenstore_buf); > - err = bind_evtchn(start_info.store_evtchn, > + > + COND(err = ) bind_evtchn(start_info.store_evtchn, > xenbus_evtchn_handler, > NULL); > unmask_evtchn(start_info.store_evtchn);I''d be tempted to suggest that either the DEBUG becomes a normal printk or is removed. I''d prefer the former since it is a useful init time message which is only printed once (maybe move the "Initialsing xenbus" printk to the end "Initialised xenbus on IRQ%d MFN %#lx" or something) In this case:> @@ -475,11 +480,17 @@ static void xenbus_debug_msg(const char > { "print", sizeof("print") }, > { msg, len }, > { "", 1 }}; > +#ifdef XENBUS_DEBUG > struct xsd_sockmsg *reply; > +#endif > > +#ifdef XENBUS_DEBUG > reply = xenbus_msg_reply(XS_DEBUG, 0, req, ARRAY_SIZE(req)); > DEBUG("Got a reply, type %d, id %d, len %d.\n", > reply->type, reply->req_id, reply->len); > +#else > + xenbus_msg_reply(XS_DEBUG, 0, req, ARRAY_SIZE(req)); > +#endif > } > > /* List the contents of a directory. Returns a malloc()ed array ofI think all the DEBUG''s reached from "test_xenbus()" can just become printks -- the calls from test_xenbus don''t serve any other purpose than to print those messages. This includes the DEBUG message at stake here. Ian.
Ian, So like this? I took out the linefeed between the 2 printk in init_xenbus, to save a line of terminal output. (Mercurial foo is our internal number.) Sincerely, John ---- # HG changeset patch # Parent 4b43dc4c31c3daa6ca2544af062044210f7ad189 mini-os: stop compiler complaint about unused variables gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-1) complains about unused variables in mini-os drivers Signed-off-by: John McDermott <john.mcdermott@nrl.navy.mil> diff -r 4b43dc4c31c3 extras/mini-os/xenbus/xenbus.c --- a/extras/mini-os/xenbus/xenbus.c Thu Feb 09 09:47:34 2012 -0500 +++ b/extras/mini-os/xenbus/xenbus.c Mon Feb 13 09:50:12 2012 -0500 @@ -328,16 +328,16 @@ static int allocate_xenbus_id(void) void init_xenbus(void) { int err; - printk("Initialising xenbus\n"); - DEBUG("init_xenbus called.\n"); + printk("Initialising xenbus. "); + printk("init_xenbus called.\n"); xenstore_buf = mfn_to_virt(start_info.store_mfn); create_thread("xenstore", xenbus_thread_func, NULL); - DEBUG("buf at %p.\n", xenstore_buf); + printk("buf at %p.\n", xenstore_buf); err = bind_evtchn(start_info.store_evtchn, xenbus_evtchn_handler, NULL); unmask_evtchn(start_info.store_evtchn); - DEBUG("xenbus on irq %d\n", err); + printk("xenbus on irq %d\n", err); } void fini_xenbus(void) @@ -478,7 +478,7 @@ static void xenbus_debug_msg(const char struct xsd_sockmsg *reply; reply = xenbus_msg_reply(XS_DEBUG, 0, req, ARRAY_SIZE(req)); - DEBUG("Got a reply, type %d, id %d, len %d.\n", + printk("Got a reply, type %d, id %d, len %d.\n", reply->type, reply->req_id, reply->len); } On Feb 13, 2012, at 7:43 AM, Ian Campbell wrote:> I think all the DEBUG''s reached from "test_xenbus()" can just become > printks -- the calls from test_xenbus don''t serve any other purpose than > to print those messages. This includes the DEBUG message at stake here. > > Ian.---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942
On Mon, 2012-02-13 at 14:53 +0000, John McDermott CIV wrote:> Ian, > > So like this?I was thinking more like the following, e..g change all the test_* DEBUG into print (for consistency) and make the init_xenbus messages more useful while also using the unused var. Only tested with my compiler which does not complain in the same way as yours. Ian. # HG changeset patch # User Ian Campbell <ian.campbell@citrix.com> # Date 1329145492 0 # Node ID 858955a31cf51baef86c885af3db7fc85a04ec17 # Parent 4ed7fb2c7bd837a33ef7abbd5fbef6cc4c9992df minios: Remove unused variables warnings s/DEBUG/printk/ in test_xenbus and all associated do_*_test+xenbus_dbg_message and always print the IRQ and MFN used by the xenbus on init. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff -r 4ed7fb2c7bd8 -r 858955a31cf5 extras/mini-os/xenbus/xenbus.c --- a/extras/mini-os/xenbus/xenbus.c Mon Feb 13 13:00:36 2012 +0000 +++ b/extras/mini-os/xenbus/xenbus.c Mon Feb 13 15:04:52 2012 +0000 @@ -328,7 +328,6 @@ static int allocate_xenbus_id(void) void init_xenbus(void) { int err; - printk("Initialising xenbus\n"); DEBUG("init_xenbus called.\n"); xenstore_buf = mfn_to_virt(start_info.store_mfn); create_thread("xenstore", xenbus_thread_func, NULL); @@ -337,7 +336,8 @@ void init_xenbus(void) xenbus_evtchn_handler, NULL); unmask_evtchn(start_info.store_evtchn); - DEBUG("xenbus on irq %d\n", err); + printk("xenbus initialised on irq %d mfn %#lx\n", + err, start_info.store_mfn); } void fini_xenbus(void) @@ -478,7 +478,7 @@ static void xenbus_debug_msg(const char struct xsd_sockmsg *reply; reply = xenbus_msg_reply(XS_DEBUG, 0, req, ARRAY_SIZE(req)); - DEBUG("Got a reply, type %d, id %d, len %d.\n", + printk("Got a reply, type %d, id %d, len %d.\n", reply->type, reply->req_id, reply->len); } @@ -752,16 +752,16 @@ static void do_ls_test(const char *pre) char **dirs, *msg; int x; - DEBUG("ls %s...\n", pre); + printk("ls %s...\n", pre); msg = xenbus_ls(XBT_NIL, pre, &dirs); if (msg) { - DEBUG("Error in xenbus ls: %s\n", msg); + printk("Error in xenbus ls: %s\n", msg); free(msg); return; } for (x = 0; dirs[x]; x++) { - DEBUG("ls %s[%d] -> %s\n", pre, x, dirs[x]); + printk("ls %s[%d] -> %s\n", pre, x, dirs[x]); free(dirs[x]); } free(dirs); @@ -770,68 +770,68 @@ static void do_ls_test(const char *pre) static void do_read_test(const char *path) { char *res, *msg; - DEBUG("Read %s...\n", path); + printk("Read %s...\n", path); msg = xenbus_read(XBT_NIL, path, &res); if (msg) { - DEBUG("Error in xenbus read: %s\n", msg); + printk("Error in xenbus read: %s\n", msg); free(msg); return; } - DEBUG("Read %s -> %s.\n", path, res); + printk("Read %s -> %s.\n", path, res); free(res); } static void do_write_test(const char *path, const char *val) { char *msg; - DEBUG("Write %s to %s...\n", val, path); + printk("Write %s to %s...\n", val, path); msg = xenbus_write(XBT_NIL, path, val); if (msg) { - DEBUG("Result %s\n", msg); + printk("Result %s\n", msg); free(msg); } else { - DEBUG("Success.\n"); + printk("Success.\n"); } } static void do_rm_test(const char *path) { char *msg; - DEBUG("rm %s...\n", path); + printk("rm %s...\n", path); msg = xenbus_rm(XBT_NIL, path); if (msg) { - DEBUG("Result %s\n", msg); + printk("Result %s\n", msg); free(msg); } else { - DEBUG("Success.\n"); + printk("Success.\n"); } } /* Simple testing thing */ void test_xenbus(void) { - DEBUG("Doing xenbus test.\n"); + printk("Doing xenbus test.\n"); xenbus_debug_msg("Testing xenbus...\n"); - DEBUG("Doing ls test.\n"); + printk("Doing ls test.\n"); do_ls_test("device"); do_ls_test("device/vif"); do_ls_test("device/vif/0"); - DEBUG("Doing read test.\n"); + printk("Doing read test.\n"); do_read_test("device/vif/0/mac"); do_read_test("device/vif/0/backend"); - DEBUG("Doing write test.\n"); + printk("Doing write test.\n"); do_write_test("device/vif/0/flibble", "flobble"); do_read_test("device/vif/0/flibble"); do_write_test("device/vif/0/flibble", "widget"); do_read_test("device/vif/0/flibble"); - DEBUG("Doing rm test.\n"); + printk("Doing rm test.\n"); do_rm_test("device/vif/0/flibble"); do_read_test("device/vif/0/flibble"); - DEBUG("(Should have said ENOENT)\n"); + printk("(Should have said ENOENT)\n"); } /*
Ian, My compiler (Fedora 16) is happy with that. Sincerely, John ---- On Feb 13, 2012, at 10:06 AM, Ian Campbell wrote:> On Mon, 2012-02-13 at 14:53 +0000, John McDermott CIV wrote: >> Ian, >> >> So like this? > I was thinking more like the following, e..g change all the test_* DEBUG > into print (for consistency) and make the init_xenbus messages more > useful while also using the unused var. > > Only tested with my compiler which does not complain in the same way as > yours. > > Ian. > > # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1329145492 0 > # Node ID 858955a31cf51baef86c885af3db7fc85a04ec17 > # Parent 4ed7fb2c7bd837a33ef7abbd5fbef6cc4c9992df > minios: Remove unused variables warnings > > s/DEBUG/printk/ in test_xenbus and all associated do_*_test+xenbus_dbg_message > and always print the IRQ and MFN used by the xenbus on init. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > diff -r 4ed7fb2c7bd8 -r 858955a31cf5 extras/mini-os/xenbus/xenbus.c > --- a/extras/mini-os/xenbus/xenbus.c Mon Feb 13 13:00:36 2012 +0000 > +++ b/extras/mini-os/xenbus/xenbus.c Mon Feb 13 15:04:52 2012 +0000 > @@ -328,7 +328,6 @@ static int allocate_xenbus_id(void) > void init_xenbus(void) > { > int err; > - printk("Initialising xenbus\n"); > DEBUG("init_xenbus called.\n"); > xenstore_buf = mfn_to_virt(start_info.store_mfn); > create_thread("xenstore", xenbus_thread_func, NULL); > @@ -337,7 +336,8 @@ void init_xenbus(void) > xenbus_evtchn_handler, > NULL); > unmask_evtchn(start_info.store_evtchn); > - DEBUG("xenbus on irq %d\n", err); > + printk("xenbus initialised on irq %d mfn %#lx\n", > + err, start_info.store_mfn); > } > > void fini_xenbus(void) > @@ -478,7 +478,7 @@ static void xenbus_debug_msg(const char > struct xsd_sockmsg *reply; > > reply = xenbus_msg_reply(XS_DEBUG, 0, req, ARRAY_SIZE(req)); > - DEBUG("Got a reply, type %d, id %d, len %d.\n", > + printk("Got a reply, type %d, id %d, len %d.\n", > reply->type, reply->req_id, reply->len); > } > > @@ -752,16 +752,16 @@ static void do_ls_test(const char *pre) > char **dirs, *msg; > int x; > > - DEBUG("ls %s...\n", pre); > + printk("ls %s...\n", pre); > msg = xenbus_ls(XBT_NIL, pre, &dirs); > if (msg) { > - DEBUG("Error in xenbus ls: %s\n", msg); > + printk("Error in xenbus ls: %s\n", msg); > free(msg); > return; > } > for (x = 0; dirs[x]; x++) > { > - DEBUG("ls %s[%d] -> %s\n", pre, x, dirs[x]); > + printk("ls %s[%d] -> %s\n", pre, x, dirs[x]); > free(dirs[x]); > } > free(dirs); > @@ -770,68 +770,68 @@ static void do_ls_test(const char *pre) > static void do_read_test(const char *path) > { > char *res, *msg; > - DEBUG("Read %s...\n", path); > + printk("Read %s...\n", path); > msg = xenbus_read(XBT_NIL, path, &res); > if (msg) { > - DEBUG("Error in xenbus read: %s\n", msg); > + printk("Error in xenbus read: %s\n", msg); > free(msg); > return; > } > - DEBUG("Read %s -> %s.\n", path, res); > + printk("Read %s -> %s.\n", path, res); > free(res); > } > > static void do_write_test(const char *path, const char *val) > { > char *msg; > - DEBUG("Write %s to %s...\n", val, path); > + printk("Write %s to %s...\n", val, path); > msg = xenbus_write(XBT_NIL, path, val); > if (msg) { > - DEBUG("Result %s\n", msg); > + printk("Result %s\n", msg); > free(msg); > } else { > - DEBUG("Success.\n"); > + printk("Success.\n"); > } > } > > static void do_rm_test(const char *path) > { > char *msg; > - DEBUG("rm %s...\n", path); > + printk("rm %s...\n", path); > msg = xenbus_rm(XBT_NIL, path); > if (msg) { > - DEBUG("Result %s\n", msg); > + printk("Result %s\n", msg); > free(msg); > } else { > - DEBUG("Success.\n"); > + printk("Success.\n"); > } > } > > /* Simple testing thing */ > void test_xenbus(void) > { > - DEBUG("Doing xenbus test.\n"); > + printk("Doing xenbus test.\n"); > xenbus_debug_msg("Testing xenbus...\n"); > > - DEBUG("Doing ls test.\n"); > + printk("Doing ls test.\n"); > do_ls_test("device"); > do_ls_test("device/vif"); > do_ls_test("device/vif/0"); > > - DEBUG("Doing read test.\n"); > + printk("Doing read test.\n"); > do_read_test("device/vif/0/mac"); > do_read_test("device/vif/0/backend"); > > - DEBUG("Doing write test.\n"); > + printk("Doing write test.\n"); > do_write_test("device/vif/0/flibble", "flobble"); > do_read_test("device/vif/0/flibble"); > do_write_test("device/vif/0/flibble", "widget"); > do_read_test("device/vif/0/flibble"); > > - DEBUG("Doing rm test.\n"); > + printk("Doing rm test.\n"); > do_rm_test("device/vif/0/flibble"); > do_read_test("device/vif/0/flibble"); > - DEBUG("(Should have said ENOENT)\n"); > + printk("(Should have said ENOENT)\n"); > } > > /* >---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 john.mcdermott@nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942
Ian Campbell writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"):> s/DEBUG/printk/ in test_xenbus and all associated do_*_test+xenbus_dbg_message > and always print the IRQ and MFN used by the xenbus on init.This looks plausible to me. Anyone else have any comments ? Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> Ian.
Ian Campbell writes ("Re: [Xen-devel] Mini-os fbfront.c unused variable complaint"):> minios: Remove unused variables warnings > > s/DEBUG/printk/ in test_xenbus and all associated do_*_test+xenbus_dbg_message > and always print the IRQ and MFN used by the xenbus on init.Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>