[dpdk-dev] [PATCH] app/test: enhance power manager unit tests

[dpdk-dev] [PATCH] app/test: enhance power manager unit tests

[Reshma Pattan]

Unit Testcases are added for power_acpi_cpu_freq,
power_kvm_vm_test to improve coverage

Signed-off-by: Jananee Parthasarathy <jananeex.m.parthasarathy@intel.com>
---
 test/test/test_power_acpi_cpufreq.c |  2 +-
 test/test/test_power_kvm_vm.c       | 62 +++++++++++++++++++++++++++++++++----
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/test/test/test_power_acpi_cpufreq.c b/test/test/test_power_acpi_cpufreq.c
index 3bfd033..8da2dcc 100644
--- a/test/test/test_power_acpi_cpufreq.c
+++ b/test/test/test_power_acpi_cpufreq.c
@@ -27,7 +27,7 @@
 #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
 
 #define TEST_POWER_SYSFILE_CUR_FREQ \
-	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_cur_freq"
+	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"
 
 static uint32_t total_freq_num;
 static uint32_t freqs[TEST_POWER_FREQS_NUM_MAX];
diff --git a/test/test/test_power_kvm_vm.c b/test/test/test_power_kvm_vm.c
index 91b31c4..012ad82 100644
--- a/test/test/test_power_kvm_vm.c
+++ b/test/test/test_power_kvm_vm.c
@@ -25,12 +25,19 @@
 #define TEST_POWER_VM_LCORE_ID            0U
 #define TEST_POWER_VM_LCORE_OUT_OF_BOUNDS (RTE_MAX_LCORE+1)
 #define TEST_POWER_VM_LCORE_INVALID       1U
+#define TEMP_POWER_MANAGER_FILE_PATH  "/tmp/testpm"
+
+int guest_channel_host_connect(const char *path, unsigned int lcore_id);
+int power_kvm_vm_enable_turbo(unsigned int lcore_id);
+int power_kvm_vm_disable_turbo(unsigned int lcore_id);
 
 static int
 test_power_kvm_vm(void)
 {
 	int ret;
 	enum power_management_env env;
+	char fPath[PATH_MAX];
+	FILE *fPtr = NULL;
 
 	ret = rte_power_set_env(PM_ENV_KVM_VM);
 	if (ret != 0) {
@@ -95,12 +102,31 @@
 	/* Test initialisation of a valid lcore */
 	ret = rte_power_init(TEST_POWER_VM_LCORE_ID);
 	if (ret < 0) {
-		printf("Cannot initialise power management for lcore %u, this "
-				"may occur if environment is not configured "
-				"correctly(KVM VM) or operating in another valid "
-				"Power management environment\n", TEST_POWER_VM_LCORE_ID);
-		rte_power_unset_env();
-		return -1;
+		printf("rte_power_init failed as expected in host\n");
+		/* This test would be successful when run on VM,
+		 * in order to run in Host itself, temporary file path
+		 * is created and same is used for further communication
+		 */
+
+		snprintf(fPath, PATH_MAX, "%s.%u",
+			TEMP_POWER_MANAGER_FILE_PATH, TEST_POWER_VM_LCORE_ID);
+		fPtr = fopen(fPath, "w");
+		if (fPtr == NULL) {
+			printf(" Unable to create file\n");
+			rte_power_unset_env();
+			return -1;
+		}
+		ret = guest_channel_host_connect(TEMP_POWER_MANAGER_FILE_PATH,
+			TEST_POWER_VM_LCORE_ID);
+		if (ret == 0)
+			printf("guest_channel_host_connect successful\n");
+		else {
+			printf("guest_channel_host_connect failed\n");
+			rte_power_unset_env();
+			fclose(fPtr);
+			remove(fPath);
+			return -1;
+		}
 	}
 
 	/* Test initialisation of previously initialised lcore */
@@ -175,6 +201,22 @@
 		goto fail_all;
 	}
 
+	/* Test KVM_VM Enable Turbo of valid core */
+	ret = power_kvm_vm_enable_turbo(TEST_POWER_VM_LCORE_ID);
+	if (ret == -1) {
+		printf("power_kvm_vm_enable_turbo failed on valid lcore"
+			"%u\n", TEST_POWER_VM_LCORE_ID);
+		goto fail_all;
+	}
+
+	/* Test KVM_VM Disable Turbo of valid core */
+	ret = power_kvm_vm_disable_turbo(TEST_POWER_VM_LCORE_ID);
+	if (ret == -1) {
+		printf("power_kvm_vm_disable_turbo failed on valid lcore"
+		"%u\n", TEST_POWER_VM_LCORE_ID);
+		goto fail_all;
+	}
+
 	/* Test frequency up of valid lcore */
 	ret = rte_power_freq_up(TEST_POWER_VM_LCORE_ID);
 	if (ret != 1) {
@@ -274,10 +316,18 @@
 		return -1;
 	}
 	rte_power_unset_env();
+	if (fPtr != NULL) {
+		fclose(fPtr);
+		remove(fPath);
+	}
 	return 0;
 fail_all:
 	rte_power_exit(TEST_POWER_VM_LCORE_ID);
 	rte_power_unset_env();
+	if (fPtr != NULL) {
+		fclose(fPtr);
+		remove(fPath);
+	}
 	return -1;
 }
 #endif
