kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
@ 2021-12-04  6:14 haibiao.xiao
  2021-12-08 16:38 ` Alexandru Elisei
  2021-12-09 15:57 ` Andre Przywara
  0 siblings, 2 replies; 9+ messages in thread
From: haibiao.xiao @ 2021-12-04  6:14 UTC (permalink / raw)
  To: kvm; +Cc: will, julien.thierry.kdev, haibiao.xiao

From: "haibiao.xiao" <xiaohaibiao331@outlook.com>

Command 'lvm version' works incorrect.
It is expected to print:

    # ./lvm version
    # kvm tool [KVMTOOLS_VERSION]

but the KVMTOOLS_VERSION is missed:

    # ./lvm version
    # kvm tool

The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
is included at the end of Makefile. Since the CFLAGS is a 'Simply
expanded variables' which means CFLAGS is only scanned once. So the
definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.

I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
before the CFLAGS.

Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index bb7ad3e..9afb5e3 100644
--- a/Makefile
+++ b/Makefile
@@ -17,6 +17,7 @@ export E Q
 
 include config/utilities.mak
 include config/feature-tests.mak
+-include $(OUTPUT)KVMTOOLS-VERSION-FILE
 
 CC	:= $(CROSS_COMPILE)gcc
 CFLAGS	:=
@@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
 
 KVMTOOLS-VERSION-FILE:
 	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
--include $(OUTPUT)KVMTOOLS-VERSION-FILE
-endif
+endif
\ No newline at end of file
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-04  6:14 [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION) haibiao.xiao
@ 2021-12-08 16:38 ` Alexandru Elisei
  2021-12-09  3:31   ` haibiao.xiao
  2021-12-09 15:57 ` Andre Przywara
  1 sibling, 1 reply; 9+ messages in thread
From: Alexandru Elisei @ 2021-12-08 16:38 UTC (permalink / raw)
  To: haibiao.xiao; +Cc: kvm, will, julien.thierry.kdev, haibiao.xiao

Hi,

On Sat, Dec 04, 2021 at 02:14:36PM +0800, haibiao.xiao wrote:
> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>

The subject line should be a summary of what the patch does (and perhaps
why it does it), not a description of what is broken. How about this:

Makefile: Calculate the correct kvmtool version

or something else that you prefer. Tested the patch and it works as
advertised:

Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> 
> Command 'lvm version' works incorrect.
> It is expected to print:
> 
>     # ./lvm version
>     # kvm tool [KVMTOOLS_VERSION]
> 
> but the KVMTOOLS_VERSION is missed:
> 
>     # ./lvm version
>     # kvm tool
> 
> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
> is included at the end of Makefile. Since the CFLAGS is a 'Simply
> expanded variables' which means CFLAGS is only scanned once. So the
> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
> 
> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
> before the CFLAGS.
> 
> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index bb7ad3e..9afb5e3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -17,6 +17,7 @@ export E Q
>  
>  include config/utilities.mak
>  include config/feature-tests.mak
> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
>  
>  CC	:= $(CROSS_COMPILE)gcc
>  CFLAGS	:=
> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
>  
>  KVMTOOLS-VERSION-FILE:
>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
> -endif
> +endif
> \ No newline at end of file
> -- 
> 2.32.0
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-08 16:38 ` Alexandru Elisei
@ 2021-12-09  3:31   ` haibiao.xiao
  2021-12-09 11:00     ` Alexandru Elisei
  0 siblings, 1 reply; 9+ messages in thread
From: haibiao.xiao @ 2021-12-09  3:31 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: kvm, will, julien.thierry.kdev, haibiao.xiao

Hi,

Thanks for your reply. I'd like to changed the subject line 
as you suggested. But I don't know how to deal with it, 
should I send another patch mail?

Thanks,
haibiao.xiao