-- 
1.7.12.2

Re: [dpdk-dev] [PATCH] app/test: enhance power manager unit tests

[Hunt, David]

Hi Reshma,


On 6/4/2018 2:51 PM, Reshma Pattan wrote:
> Unit Testcases are added for power_acpi_cpu_freq,
> power_kvm_vm_test to improve coverage
>
> Signed-off-by: Jananee Parthasarathy <jananeex.m.parthasarathy@intel.com>
> ---
>   test/test/test_power_acpi_cpufreq.c |  2 +-
>   test/test/test_power_kvm_vm.c       | 62 +++++++++++++++++++++++++++++++++----
>   2 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/test/test/test_power_acpi_cpufreq.c b/test/test/test_power_acpi_cpufreq.c
> index 3bfd033..8da2dcc 100644
> --- a/test/test/test_power_acpi_cpufreq.c
> +++ b/test/test/test_power_acpi_cpufreq.c
> @@ -27,7 +27,7 @@
>   #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
>   
>   #define TEST_POWER_SYSFILE_CUR_FREQ \
> -	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_cur_freq"
> +	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"

This change is OK with me, from what I can see using cpuinfo_cur_freq 
instead of
scaling_cpu_freq gives us more compatibility on a wider selection of 
operating systems.

>   
>   static uint32_t total_freq_num;
>   static uint32_t freqs[TEST_POWER_FREQS_NUM_MAX];
> diff --git a/test/test/test_power_kvm_vm.c b/test/test/test_power_kvm_vm.c
> index 91b31c4..012ad82 100644
> --- a/test/test/test_power_kvm_vm.c
> +++ b/test/test/test_power_kvm_vm.c
> @@ -25,12 +25,19 @@
>   #define TEST_POWER_VM_LCORE_ID            0U
>   #define TEST_POWER_VM_LCORE_OUT_OF_BOUNDS (RTE_MAX_LCORE+1)
>   #define TEST_POWER_VM_LCORE_INVALID       1U
> +#define TEMP_POWER_MANAGER_FILE_PATH  "/tmp/testpm"
> +
> +int guest_channel_host_connect(const char *path, unsigned int lcore_id);
> +int power_kvm_vm_enable_turbo(unsigned int lcore_id);
> +int power_kvm_vm_disable_turbo(unsigned int lcore_id);

I see here you are calling guest_channel_host_connect to "emulate" a 
virtio-serial connection
to a host. While I am not a huge fan of faking functionality, I feel 
that having these unit tests
check ABI and API breakages is more beneficial, so I'm good with it for 
this reason.

However, there's no need to have the power_kvm_vm_enable/disable_turbo() 
prototypes, as you
can just use rte_power_freq_enable_turbo() and 
rte_power_freq_disable_turbo(), which in turn
call power_kvm_vm_enable_turbo() and power_kvm_vm_disable_turbo()

>   
>   static int
>   test_power_kvm_vm(void)
>   {
>   	int ret;
>   	enum power_management_env env;
> +	char fPath[PATH_MAX];
> +	FILE *fPtr = NULL;
>   
>   	ret = rte_power_set_env(PM_ENV_KVM_VM);
>   	if (ret != 0) {
> @@ -95,12 +102,31 @@
>   	/* Test initialisation of a valid lcore */
>   	ret = rte_power_init(TEST_POWER_VM_LCORE_ID);
>   	if (ret < 0) {
> -		printf("Cannot initialise power management for lcore %u, this "
> -				"may occur if environment is not configured "
> -				"correctly(KVM VM) or operating in another valid "
> -				"Power management environment\n", TEST_POWER_VM_LCORE_ID);
> -		rte_power_unset_env();
> -		return -1;
> +		printf("rte_power_init failed as expected in host\n");
> +		/* This test would be successful when run on VM,
> +		 * in order to run in Host itself, temporary file path
> +		 * is created and same is used for further communication
> +		 */
> +
> +		snprintf(fPath, PATH_MAX, "%s.%u",
> +			TEMP_POWER_MANAGER_FILE_PATH, TEST_POWER_VM_LCORE_ID);
> +		fPtr = fopen(fPath, "w");
> +		if (fPtr == NULL) {
> +			printf(" Unable to create file\n");
> +			rte_power_unset_env();
> +			return -1;
> +		}
> +		ret = guest_channel_host_connect(TEMP_POWER_MANAGER_FILE_PATH,
> +			TEST_POWER_VM_LCORE_ID);
> +		if (ret == 0)
> +			printf("guest_channel_host_connect successful\n");
> +		else {
> +			printf("guest_channel_host_connect failed\n");
> +			rte_power_unset_env();
> +			fclose(fPtr);
> +			remove(fPath);
> +			return -1;
> +		}
>   	}
>   
>   	/* Test initialisation of previously initialised lcore */
> @@ -175,6 +201,22 @@
>   		goto fail_all;
>   	}
>   
> +	/* Test KVM_VM Enable Turbo of valid core */
> +	ret = power_kvm_vm_enable_turbo(TEST_POWER_VM_LCORE_ID);

see comment above about using rte_power_freq_enable_turbo()

> +	if (ret == -1) {
> +		printf("power_kvm_vm_enable_turbo failed on valid lcore"
> +			"%u\n", TEST_POWER_VM_LCORE_ID);
> +		goto fail_all;
> +	}
> +
> +	/* Test KVM_VM Disable Turbo of valid core */
> +	ret = power_kvm_vm_disable_turbo(TEST_POWER_VM_LCORE_ID);

see comment above about using rte_power_freq_disable_turbo()

> +	if (ret == -1) {
> +		printf("power_kvm_vm_disable_turbo failed on valid lcore"
> +		"%u\n", TEST_POWER_VM_LCORE_ID);
> +		goto fail_all;
> +	}
> +
>   	/* Test frequency up of valid lcore */
>   	ret = rte_power_freq_up(TEST_POWER_VM_LCORE_ID);
>   	if (ret != 1) {
> @@ -274,10 +316,18 @@
>   		return -1;
>   	}
>   	rte_power_unset_env();
> +	if (fPtr != NULL) {
> +		fclose(fPtr);
> +		remove(fPath);
> +	}
>   	return 0;
>   fail_all:
>   	rte_power_exit(TEST_POWER_VM_LCORE_ID);
>   	rte_power_unset_env();
> +	if (fPtr != NULL) {
> +		fclose(fPtr);
> +		remove(fPath);
> +	}
>   	return -1;
>   }
>   #endif

With the changes described above:

Acked-by: David Hunt <david.hunt@intel.com>

[dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Reshma Pattan]

Unit Testcases are added for power_acpi_cpu_freq,
power_kvm_vm_test to improve coverage

Signed-off-by: Jananee Parthasarathy <jananeex.m.parthasarathy@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
V3: removed unnecessary extern funtion prototypes.
---
 test/test/test_power_acpi_cpufreq.c |  2 +-
 test/test/test_power_kvm_vm.c       | 60 +++++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/test/test/test_power_acpi_cpufreq.c b/test/test/test_power_acpi_cpufreq.c
index 3bfd033..8da2dcc 100644
--- a/test/test/test_power_acpi_cpufreq.c
+++ b/test/test/test_power_acpi_cpufreq.c
@@ -27,7 +27,7 @@
 #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
 
 #define TEST_POWER_SYSFILE_CUR_FREQ \
-	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_cur_freq"
+	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"
 
 static uint32_t total_freq_num;
 static uint32_t freqs[TEST_POWER_FREQS_NUM_MAX];
diff --git a/test/test/test_power_kvm_vm.c b/test/test/test_power_kvm_vm.c
index 91b31c4..2ac7491 100644
--- a/test/test/test_power_kvm_vm.c
+++ b/test/test/test_power_kvm_vm.c
@@ -25,12 +25,17 @@
 #define TEST_POWER_VM_LCORE_ID            0U
 #define TEST_POWER_VM_LCORE_OUT_OF_BOUNDS (RTE_MAX_LCORE+1)
 #define TEST_POWER_VM_LCORE_INVALID       1U
+#define TEMP_POWER_MANAGER_FILE_PATH  "/tmp/testpm"
+
+int guest_channel_host_connect(const char *path, unsigned int lcore_id);
 
 static int
 test_power_kvm_vm(void)
 {
 	int ret;
 	enum power_management_env env;
+	char fPath[PATH_MAX];
+	FILE *fPtr = NULL;
 
 	ret = rte_power_set_env(PM_ENV_KVM_VM);
 	if (ret != 0) {
@@ -95,12 +100,31 @@
 	/* Test initialisation of a valid lcore */
 	ret = rte_power_init(TEST_POWER_VM_LCORE_ID);
 	if (ret < 0) {
-		printf("Cannot initialise power management for lcore %u, this "
-				"may occur if environment is not configured "
-				"correctly(KVM VM) or operating in another valid "
-				"Power management environment\n", TEST_POWER_VM_LCORE_ID);
-		rte_power_unset_env();
-		return -1;
+		printf("rte_power_init failed as expected in host\n");
+		/* This test would be successful when run on VM,
+		 * in order to run in Host itself, temporary file path
+		 * is created and same is used for further communication
+		 */
+
+		snprintf(fPath, PATH_MAX, "%s.%u",
+			TEMP_POWER_MANAGER_FILE_PATH, TEST_POWER_VM_LCORE_ID);
+		fPtr = fopen(fPath, "w");
+		if (fPtr == NULL) {
+			printf(" Unable to create file\n");
+			rte_power_unset_env();
+			return -1;
+		}
+		ret = guest_channel_host_connect(TEMP_POWER_MANAGER_FILE_PATH,
+			TEST_POWER_VM_LCORE_ID);
+		if (ret == 0)
+			printf("guest_channel_host_connect successful\n");
+		else {
+			printf("guest_channel_host_connect failed\n");
+			rte_power_unset_env();
+			fclose(fPtr);
+			remove(fPath);
+			return -1;
+		}
 	}
 
 	/* Test initialisation of previously initialised lcore */
@@ -175,6 +199,22 @@
 		goto fail_all;
 	}
 
+	/* Test KVM_VM Enable Turbo of valid core */
+	ret = rte_power_freq_enable_turbo(TEST_POWER_VM_LCORE_ID);
+	if (ret == -1) {
+		printf("rte_power_freq_enable_turbo failed on valid lcore"
+			"%u\n", TEST_POWER_VM_LCORE_ID);
+		goto fail_all;
+	}
+
+	/* Test KVM_VM Disable Turbo of valid core */
+	ret = rte_power_freq_disable_turbo(TEST_POWER_VM_LCORE_ID);
+	if (ret == -1) {
+		printf("rte_power_freq_disable_turbo failed on valid lcore"
+		"%u\n", TEST_POWER_VM_LCORE_ID);
+		goto fail_all;
+	}
+
 	/* Test frequency up of valid lcore */
 	ret = rte_power_freq_up(TEST_POWER_VM_LCORE_ID);
 	if (ret != 1) {
@@ -274,10 +314,18 @@
 		return -1;
 	}
 	rte_power_unset_env();
+	if (fPtr != NULL) {
+		fclose(fPtr);
+		remove(fPath);
+	}
 	return 0;
 fail_all:
 	rte_power_exit(TEST_POWER_VM_LCORE_ID);
 	rte_power_unset_env();
+	if (fPtr != NULL) {
+		fclose(fPtr);
+		remove(fPath);
+	}
 	return -1;
 }
 #endif