On Wed, 8 Dec 2021 16:38:17 +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Sat, Dec 04, 2021 at 02:14:36PM +0800, haibiao.xiao wrote:
>> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
> 
> The subject line should be a summary of what the patch does (and perhaps
> why it does it), not a description of what is broken. How about this:
> 
> Makefile: Calculate the correct kvmtool version
> 
> or something else that you prefer. Tested the patch and it works as
> advertised:
> 
> Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> Thanks,
> Alex
> 
>>
>> Command 'lvm version' works incorrect.
>> It is expected to print:
>>
>>     # ./lvm version
>>     # kvm tool [KVMTOOLS_VERSION]
>>
>> but the KVMTOOLS_VERSION is missed:
>>
>>     # ./lvm version
>>     # kvm tool
>>
>> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
>> is included at the end of Makefile. Since the CFLAGS is a 'Simply
>> expanded variables' which means CFLAGS is only scanned once. So the
>> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
>> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
>>
>> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
>> before the CFLAGS.
>>
>> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
>> ---
>>  Makefile | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index bb7ad3e..9afb5e3 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -17,6 +17,7 @@ export E Q
>>  
>>  include config/utilities.mak
>>  include config/feature-tests.mak
>> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
>>  
>>  CC	:= $(CROSS_COMPILE)gcc
>>  CFLAGS	:=
>> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
>>  
>>  KVMTOOLS-VERSION-FILE:
>>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
>> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
>> -endif
>> +endif
>> \ No newline at end of file
>> -- 
>> 2.32.0
>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-09  3:31   ` haibiao.xiao
@ 2021-12-09 11:00     ` Alexandru Elisei
  0 siblings, 0 replies; 9+ messages in thread
From: Alexandru Elisei @ 2021-12-09 11:00 UTC (permalink / raw)
  To: haibiao.xiao; +Cc: kvm, will, julien.thierry.kdev, haibiao.xiao

Hi,

On Thu, Dec 09, 2021 at 11:31:58AM +0800, haibiao.xiao wrote:
> Hi,
> 
> Thanks for your reply. I'd like to changed the subject line 
> as you suggested. But I don't know how to deal with it, 
> should I send another patch mail?

Yes, please send version 2 of the patch. The subject line for the email
should changed to (notice the extra v2):

[PATCH v2 kvmtool] <your summary here>

You can put the Tested-by tag after your Signed-off.

Thanks,
Alex

> 
> Thanks,
> haibiao.xiao
> 
> On Wed, 8 Dec 2021 16:38:17 +0000, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Sat, Dec 04, 2021 at 02:14:36PM +0800, haibiao.xiao wrote:
> >> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
> > 
> > The subject line should be a summary of what the patch does (and perhaps
> > why it does it), not a description of what is broken. How about this:
> > 
> > Makefile: Calculate the correct kvmtool version
> > 
> > or something else that you prefer. Tested the patch and it works as
> > advertised:
> > 
> > Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > 
> > Thanks,
> > Alex
> > 
> >>
> >> Command 'lvm version' works incorrect.
> >> It is expected to print:
> >>
> >>     # ./lvm version
> >>     # kvm tool [KVMTOOLS_VERSION]
> >>
> >> but the KVMTOOLS_VERSION is missed:
> >>
> >>     # ./lvm version
> >>     # kvm tool
> >>
> >> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
> >> is included at the end of Makefile. Since the CFLAGS is a 'Simply
> >> expanded variables' which means CFLAGS is only scanned once. So the
> >> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
> >> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
> >>
> >> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
> >> before the CFLAGS.
> >>
> >> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
> >> ---
> >>  Makefile | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index bb7ad3e..9afb5e3 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -17,6 +17,7 @@ export E Q
> >>  
> >>  include config/utilities.mak
> >>  include config/feature-tests.mak
> >> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
> >>  
> >>  CC	:= $(CROSS_COMPILE)gcc
> >>  CFLAGS	:=
> >> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
> >>  
> >>  KVMTOOLS-VERSION-FILE:
> >>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> >> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
> >> -endif
> >> +endif
> >> \ No newline at end of file
> >> -- 
> >> 2.32.0
> >>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-04  6:14 [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION) haibiao.xiao
  2021-12-08 16:38 ` Alexandru Elisei
@ 2021-12-09 15:57 ` Andre Przywara
  2021-12-10  2:55   ` haibiao.xiao
  1 sibling, 1 reply; 9+ messages in thread
From: Andre Przywara @ 2021-12-09 15:57 UTC (permalink / raw)
  To: haibiao.xiao
  Cc: kvm, will, julien.thierry.kdev, haibiao.xiao, Alexandru Elisei

On Sat,  4 Dec 2021 14:14:36 +0800
"haibiao.xiao" <haibiao.xiao@zstack.io> wrote:

Hi,

> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
> 
> Command 'lvm version' works incorrect.
> It is expected to print:
> 
>     # ./lvm version
>     # kvm tool [KVMTOOLS_VERSION]
> 
> but the KVMTOOLS_VERSION is missed:
> 
>     # ./lvm version
>     # kvm tool
> 
> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
> is included at the end of Makefile. Since the CFLAGS is a 'Simply
> expanded variables' which means CFLAGS is only scanned once. So the
> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
> 
> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
> before the CFLAGS.

While this is indeed a bug that this patch fixes, I wonder if we should
actually get rid of this whole versioning attempt altogether at this
point. Originally this was following the containing kernel version, but
it is stuck ever since at v3.18, without any change.

So either we introduce proper versioning (not sure it's worth it?), or we
just remove all code that pretends to print a version number? Or just
hardcode v3.18 into the printf, at least for now? At the very least I
think we don't need a KVMTOOLS-VERSION-FILE anymore.

Cheers,
Andre

> 
> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index bb7ad3e..9afb5e3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -17,6 +17,7 @@ export E Q
>  
>  include config/utilities.mak
>  include config/feature-tests.mak
> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
>  
>  CC	:= $(CROSS_COMPILE)gcc
>  CFLAGS	:=
> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
>  
>  KVMTOOLS-VERSION-FILE:
>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
> -endif
> +endif
> \ No newline at end of file


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-09 15:57 ` Andre Przywara
@ 2021-12-10  2:55   ` haibiao.xiao
  2021-12-10 11:00     ` Andre Przywara
  0 siblings, 1 reply; 9+ messages in thread