-- 
1.7.12.2

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Thomas Monjalon]

11/04/2018 16:14, Reshma Pattan:
> Unit Testcases are added for power_acpi_cpu_freq,
> power_kvm_vm_test to improve coverage
> 
> Signed-off-by: Jananee Parthasarathy <jananeex.m.parthasarathy@intel.com>
> Acked-by: David Hunt <david.hunt@intel.com>

Applied, thanks

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Bruce Richardson]

On Mon, Apr 23, 2018 at 11:04:27PM +0200, Thomas Monjalon wrote:
> 11/04/2018 16:14, Reshma Pattan:
> > Unit Testcases are added for power_acpi_cpu_freq,
> > power_kvm_vm_test to improve coverage
> > 
> > Signed-off-by: Jananee Parthasarathy <jananeex.m.parthasarathy@intel.com>
> > Acked-by: David Hunt <david.hunt@intel.com>
> 
> Applied, thanks
> 
Sadly, this patch seems to break shared library builds. If you try doing
"make test-build" with shared libraries on it will fail, or if you do a
meson build using shared libraries you will get the same result.

The root cause is that the function guest_channel_host_connect() is a
private function and so is not listed in the shared library map file,
preventing the test app from linking.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, April 24, 2018 11:59 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Parthasarathy, JananeeX M <jananeex.m.parthasarathy@intel.com>;
> dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Hunt, David
> <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit
> tests
> 
> On Mon, Apr 23, 2018 at 11:04:27PM +0200, Thomas Monjalon wrote:
> > 11/04/2018 16:14, Reshma Pattan:
> > > Unit Testcases are added for power_acpi_cpu_freq, power_kvm_vm_test
> > > to improve coverage
> > >
> > > Signed-off-by: Jananee Parthasarathy
> > > <jananeex.m.parthasarathy@intel.com>
> > > Acked-by: David Hunt <david.hunt@intel.com>
> >
> > Applied, thanks
> >
> Sadly, this patch seems to break shared library builds. If you try doing "make
> test-build" with shared libraries on it will fail, or if you do a meson build using
> shared libraries you will get the same result.
> 
> The root cause is that the function guest_channel_host_connect() is a private
> function and so is not listed in the shared library map file, preventing the test
> app from linking.
> 

Any action from my side required? Let me know.

> Regards,
> /Bruce

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Hunt, David]

On 24/4/2018 12:23 PM, Pattan, Reshma wrote:
> Hi,
>
>> -----Original Message-----
>> From: Richardson, Bruce
>> Sent: Tuesday, April 24, 2018 11:59 AM
>> To: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Parthasarathy, JananeeX M <jananeex.m.parthasarathy@intel.com>;
>> dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Hunt, David
>> <david.hunt@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit
>> tests
>>
>> On Mon, Apr 23, 2018 at 11:04:27PM +0200, Thomas Monjalon wrote:
>>> 11/04/2018 16:14, Reshma Pattan:
>>>> Unit Testcases are added for power_acpi_cpu_freq, power_kvm_vm_test
>>>> to improve coverage
>>>>
>>>> Signed-off-by: Jananee Parthasarathy
>>>> <jananeex.m.parthasarathy@intel.com>
>>>> Acked-by: David Hunt <david.hunt@intel.com>
>>> Applied, thanks
>>>
>> Sadly, this patch seems to break shared library builds. If you try doing "make
>> test-build" with shared libraries on it will fail, or if you do a meson build using
>> shared libraries you will get the same result.
>>
>> The root cause is that the function guest_channel_host_connect() is a private
>> function and so is not listed in the shared library map file, preventing the test
>> app from linking.
>>
> Any action from my side required? Let me know.

Reshma,
     Looking at this, I think this particular unit test needs to be 
removed. The way it is at the moment, it's "faking" the connect, then 
any commands that are sent to the dummy host are only really to test to 
see if the API breaks, which is going to be captured by compilation 
tests anyway. I don't see the value of this unit test unless you have 
the full host setup underneath is, in which case it's no longer a unit 
test.
Also, we don't want to make these functions public, as they are only of 
use to the library internally, and there is no use for them publicly 
(unless a guest wants to fake a connection to a non-existent host).

What do you think?
Dave.

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Hunt, David
> Sent: Tuesday, April 24, 2018 1:10 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: Parthasarathy, JananeeX M <jananeex.m.parthasarathy@intel.com>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit
> tests
> 
> 
> On 24/4/2018 12:23 PM, Pattan, Reshma wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Richardson, Bruce
> >> Sent: Tuesday, April 24, 2018 11:59 AM
> >> To: Thomas Monjalon <thomas@monjalon.net>
> >> Cc: Parthasarathy, JananeeX M <jananeex.m.parthasarathy@intel.com>;
> >> dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Hunt, David
> >> <david.hunt@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager
> >> unit tests
> >>
> >> On Mon, Apr 23, 2018 at 11:04:27PM +0200, Thomas Monjalon wrote:
> >>> 11/04/2018 16:14, Reshma Pattan:
> >>>> Unit Testcases are added for power_acpi_cpu_freq,
> power_kvm_vm_test
> >>>> to improve coverage
> >>>>
> >>>> Signed-off-by: Jananee Parthasarathy
> >>>> <jananeex.m.parthasarathy@intel.com>
> >>>> Acked-by: David Hunt <david.hunt@intel.com>
> >>> Applied, thanks
> >>>
> >> Sadly, this patch seems to break shared library builds. If you try
> >> doing "make test-build" with shared libraries on it will fail, or if
> >> you do a meson build using shared libraries you will get the same result.
> >>
> >> The root cause is that the function guest_channel_host_connect() is a
> >> private function and so is not listed in the shared library map file,
> >> preventing the test app from linking.
> >>
> > Any action from my side required? Let me know.
> 
> Reshma,
>      Looking at this, I think this particular unit test needs to be removed. The
> way it is at the moment, it's "faking" the connect, then any commands that
> are sent to the dummy host are only really to test to see if the API breaks,
> which is going to be captured by compilation tests anyway. I don't see the
> value of this unit test unless you have the full host setup underneath is, in
> which case it's no longer a unit test.
> Also, we don't want to make these functions public, as they are only of use to
> the library internally, and there is no use for them publicly (unless a guest
> wants to fake a connection to a non-existent host).
> 
> What do you think?

Fine, we are reverting the changes and will send the patch soon.

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Thomas Monjalon]