From: haibiao.xiao @ 2021-12-10  2:55 UTC (permalink / raw)
  To: Andre Przywara
  Cc: kvm, will, julien.thierry.kdev, haibiao.xiao, Alexandru Elisei

Hi,

On Thu, 9 Dec 2021 15:57:46 +0000, Andre Przywara wrote:
> On Sat,  4 Dec 2021 14:14:36 +0800
> "haibiao.xiao" <haibiao.xiao@zstack.io> wrote:
> 
> Hi,
> 
>> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
>>
>> Command 'lvm version' works incorrect.
>> It is expected to print:
>>
>>     # ./lvm version
>>     # kvm tool [KVMTOOLS_VERSION]
>>
>> but the KVMTOOLS_VERSION is missed:
>>
>>     # ./lvm version
>>     # kvm tool
>>
>> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
>> is included at the end of Makefile. Since the CFLAGS is a 'Simply
>> expanded variables' which means CFLAGS is only scanned once. So the
>> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
>> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
>>
>> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
>> before the CFLAGS.
> 
> While this is indeed a bug that this patch fixes, I wonder if we should
> actually get rid of this whole versioning attempt altogether at this
> point. Originally this was following the containing kernel version, but
> it is stuck ever since at v3.18, without any change.
> 
> So either we introduce proper versioning (not sure it's worth it?), or we
> just remove all code that pretends to print a version number? Or just
> hardcode v3.18 into the printf, at least for now? At the very least I
> think we don't need a KVMTOOLS-VERSION-FILE anymore.
> 

Thanks for your reply. 

The reason I look at this project is tend to learn something about kvm. 
With the version number I can tell which kernel(kvm) is compatible.
 
Although it is stuck at v3.18, there still some commits in recent 
months, which means the kvmtool still changing according to the kvm 
features. So I think what's kvmtool need is a version control, but not 
remove/hardcode.

Thanks,
haibiao.xiao
> Cheers,
> Andre
> 
>>
>> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
>> ---
>>  Makefile | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index bb7ad3e..9afb5e3 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -17,6 +17,7 @@ export E Q
>>  
>>  include config/utilities.mak
>>  include config/feature-tests.mak
>> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
>>  
>>  CC	:= $(CROSS_COMPILE)gcc
>>  CFLAGS	:=
>> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
>>  
>>  KVMTOOLS-VERSION-FILE:
>>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
>> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
>> -endif
>> +endif
>> \ No newline at end of file

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-10  2:55   ` haibiao.xiao
@ 2021-12-10 11:00     ` Andre Przywara
  2021-12-13  3:19       ` haibiao.xiao
  0 siblings, 1 reply; 9+ messages in thread
From: Andre Przywara @ 2021-12-10 11:00 UTC (permalink / raw)
  To: haibiao.xiao
  Cc: kvm, will, julien.thierry.kdev, haibiao.xiao, Alexandru Elisei

On Fri, 10 Dec 2021 10:55:40 +0800
"haibiao.xiao" <haibiao.xiao@zstack.io> wrote:

Hi,

> On Thu, 9 Dec 2021 15:57:46 +0000, Andre Przywara wrote:
> > On Sat,  4 Dec 2021 14:14:36 +0800
> > "haibiao.xiao" <haibiao.xiao@zstack.io> wrote:
> > 
> > Hi,
> >   
> >> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
> >>
> >> Command 'lvm version' works incorrect.
> >> It is expected to print:
> >>
> >>     # ./lvm version
> >>     # kvm tool [KVMTOOLS_VERSION]
> >>
> >> but the KVMTOOLS_VERSION is missed:
> >>
> >>     # ./lvm version
> >>     # kvm tool
> >>
> >> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
> >> is included at the end of Makefile. Since the CFLAGS is a 'Simply
> >> expanded variables' which means CFLAGS is only scanned once. So the
> >> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
> >> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
> >>
> >> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
> >> before the CFLAGS.  
> > 
> > While this is indeed a bug that this patch fixes, I wonder if we should
> > actually get rid of this whole versioning attempt altogether at this
> > point. Originally this was following the containing kernel version, but
> > it is stuck ever since at v3.18, without any change.
> > 
> > So either we introduce proper versioning (not sure it's worth it?), or we
> > just remove all code that pretends to print a version number? Or just
> > hardcode v3.18 into the printf, at least for now? At the very least I
> > think we don't need a KVMTOOLS-VERSION-FILE anymore.
> >   
> 
> Thanks for your reply. 
> 
> The reason I look at this project is tend to learn something about kvm. 
> With the version number I can tell which kernel(kvm) is compatible.

I am afraid this is not what the version number tells you. The Linux
kernel provides a stable interface to userland, that includes KVM.
So you should be able to run any kvmtool version on any host kernel
(ignoring bugs). Granted, you will only get the features implemented in
both parts, but this only applies to new features (like PMU
virtualisation), not the main functionality.

> Although it is stuck at v3.18, there still some commits in recent 
> months, which means the kvmtool still changing according to the kvm 
> features.

Sure, we keep it alive, but adapting to new kernel features is only one
part of that. Recently we mostly improved functionality (like
adding firmware support, emulating later PCIe versions, ...) and were
fixing bugs, independent of the kernel version.

> So I think what's kvmtool need is a version control, but not 
> remove/hardcode.

So whether that's really worth it, is the question. If you need some number
to compare, distributions tend to use the date of the last commit for that.

Cheers,
Andre


> 
> Thanks,
> haibiao.xiao
> > Cheers,
> > Andre
> >   
> >>
> >> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
> >> ---
> >>  Makefile | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index bb7ad3e..9afb5e3 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -17,6 +17,7 @@ export E Q
> >>  
> >>  include config/utilities.mak
> >>  include config/feature-tests.mak
> >> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
> >>  
> >>  CC	:= $(CROSS_COMPILE)gcc
> >>  CFLAGS	:=
> >> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
> >>  
> >>  KVMTOOLS-VERSION-FILE:
> >>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> >> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
> >> -endif
> >> +endif
> >> \ No newline at end of file  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
  2021-12-10 11:00     ` Andre Przywara
@ 2021-12-13  3:19       ` haibiao.xiao
  0 siblings, 0 replies; 9+ messages in thread
From: haibiao.xiao @ 2021-12-13  3:19 UTC (permalink / raw)
  To: Andre Przywara
  Cc: kvm, will, julien.thierry.kdev, haibiao.xiao, Alexandru Elisei


Hi,

Sorry for getting back to you late.

Fri, 10 Dec 2021 11:00:02 +0000, Andre Przywara wrote:
> On Fri, 10 Dec 2021 10:55:40 +0800
> "haibiao.xiao" <haibiao.xiao@zstack.io> wrote:
> 
> Hi,
> 
>> On Thu, 9 Dec 2021 15:57:46 +0000, Andre Przywara wrote:
>>> On Sat,  4 Dec 2021 14:14:36 +0800
>>> "haibiao.xiao" <haibiao.xiao@zstack.io> wrote:
>>>
>>> Hi,
>>>   
>>>> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
>>>>
>>>> Command 'lvm version' works incorrect.
>>>> It is expected to print:
>>>>
>>>>     # ./lvm version
>>>>     # kvm tool [KVMTOOLS_VERSION]
>>>>
>>>> but the KVMTOOLS_VERSION is missed:
>>>>
>>>>     # ./lvm version
>>>>     # kvm tool
>>>>
>>>> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
>>>> is included at the end of Makefile. Since the CFLAGS is a 'Simply
>>>> expanded variables' which means CFLAGS is only scanned once. So the
>>>> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
>>>> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
>>>>
>>>> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
>>>> before the CFLAGS.  
>>>
>>> While this is indeed a bug that this patch fixes, I wonder if we should
>>> actually get rid of this whole versioning attempt altogether at this
>>> point. Originally this was following the containing kernel version, but
>>> it is stuck ever since at v3.18, without any change.
>>>
>>> So either we introduce proper versioning (not sure it's worth it?), or we
>>> just remove all code that pretends to print a version number? Or just
>>> hardcode v3.18 into the printf, at least for now? At the very least I
>>> think we don't need a KVMTOOLS-VERSION-FILE anymore.
>>>   
>>
>> Thanks for your reply. 
>>
>> The reason I look at this project is tend to learn something about kvm. 
>> With the version number I can tell which kernel(kvm) is compatible.
> 
> I am afraid this is not what the version number tells you. The Linux
> kernel provides a stable interface to userland, that includes KVM.
> So you should be able to run any kvmtool version on any host kernel
> (ignoring bugs). Granted, you will only get the features implemented in
> both parts, but this only applies to new features (like PMU
> virtualisation), not the main functionality.
> 

I thought about it again. Yes, you are right.

>> Although it is stuck at v3.18, there still some commits in recent 
>> months, which means the kvmtool still changing according to the kvm 
>> features.
> 
> Sure, we keep it alive, but adapting to new kernel features is only one
> part of that. Recently we mostly improved functionality (like
> adding firmware support, emulating later PCIe versions, ...) and were
> fixing bugs, independent of the kernel version.
> 
>> So I think what's kvmtool need is a version control, but not 
>> remove/hardcode.
> 
> So whether that's really worth it, is the question. If you need some number
> to compare, distributions tend to use the date of the last commit for that.
> 

And I reviewed the code about version generation this weekends. I found it
was already hardcode to 3.18.0 in `kvmtool/util/KVMTOOLS-VERSION-GEN`. What's
more, it leaves a way to change the version number by getting from git 
describe.

> Cheers,
> Andre
> 
> 
>>
>> Thanks,
>> haibiao.xiao
>>> Cheers,
>>> Andre
>>>   
>>>>
>>>> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
>>>> ---
>>>>  Makefile | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index bb7ad3e..9afb5e3 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -17,6 +17,7 @@ export E Q
>>>>  
>>>>  include config/utilities.mak
>>>>  include config/feature-tests.mak
>>>> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
>>>>  
>>>>  CC	:= $(CROSS_COMPILE)gcc
>>>>  CFLAGS	:=
>>>> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
>>>>  
>>>>  KVMTOOLS-VERSION-FILE:
>>>>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
>>>> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
>>>> -endif
>>>> +endif
>>>> \ No newline at end of file  

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
@ 2021-07-05  4:46 haibiao.xiao
  0 siblings, 0 replies; 9+ messages in thread
From: haibiao.xiao @ 2021-07-05  4:46 UTC (permalink / raw)
  To: kvm; +Cc: haibiao.xiao

From: "haibiao.xiao" <xiaohaibiao331@outlook.com>

Command 'lvm version' works incorrect.
It is expected to print:

    # ./lvm version
    # kvm tool [KVMTOOLS_VERSION]

but the KVMTOOLS_VERSION is missed:

    # ./lvm version
    # kvm tool

The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
is included at the end of Makefile. Since the CFLAGS is a 'Simply
expanded variables' which means CFLAGS is only scanned once. So the
definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.

I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
before the CFLAGS.
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index bb7ad3e..9afb5e3 100644
--- a/Makefile
+++ b/Makefile
@@ -17,6 +17,7 @@ export E Q
 
 include config/utilities.mak
 include config/feature-tests.mak
+-include $(OUTPUT)KVMTOOLS-VERSION-FILE
 
 CC	:= $(CROSS_COMPILE)gcc
 CFLAGS	:=
@@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
 
 KVMTOOLS-VERSION-FILE:
 	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
--include $(OUTPUT)KVMTOOLS-VERSION-FILE
-endif
+endif
\ No newline at end of file
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-12-13  3:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-04  6:14 [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION) haibiao.xiao
2021-12-08 16:38 ` Alexandru Elisei
2021-12-09  3:31   ` haibiao.xiao
2021-12-09 11:00     ` Alexandru Elisei
2021-12-09 15:57 ` Andre Przywara
2021-12-10  2:55   ` haibiao.xiao
2021-12-10 11:00     ` Andre Przywara
2021-12-13  3:19       ` haibiao.xiao
  -- strict thread matches above, loose matches on Subject: below --
2021-07-05  4:46 haibiao.xiao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).