24/04/2018 14:51, Pattan, Reshma:
> From: Hunt, David
> > On 24/4/2018 12:23 PM, Pattan, Reshma wrote:
> > > From: Richardson, Bruce
> > >> On Mon, Apr 23, 2018 at 11:04:27PM +0200, Thomas Monjalon wrote:
> > >>> 11/04/2018 16:14, Reshma Pattan:
> > >>>> Unit Testcases are added for power_acpi_cpu_freq,
> > power_kvm_vm_test
> > >>>> to improve coverage
> > >>>>
> > >>>> Signed-off-by: Jananee Parthasarathy
> > >>>> <jananeex.m.parthasarathy@intel.com>
> > >>>> Acked-by: David Hunt <david.hunt@intel.com>
> > >>> Applied, thanks
> > >>>
> > >> Sadly, this patch seems to break shared library builds. If you try
> > >> doing "make test-build" with shared libraries on it will fail, or if
> > >> you do a meson build using shared libraries you will get the same result.
> > >>
> > >> The root cause is that the function guest_channel_host_connect() is a
> > >> private function and so is not listed in the shared library map file,
> > >> preventing the test app from linking.
> > >>
> > > Any action from my side required? Let me know.
> > 
> > Reshma,
> >      Looking at this, I think this particular unit test needs to be removed. The
> > way it is at the moment, it's "faking" the connect, then any commands that
> > are sent to the dummy host are only really to test to see if the API breaks,
> > which is going to be captured by compilation tests anyway. I don't see the
> > value of this unit test unless you have the full host setup underneath is, in
> > which case it's no longer a unit test.
> > Also, we don't want to make these functions public, as they are only of use to
> > the library internally, and there is no use for them publicly (unless a guest
> > wants to fake a connection to a non-existent host).
> > 
> > What do you think?
> 
> Fine, we are reverting the changes and will send the patch soon.

Where is the patch?
I will revert it myself.

Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit tests

[Pattan, Reshma]

Hi Thomas,

I sent it few mins back. Can you check  and apply

Thanks,
Reshma

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, April 24, 2018 11:34 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: dev@dpdk.org; Hunt, David <david.hunt@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Parthasarathy, JananeeX M
> <jananeex.m.parthasarathy@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] app/test: enhance power manager unit
> tests
> 
> 24/04/2018 14:51, Pattan, Reshma:
> > From: Hunt, David
> > > On 24/4/2018 12:23 PM, Pattan, Reshma wrote:
> > > > From: Richardson, Bruce
> > > >> On Mon, Apr 23, 2018 at 11:04:27PM +0200, Thomas Monjalon wrote:
> > > >>> 11/04/2018 16:14, Reshma Pattan:
> > > >>>> Unit Testcases are added for power_acpi_cpu_freq,
> > > power_kvm_vm_test
> > > >>>> to improve coverage
> > > >>>>
> > > >>>> Signed-off-by: Jananee Parthasarathy
> > > >>>> <jananeex.m.parthasarathy@intel.com>
> > > >>>> Acked-by: David Hunt <david.hunt@intel.com>
> > > >>> Applied, thanks
> > > >>>
> > > >> Sadly, this patch seems to break shared library builds. If you
> > > >> try doing "make test-build" with shared libraries on it will
> > > >> fail, or if you do a meson build using shared libraries you will get the
> same result.
> > > >>
> > > >> The root cause is that the function guest_channel_host_connect()
> > > >> is a private function and so is not listed in the shared library
> > > >> map file, preventing the test app from linking.
> > > >>
> > > > Any action from my side required? Let me know.
> > >
> > > Reshma,
> > >      Looking at this, I think this particular unit test needs to be
> > > removed. The way it is at the moment, it's "faking" the connect,
> > > then any commands that are sent to the dummy host are only really to
> > > test to see if the API breaks, which is going to be captured by
> > > compilation tests anyway. I don't see the value of this unit test
> > > unless you have the full host setup underneath is, in which case it's no
> longer a unit test.
> > > Also, we don't want to make these functions public, as they are only
> > > of use to the library internally, and there is no use for them
> > > publicly (unless a guest wants to fake a connection to a non-existent host).
> > >
> > > What do you think?
> >
> > Fine, we are reverting the changes and will send the patch soon.
> 
> Where is the patch?
> I will revert it myself.
>