Re: [PATCH 0/2] use helper when using abstract QOM parent functions
Hi, On 10/14/19 5:12 PM, Auger Eric wrote: Hi, On 10/12/19 11:43 AM, Mao Zhongyi wrote: Philippe introduced a series of helpers to make the device class_init() easier to understand when a device class change the parent hooks, some devices in the source tree missed helper, so convert it. Cc: eric.au...@redhat.com Cc: peter.mayd...@linaro.org Cc: hpous...@reactos.org Cc: f4...@amsat.org Mao Zhongyi (2): arm/smmuv3: use helpers to be more easier to understand when using abstract QOM parent functions. isa/pc87312: use helpers to be more easier to understand when using abstract QOM parent functions. hw/arm/smmuv3.c | 3 +-- hw/isa/pc87312.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) for the series: Reviewed-by: Eric Auger ping... Eric
Re: [PATCH 9/9] migration/ram: calculate un/encoded_size only whenneeded.
On 6/12/20 3:05 AM, Dr. David Alan Gilbert wrote:
* Mao Zhongyi (maozhon...@cmss.chinamobile.com) wrote:
Signed-off-by: Mao Zhongyi
---
migration/ram.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 41cc530d9d..ca20030b64 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -910,14 +910,15 @@ static void migration_update_rates(RAMState *rs, int64_t
end_time)
xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss
-
rs->xbzrle_cache_miss_prev) / page_count;
rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
-encoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
- TARGET_PAGE_SIZE;
-encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
xbzrle_counters.encoding_rate = 0;
-} else if (!encoded_size) {
+} else if (xbzrle_counters.bytes == rs->xbzrle_bytes_prev) {
No, I don't think this change is worth it - this is really just the same
as 'encoded_size', and then we may as well keep the two together.
ok, thanks, let's keep 'encode_size' here.
BTW, this change borrows from the behavior of comppressed:
...
compressed_size = compression_counters.compressed_size -
rs->compressed_size_prev;
if (compressed_size) {
double uncompressed_size = (compression_counters.pages -
rs->compress_pages_prev) *
TARGET_PAGE_SIZE;
/* Compression-Ratio = Uncompressed-size / Compressed-size */
compression_counters.compression_rate =
uncompressed_size /
compressed_size;
...
It splits 'compressed_size' and 'uncompressed_size', and calculates
'uncompressed_size' only when needed. Although 'unencoded_size' is
calculated, it is not necessarily used. if you think this split is
unnecessary, just discard it, so do I need to drop this patch and
resend the v2?
Thanks,
Mao
Dave
xbzrle_counters.encoding_rate = UINT64_MAX;
} else {
+unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
+ TARGET_PAGE_SIZE;
+encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
+
xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
}
rs->xbzrle_pages_prev = xbzrle_counters.pages;
--
2.17.1
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
Hi Markus,
On 3/30/20 3:18 PM, Markus Armbruster wrote:
"Dr. David Alan Gilbert" writes:
* Mao Zhongyi (maozhon...@cmss.chinamobile.com) wrote:
run:
(qemu) info migrate_parameters
announce-initial: 50 ms
...
announce-max: 550 ms
multifd-compression: none
xbzrle-cache-size: 4194304
max-postcopy-bandwidth: 0
tls-authz: '(null)'
Migration parameter 'tls-authz' is used to provide the QOM ID
of a QAuthZ subclass instance that provides the access control
check, default is NULL. But the empty string is not a valid
object ID, so use "" instead of the default. Although it will
fail when lookup an object with ID "", it is harmless, just
consistent with tls_creds.
Yes, it's probably the best we can do given Dan's explanation that
we can't change tls_authz to be non-null.
As I explained in Message-ID: <878sjv11xm@dusky.pond.sub.org>, this
is actually a crash bug on some systems. The commit message neglects to
mention that. Too late to fix now. Next time :)
Oops, I will continue to follow up on this issue.
Also fixed the bad indentation on the last line.
Signed-off-by: Mao Zhongyi
---
migration/migration.c | 3 ++-
monitor/hmp-cmds.c| 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index c1d88ace7f..b060153ef7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error
**errp)
params->has_tls_hostname = true;
params->tls_hostname = g_strdup(s->parameters.tls_hostname);
params->has_tls_authz = true;
-params->tls_authz = g_strdup(s->parameters.tls_authz);
+params->tls_authz = s->parameters.tls_authz ? \
+g_strdup(s->parameters.tls_authz) : g_strdup("");
The \ is unneeded; this isn't a macro; it's also a little shorter to do
it as:
params->tls_authz = g_strdup(s->parameters.tls_authz ?
s->parameters.tls_authz : "");
Even shorter:
params->tls_authz = g_strdup(s->parameters.tls_authz ?: "");
?: is a GNU C extension. We use it all over the place.
Just FYI. I'm *not* asking for the code to be changed.
Yes, it does look cooler, thanks for the clarification. I would prefer
to use this syntax in future patches.
Thanks,
Mao
Re: [PATCH v2] monitor/hmp-cmds: add units for migrate_parameters.
On 3/28/20 2:02 AM, Dr. David Alan Gilbert wrote: * Mao Zhongyi (maozhon...@cmss.chinamobile.com) wrote: When running: (qemu) info migrate_parameters announce-initial: 50 ms announce-max: 550 ms announce-step: 100 ms compress-wait-thread: on ... max-bandwidth: 33554432 bytes/second downtime-limit: 300 milliseconds x-checkpoint-delay: 2 ... xbzrle-cache-size: 67108864 add units for the parameters 'x-checkpoint-delay' and 'xbzrle-cache-size', it's easier to read, also move milliseconds to ms to keep the same style. Signed-off-by: Mao Zhongyi Thanks Reviewed-by: Dr. David Alan Gilbert (info migrate could also be fixed, but that's a separate issue) Yes, will fix it in a separated patch. Thanks, Mao --- monitor/hmp-cmds.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 2a900a528a..790fad3afe 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -436,11 +436,11 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH), params->max_bandwidth); assert(params->has_downtime_limit); -monitor_printf(mon, "%s: %" PRIu64 " milliseconds\n", +monitor_printf(mon, "%s: %" PRIu64 " ms\n", MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT), params->downtime_limit); assert(params->has_x_checkpoint_delay); -monitor_printf(mon, "%s: %u\n", +monitor_printf(mon, "%s: %u ms\n", MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY), params->x_checkpoint_delay); assert(params->has_block_incremental); @@ -453,7 +453,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%s: %s\n", MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION), MultiFDCompression_str(params->multifd_compression)); -monitor_printf(mon, "%s: %" PRIu64 "\n", +monitor_printf(mon, "%s: %" PRIu64 " bytes\n", MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), params->xbzrle_cache_size); monitor_printf(mon, "%s: %" PRIu64 "\n", -- 2.17.1 -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] migration: fix bad indentation in error_report()
On 3/27/20 7:41 PM, Dr. David Alan Gilbert wrote:
* Mao Zhongyi (maozhon...@cmss.chinamobile.com) wrote:
bad indentation conflicts with CODING_STYLE doc.
Signed-off-by: Mao Zhongyi
---
migration/migration.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index c4c9aee15e..aa43137bd2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1203,15 +1203,15 @@ static bool migrate_params_check(MigrationParameters
*params, Error **errp)
if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) {
error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
- " range of 0 to %zu bytes/second", SIZE_MAX);
+ " range of 0 to %zu bytes/second", SIZE_MAX);
No, where a parameter crosses multiple lines, it is clearer to line up
the continuation of the parameter with the parameter above.
Well, indeed. I got it, Thanks.
Mao
return false;
}
if (params->has_downtime_limit &&
(params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
- "the range of 0 to %d milliseconds",
- MAX_MIGRATE_DOWNTIME);
+ "the range of 0 to %d milliseconds",
+ MAX_MIGRATE_DOWNTIME);
same as above.
return false;
}
@@ -2109,8 +2109,8 @@ void qmp_migrate_set_downtime(double value, Error **errp)
{
if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) {
error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
- "the range of 0 to %d seconds",
- MAX_MIGRATE_DOWNTIME_SECONDS);
+ "the range of 0 to %d seconds",
+ MAX_MIGRATE_DOWNTIME_SECONDS);
same as above.
return;
}
@@ -2495,7 +2495,7 @@ retry:
if (header_type >= MIG_RP_MSG_MAX ||
header_type == MIG_RP_MSG_INVALID) {
error_report("RP: Received invalid message 0x%04x length 0x%04x",
-header_type, header_len);
+ header_type, header_len);
OK, yes that's better.
mark_source_rp_bad(ms);
goto out;
}
@@ -2504,9 +2504,9 @@ retry:
header_len != rp_cmd_args[header_type].len) ||
header_len > sizeof(buf)) {
error_report("RP: Received '%s' message (0x%04x) with"
-"incorrect length %d expecting %zu",
-rp_cmd_args[header_type].name, header_type, header_len,
-(size_t)rp_cmd_args[header_type].len);
+ "incorrect length %d expecting %zu",
+ rp_cmd_args[header_type].name, header_type,
header_len,
+ (size_t)rp_cmd_args[header_type].len);
OK.
mark_source_rp_bad(ms);
goto out;
}
@@ -2561,7 +2561,7 @@ retry:
}
if (header_len != expected_len) {
error_report("RP: Req_Page_id with length %d expecting %zd",
-header_len, expected_len);
+ header_len, expected_len);
OK.
Dave
mark_source_rp_bad(ms);
goto out;
}
--
2.17.1
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] monitor/hmp-cmds: add units for mirate_parameters.
On 3/27/20 9:21 PM, Dr. David Alan Gilbert wrote: * Stefano Garzarella (sgarz...@redhat.com) wrote: On Fri, Mar 27, 2020 at 11:28:14AM +, Dr. David Alan Gilbert wrote: * Stefano Garzarella (sgarz...@redhat.com) wrote: Hi Mao, On Fri, Mar 27, 2020 at 03:32:10PM +0800, Mao Zhongyi wrote: When running: (qemu) info migrate_parameters announce-initial: 50 ms announce-max: 550 ms announce-step: 100 ms compress-wait-thread: on ... max-bandwidth: 33554432 bytes/second downtime-limit: 300 milliseconds x-checkpoint-delay: 2 ... xbzrle-cache-size: 67108864 add units for the parameters 'x-checkpoint-delay' and 'xbzrle-cache-size', it's easier to read. Signed-off-by: Mao Zhongyi --- monitor/hmp-cmds.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 2a900a528a..8d22f96e57 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -440,7 +440,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT), params->downtime_limit); assert(params->has_x_checkpoint_delay); -monitor_printf(mon, "%s: %u\n", +monitor_printf(mon, "%s: %u" " milliseconds\n", ^ here we can remove the space and use a single string "%s: %u milliseconds\n" Yes. I've noticed that we use both ms or milliseconds, if you want to clean up in a separate patch, maybe we could use one of these everywhere. (I vote for 'ms') I do prefer 'ms', however we do seem to just use milliseconds in info migrate IIUC, currently with 'info migrate_parameters' we have: - announce-initial, announce-max, and announce-step with 'ms' - downtime-limit with 'milliseconds' You're right, so we do - in that case I agree, lets just move them all to 'ms'. thanks for your explanation, I got it, will fix it. Thanks Mao Dave Stefano -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output
On 3/21/20 3:14 PM, Markus Armbruster wrote:
"Dr. David Alan Gilbert" writes:
* Daniel P. Berrangé (berra...@redhat.com) wrote:
On Fri, Mar 20, 2020 at 05:31:17PM +, Dr. David Alan Gilbert wrote:
(Rearranging the text a bit)
* Markus Armbruster (arm...@redhat.com) wrote:
David (cc'ed) should be able to tell us which fix is right.
@tls_creds and @tls_hostname look like they could have the same issue.
A certain Markus removed the Null checks in 8cc99dc because 4af245d
guaranteed they would be None-Null for tls-creds/hostname - so we
should be OK for those.
But tls-authz came along a lot later in d2f1d29 and doesn't
seem to have the initialisation, which is now in
migration_instance_init.
So I *think* the fix for this is to do the modern equivalent of 4af245d
:
diff --git a/migration/migration.c b/migration/migration.c
index c1d88ace7f..0bc1b93277 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3686,6 +3686,7 @@ static void migration_instance_init(Object *obj)
params->tls_hostname = g_strdup("");
params->tls_creds = g_strdup("");
+params->tls_authz = g_strdup("");
/* Set has_* up only for parameter checks */
params->has_compress_level = true;
Copying in Dan to check that wouldn't break tls.
It *will* break TLS, because it will cause the TLS code to lookup
an object with the ID of "". NULL must be preserved when calling
the TLS APIs.
OK, good I asked...
The assignment of "" to tls_hostname would also have broken TLS,
so the migration_tls_channel_connect method had to turn it back
into a real NULL.
The use of "" for tls_creds will similarly cause it to try and
lookup an object with ID of "", and fail. That one's harmless
though, because it would also fail if it were NULL.
OK.
It looks like the output of query-migrate-parameters though already
turns it into "", so I don't think you can tell it's NULL from that:
{"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": "qemu-4.2.0-4.fc31"},
"capabilities": ["oob"]}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "query-migrate-parameters" }
{"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, "announce-max": 550, "decompress-threads": 2, "compress-threads": 8, "compress-level": 1, "multifd-channels": 2, "announce-initial": 50, "block-incremental":
false, "compress-wait-thread": true, "downtime-limit": 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, "tls-hostname":
"", "max-bandwidth": 33554432, "x-checkpoint-delay": 2, "cpu-throttle-increment": 10}}
I'm not sure who turns a Null into a "" but I guess it's somewhere in
the json output iterator.
It's this wart:
static void qobject_output_type_str(Visitor *v, const char *name, char
**obj,
Error **errp)
{
QObjectOutputVisitor *qov = to_qov(v);
if (*obj) {
qobject_output_add(qov, name, qstring_from_str(*obj));
} else {
qobject_output_add(qov, name, qstring_from_str(""));
}
}
Warty since day 1.
In theory, !*obj should not happen, since QAPI type 'str' is not
nullable.
In practice, it handwritten code can easily make it happen.
So we can fix this problem either in qmp_query_migrate_parameters
and just strdup a "", or substitute it in hmp_info_migrate_parameters.
Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
'str', and bypasses the wart.
OK, thanks for the kind reply, will fix it in v2.
Thanks,
Mao
Re: [Qemu-devel] [PATCH] pci_bridge: fix a typo in comment
ping... On 11/8/18 9:12 PM, Philippe Mathieu-Daudé wrote: Cc'ing qemu-trivial@ On 8/11/18 13:21, Mao Zhongyi wrote: Signed-off-by: Mao Zhongyi Reviewed-by: Philippe Mathieu-Daudé --- hw/pci/pci_bridge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index ee9dff2d3a..da8daa3ff2 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -310,7 +310,7 @@ void pci_bridge_reset(DeviceState *qdev) /* * the default values for base/limit registers aren't specified - * in the PCI-to-PCI-bridge spec. So we don't thouch them here. + * in the PCI-to-PCI-bridge spec. So we don't touch them here. * Each implementation can override it. * typical implementation does * zero base/limit registers or
Re: [PATCH v4 0/3] some fix in tests/migration
Hi, patch2 has been merged into the master by Laurent Vivier. patch3 is still not reviewed. So ping... Thanks, Mao On 10/5/19 1:32 AM, Mao Zhongyi wrote: This patchset mainly fixes memory leak, typo and return value of stress function in stress test. v4-v3: p1: - remove redundant g_malloc return value judgment. [Laurent Vivier] p3: - always use exit_failure() as the exit function of main(). [Laurent Vivier] - update the commit message. v3->v2: p1: - replace malloc with g_malloc [Laurent Vivier] p3: - change stressone type to void and stree return value to -1 to make the path of 'if (stress(ramsizeGB, ncpus) < 0)' can be reached.[Laurent Vivier] - update the commit message. v2->v1: - use g_autofree to release memory automatically instead of free(). [Alex Bennée] Cc: arm...@redhat.com Cc: laur...@vivier.eu Cc: tony.ngu...@bt.com Cc: alex.ben...@linaro.org Mao Zhongyi (3): tests/migration: mem leak fix tests/migration: fix a typo in comment tests/migration：fix unreachable path in stress test tests/migration/stress.c | 36 1 file changed, 8 insertions(+), 28 deletions(-)
Re: [PATCH v1 5/5] contrib/gitdm: add China Mobile to the domain map
On 10/14/19 9:59 PM, Alex Bennée wrote: We've had a number of contributions from this domain. I think they are from the company rather than customers using the email address but it's hard for me to tell. Please confirm. Acked-by: Mao Zhongyi Signed-off-by: Alex Bennée Cc: Mao Zhongyi Cc: Zhang Shengju --- contrib/gitdm/domain-map | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/gitdm/domain-map b/contrib/gitdm/domain-map index 304e91010a..7fc7fda68f 100644 --- a/contrib/gitdm/domain-map +++ b/contrib/gitdm/domain-map @@ -5,6 +5,7 @@ # amd.com AMD +cmss.chinamobile.com China Mobile citrix.com Citrix greensocs.com GreenSocs fujitsu.com Fujitsu
Re: [PATCH v1 5/5] contrib/gitdm: add China Mobile to the domain map
Hi, Alex On 10/14/19 9:59 PM, Alex Bennée wrote: We've had a number of contributions from this domain. I think they are from the company rather than customers using the email address but it's hard for me to tell. Please confirm. Yes, this domain comes from a china company, not an individual, and I'm very grateful for this patch. Currently we can only view the community's patch via the GNU mailing list, Does this patch mean that we can now easily subscribe to the mailing list form the mail client? Many thanks, Mao Signed-off-by: Alex Bennée Cc: Mao Zhongyi Cc: Zhang Shengju --- contrib/gitdm/domain-map | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/gitdm/domain-map b/contrib/gitdm/domain-map index 304e91010a..7fc7fda68f 100644 --- a/contrib/gitdm/domain-map +++ b/contrib/gitdm/domain-map @@ -5,6 +5,7 @@ # amd.com AMD +cmss.chinamobile.com China Mobile citrix.com Citrix greensocs.com GreenSocs fujitsu.com Fujitsu
Re: _[PATCH_v2_3/3]_tests/migration：fix_unreachable_path_in_stress_test
On 10/3/19 5:23 PM, Laurent Vivier wrote:
Le 03/10/2019 à 09:17, maozy a écrit :
Hi, Laurent
On 10/1/19 11:46 PM, Laurent Vivier wrote:
Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
if stress function always return 0, the path
'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
so fix it to allow the test failed.
Cc: arm...@redhat.com
Cc: laur...@vivier.eu
Cc: tony.ngu...@bt.com
Signed-off-by: Mao Zhongyi
---
tests/migration/stress.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index 19a6eff5fd..35903d90c4 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
}
}
}
+ return 0;
}
before the return, we have an infinite loop "while(1) { }".
So this part is dead code.
In fact, if the function exits, it's because it fails, otherwise it
loops infinitely, so I think we should change its type to void and
stress should always return -1.
Yes, I think it's ok to change stressone typo to void because
no one cares about its return value, but if make stress always
return -1, main will always exited in exit_failure, like this:
...
if (stress(ramsizeGB, ncpus) < 0)
exit_failure();
exit_success();
}
so, perhaps also change stress typo to void may be good. then:
...
stress(ramsizeGB, ncpus);
exit_success();
}
Anther way , make stressone return 0 when infinite loop fails to
exit, then main can handle both success and failure case.
what do you think?
If stressone() or stress() exits it's because of a failure because the
test runs forever otherwise.
So I think there is no problem to use exit_failure() as the exit
function of main(): it should never be reached if the test runs without
error.
OK, I see, thanks a lot.
Thanks,
Laurent
Re: _[PATCH_v2_3/3]_tests/migration：fix_unreachable_path_in_stress_test
Hi, Laurent
On 10/1/19 11:46 PM, Laurent Vivier wrote:
Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
if stress function always return 0, the path
'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
so fix it to allow the test failed.
Cc: arm...@redhat.com
Cc: laur...@vivier.eu
Cc: tony.ngu...@bt.com
Signed-off-by: Mao Zhongyi
---
tests/migration/stress.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index 19a6eff5fd..35903d90c4 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
}
}
}
+return 0;
}
before the return, we have an infinite loop "while(1) { }".
So this part is dead code.
In fact, if the function exits, it's because it fails, otherwise it
loops infinitely, so I think we should change its type to void and
stress should always return -1.
Yes, I think it's ok to change stressone typo to void because
no one cares about its return value, but if make stress always
return -1, main will always exited in exit_failure, like this:
...
if (stress(ramsizeGB, ncpus) < 0)
exit_failure();
exit_success();
}
so, perhaps also change stress typo to void may be good. then:
...
stress(ramsizeGB, ncpus);
exit_success();
}
Anther way , make stressone return 0 when infinite loop fails to
exit, then main can handle both success and failure case.
what do you think?
Thanks,
Mao
Thanks,
Laurent
Re: [PATCH v2 1/3] tests/migration: mem leak fix
On 10/1/19 11:31 PM, Laurent Vivier wrote:
Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
‘data’ has the possibility of memory leaks， so use the
glic macros g_autofree recommended by CODING_STYLE.rst
to automatically release the memory that returned from
g_malloc().
Cc: arm...@redhat.com
Cc: laur...@vivier.eu
Cc: tony.ngu...@bt.com
Cc: alex.ben...@linaro.org
Signed-off-by: Mao Zhongyi
---
tests/migration/stress.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index d9aa4afe92..6cbb2d49d3 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -170,10 +170,10 @@ static unsigned long long now(void)
static int stressone(unsigned long long ramsizeMB)
{
size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
-char *ram = malloc(ramsizeMB * 1024 * 1024);
+g_autofree char *ram = malloc(ramsizeMB * 1024 * 1024);
char *ramptr;
size_t i, j, k;
-char *data = malloc(PAGE_SIZE);
+g_autofree char *data = malloc(PAGE_SIZE);
So perhaps g_malloc() could be a better choice as it will exit on
allocation failure?
right, will fix it in next.
Thanks,
Mao
Thanks,
Laurent
Re: [PATCH v2 3/3] tests/migration：fix unreachable path in stress test
ping... On 9/11/19 11:31 AM, Mao Zhongyi wrote: if stress function always return 0, the path 'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable, so fix it to allow the test failed. Cc: arm...@redhat.com Cc: laur...@vivier.eu Cc: tony.ngu...@bt.com Signed-off-by: Mao Zhongyi --- tests/migration/stress.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/migration/stress.c b/tests/migration/stress.c index 19a6eff5fd..35903d90c4 100644 --- a/tests/migration/stress.c +++ b/tests/migration/stress.c @@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB) } } } +return 0; } @@ -248,9 +249,7 @@ static int stress(unsigned long long ramsizeGB, int ncpus) stressthread, ); } -stressone(ramsizeMB); - -return 0; +return stressone(ramsizeMB); }
Re: [Qemu-devel] [PATCH 1/3] tests/migration: mem leak fix
On 9/11/19 2:52 AM, Alex Bennée wrote:
Mao Zhongyi writes:
Cc: arm...@redhat.com
Cc: laur...@vivier.eu
Cc: tony.ngu...@bt.com
Signed-off-by: Mao Zhongyi
---
tests/migration/stress.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index d9aa4afe92..e6c9a6b243 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -181,6 +181,8 @@ static int stressone(unsigned long long ramsizeMB)
if (!ram) {
fprintf(stderr, "%s (%05d): ERROR: cannot allocate %llu MB of RAM:
%s\n",
argv0, gettid(), ramsizeMB, strerror(errno));
+if (data)
+free(data);
I wonder if it's worth using the glib macros here so:
g_autofree char *data = g_malloc(PAGE_SIZE);
and the same for ram. You can then drop the frees.
I thins it's ok, which is also recommended in CODING_STYLE.rst.
Thx
Mao
return -1;
}
if (!data) {
--
Alex Bennée
Re: [Qemu-devel] [PATCH] riscv/cpu: use device_class_set_parent_realize
Hi, Palmer On 11/28/18 8:34 AM, Palmer Dabbelt wrote: On Mon, 26 Nov 2018 01:06:33 PST (-0800), Bastian Koppelmann wrote: On 11/26/18 4:20 AM, Mao Zhongyi wrote: Signed-off-by: Mao Zhongyi --- target/riscv/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Bastian Koppelmann Shouldn't we also use device_class_set_parent_reset right below this? device_class_set_parent_reset only used for DeviceClass *dc->reset, here is CPUClass *cc->reset. Thanks, Mao Either way, I'll queue this one for 3.2. Thanks!
Re: [Qemu-devel] [PATCH v2 15/21] pci-bridge/dec: Convert sysbus initfunction to realize function
Hi, Philippe
On 11/24/18 12:37 AM, Philippe Mathieu-Daudé wrote:
Hi Mao,
On 23/11/18 16:30, Mao Zhongyi wrote:
Use DeviceClass rather than SysBusDeviceClass in
pci_dec_21154_device_class_init().
Cc: da...@gibson.dropbear.id.au
Cc: m...@redhat.com
Cc: marcel.apfelb...@gmail.com
Cc: qemu-...@nongnu.org
Signed-off-by: Mao Zhongyi
Signed-off-by: Zhang Shengju
Reviewed-by: David Gibson
Acked-by: David Gibson
---
hw/pci-bridge/dec.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index 84492d5e5f..5b21c20e50 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -98,7 +98,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
return pci_bridge_get_sec_bus(br);
}
-static int pci_dec_21154_device_init(SysBusDevice *dev)
+static void pci_dec_21154_device_realize(DeviceState *dev, Error **errp)
{
PCIHostState *phb;
Why don't you use:
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
like in all the other patches from this series...?
Oops, sorry, I missed this one, will fix it in v3.
Thanks,
Mao
@@ -108,9 +108,8 @@ static int pci_dec_21154_device_init(SysBusDevice *dev)
dev, "pci-conf-idx", 0x1000);
memory_region_init_io(>data_mem, OBJECT(dev), _host_data_le_ops,
dev, "pci-data-idx", 0x1000);
-sysbus_init_mmio(dev, >conf_mem);
-sysbus_init_mmio(dev, >data_mem);
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >conf_mem);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >data_mem);
... this would save extra type checking here, as explain by Peter:
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03644.html
}
static void dec_21154_pci_host_realize(PCIDevice *d, Error **errp)
@@ -150,9 +149,9 @@ static const TypeInfo dec_21154_pci_host_info = {
static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
{
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
-sdc->init = pci_dec_21154_device_init;
+dc->realize = pci_dec_21154_device_realize;
}
static const TypeInfo pci_dec_21154_device_info = {
Re: [Qemu-devel] [PATCH 22/22] core/sysbus: remove the SysBusDeviceClass::initpath
On 11/24/18 2:19 AM, Peter Maydell wrote: On Fri, 23 Nov 2018 at 18:16, Eduardo Habkost wrote: I think this is good enough for now (as long as there's a comment like Peter suggested). Allowing parent_realize to be NULL would be inconvenient to all code that uses parent_realize today. It was done in v2, please review. Personally, I would love to get rid of parent_realize entirely. We could simply provide a helper to let device subclasses call the parent's realize function without the need to copy function pointers around. well, I will do it later. Agreed -- parent_realize is a hack that is working around a deficiency in our object model, and it would be nice to deal with that. But let's do our cleanups one at a time :-) OK, I see. Thanks, Mao thanks -- PMM
Re: [Qemu-devel] [PATCH 22/22] core/sysbus: remove the SysBusDeviceClass::initpath
On 11/23/18 5:02 PM, Peter Maydell wrote:
On 23 November 2018 at 03:10, maozy wrote:
In order to void the subclasses whose parent_realize field is
set to NULL, the k->realize function must be retained even
though it doesn't do anything practical. Just like this:
-/* TODO remove once all sysbus devices have been converted to realize*/
static void sysbus_realize(DeviceState *dev, Error **errp)
{
-SysBusDevice *sd = SYS_BUS_DEVICE(dev);
-SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
-
-if (!sbc->init) {
-return;
-}
-if (sbc->init(sd) < 0) {
-error_setg(errp, "Device initialization failed");
-}
}
it doesn't look elegant, but I didn't think of a better way, if you
can give me some hints, I really appreciate it. :)
If we do take this approach, we should have a comment which
says why we have an empty realize function, so that we don't
in future forget and delete the apparently unnecessary code...
OK, I got you, will do it.
Thanks,
Mao
thanks
-- PMM
Re: [Qemu-devel] [PATCH 06/22] dma/puv3_dma: Convert sysbus initfunction to realize function
On 11/20/18 10:46 PM, Peter Maydell wrote:
On 19 November 2018 at 12:08, Mao Zhongyi
wrote:
Use DeviceClass rather than SysBusDeviceClass in
puv3_dma_class_init().
Cc: g...@mprc.pku.edu.cn
Signed-off-by: Mao Zhongyi
Signed-off-by: Zhang Shengju
---
hw/dma/puv3_dma.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/hw/dma/puv3_dma.c b/hw/dma/puv3_dma.c
index b97a6c1767..c89eade029 100644
--- a/hw/dma/puv3_dma.c
+++ b/hw/dma/puv3_dma.c
@@ -76,7 +76,7 @@ static const MemoryRegionOps puv3_dma_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static int puv3_dma_init(SysBusDevice *dev)
+static void puv3_dma_realize(DeviceState *dev, Error **errp)
{
PUV3DMAState *s = PUV3_DMA(dev);
int i;
@@ -87,16 +87,14 @@ static int puv3_dma_init(SysBusDevice *dev)
memory_region_init_io(>iomem, OBJECT(s), _dma_ops, s, "puv3_dma",
PUV3_REGS_OFFSET);
-sysbus_init_mmio(dev, >iomem);
-
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
}
static void puv3_dma_class_init(ObjectClass *klass, void *data)
{
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
-sdc->init = puv3_dma_init;
+dc->realize = puv3_dma_realize;
}
static const TypeInfo puv3_dma_info = {
Reviewed-by: Peter Maydell
(I note that this device is missing a reset function and is
instead resetting in its init/realize function, but that's a
separate bug. It's also missing vmstate.)
OK, I will fix it later in a separate patch.
Thanks,
Mao
thanks
-- PMM
Re: [Qemu-devel] [PATCH 22/22] core/sysbus: remove the SysBusDeviceClass::initpath
Hi, Eduardo
On 11/20/18 7:31 AM, Eduardo Habkost wrote:
On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote:
Currently, all sysbus devices have been converted to realize(),
so remove this path.
Cc: ehabk...@redhat.com
Cc: th...@redhat.com
Cc: pbonz...@redhat.com
Cc: arm...@redhat.com
Cc: peter.mayd...@linaro.org
Cc: richard.hender...@linaro.org
Cc: alistair.fran...@wdc.com
Signed-off-by: Mao Zhongyi
Signed-off-by: Zhang Shengju
---
hw/core/sysbus.c| 15 ---
include/hw/sysbus.h | 3 ---
2 files changed, 18 deletions(-)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 7ac36ad3e7..030ad426c1 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t
ioport, uint32_t size)
}
}
-/* TODO remove once all sysbus devices have been converted to realize */
-static void sysbus_realize(DeviceState *dev, Error **errp)
-{
-SysBusDevice *sd = SYS_BUS_DEVICE(dev);
-SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
-
-if (!sbc->init) {
-return;
-}
-if (sbc->init(sd) < 0) {
-error_setg(errp, "Device initialization failed");
-}
-}
Nice. :)
-
DeviceState *sysbus_create_varargs(const char *name,
hwaddr addr, ...)
{
@@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
static void sysbus_device_class_init(ObjectClass *klass, void *data)
{
DeviceClass *k = DEVICE_CLASS(klass);
-k->realize = sysbus_realize;
Have you ensured this won't break any subclasses that
saved the original realize function on a parent_realize field?
Thanks for the catch.
Now they will have parent_realize set to NULL.
In order to void the subclasses whose parent_realize field is
set to NULL, the k->realize function must be retained even
though it doesn't do anything practical. Just like this:
-/* TODO remove once all sysbus devices have been converted to realize*/
static void sysbus_realize(DeviceState *dev, Error **errp)
{
-SysBusDevice *sd = SYS_BUS_DEVICE(dev);
-SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
-
-if (!sbc->init) {
-return;
-}
-if (sbc->init(sd) < 0) {
-error_setg(errp, "Device initialization failed");
-}
}
it doesn't look elegant, but I didn't think of a better way, if you
can give me some hints, I really appreciate it. :)
Thanks,
Mao
Most of them use device_class_set_parent_realize() to implement
that.
k->bus_type = TYPE_SYSTEM_BUS;
/*
* device_add plugs devices into a suitable bus. For "real" buses,
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 0b59a3b8d6..1aedcf05c9 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -38,9 +38,6 @@ typedef struct SysBusDevice SysBusDevice;
typedef struct SysBusDeviceClass {
/*< private >*/
DeviceClass parent_class;
-/*< public >*/
-
-int (*init)(SysBusDevice *dev);
/*
* Let the sysbus device format its own non-PIO, non-MMIO unit address.
--
2.17.1
Re: [Qemu-devel] [PATCH 07/22] gpio/puv3_gpio: Convert sysbus initfunction to realize function
On 11/19/18 10:31 PM, Peter Maydell wrote:
On 19 November 2018 at 12:08, Mao Zhongyi
wrote:
Use DeviceClass rather than SysBusDeviceClass in
puv3_gpio_class_init().
Cc: g...@mprc.pku.edu.cn
Signed-off-by: Mao Zhongyi
Signed-off-by: Zhang Shengju
---
hw/gpio/puv3_gpio.c | 28 +---
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/hw/gpio/puv3_gpio.c b/hw/gpio/puv3_gpio.c
index 445afccf9f..bd6fc43aae 100644
--- a/hw/gpio/puv3_gpio.c
+++ b/hw/gpio/puv3_gpio.c
@@ -99,7 +99,7 @@ static const MemoryRegionOps puv3_gpio_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static int puv3_gpio_init(SysBusDevice *dev)
+static void puv3_gpio_realize(DeviceState *dev, Error **errp)
{
PUV3GPIOState *s = PUV3_GPIO(dev);
@@ -107,28 +107,26 @@ static int puv3_gpio_init(SysBusDevice *dev)
s->reg_GPDR = 0;
/* FIXME: these irqs not handled yet */
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW0]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW1]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW2]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW3]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW4]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW5]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW6]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW7]);
-sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOHIGH]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW0]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW1]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW2]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW3]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW4]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW5]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW6]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOLOW7]);
+sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq[PUV3_IRQS_GPIOHIGH]);
memory_region_init_io(>iomem, OBJECT(s), _gpio_ops, s,
"puv3_gpio",
PUV3_REGS_OFFSET);
-sysbus_init_mmio(dev, >iomem);
-
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
}
The SYS_BUS_DEVICE() cast is not free (it does type
checking). It's better to do it once, ie
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
and use the variable, if we're going to be using
it several times.
Wow, sure，thanks a lot, will fix it in the next.
mao
thanks
-- PMM
Re: [Qemu-devel] [qemu-s390x] [PATCH 21/22] event-facility: ChangeSysBusDeviceClass *sbdc to SysBusDeviceClass *sbc
On 11/19/18 10:10 PM, Thomas Huth wrote: On 2018-11-19 13:25, Cornelia Huck wrote: On Mon, 19 Nov 2018 20:08:19 +0800 Mao Zhongyi wrote: Most of the SysBusDeviceClass variables are named sbc, and sbdc here is a bit weird, so changing sbdc to keep it consistent with others might look good. A quick git grep also gives sbd and k as variable names, and it is used in a total of two lines which have not been touched since 2013, so... meh. If others like the change, I'm not opposed to merging, though. I think I agree with Cornelia, just changing the variable name because it's named differently in a couple of other places is just unnecessary code churn, which makes "git blame" output harder to read later. I'd suggest to drop this patch, too. Ok, I got it. :) Thanks, Mao Thomas
Re: [Qemu-devel] [PATCH v2 2/3] qemu-iotests: remove unused variablehere
Hi, Philippe On 10/25/18 7:26 AM, Philippe Mathieu-Daudé wrote: Hi Mao, On 24/10/18 11:40, Mao Zhongyi wrote: run git grep '\$here' tests/qemu-iotests This command doesn't look correct, I believe you have to use either - git grep '$here' or - git grep \$here Yeah, it should be '$here' or \$here ... :) I hope whoever is going to push this series can fix it before pushing. If not please let me know and I am going to send a v3. Thanks in advance. Mao has 0 hits, which means we are setting a variable that no use, so execute the following cmd to remove all of the 'here=...' lines as dead code. This seems to have been removed in e8f8624d3b920de. sed -i '/here=/d' $(git grep -l 'here=' tests/qemu-iotests) Cc: kw...@redhat.com Cc: mre...@redhat.com Cc: ebl...@redhat.com Suggested-by: Eric Blake Please Cc Eric if he suggested, so he can review. Signed-off-by: Mao Zhongyi --- tests/qemu-iotests/001 | 1 - tests/qemu-iotests/002 | 1 - tests/qemu-iotests/003 | 1 - tests/qemu-iotests/004 | 1 - tests/qemu-iotests/005 | 1 - tests/qemu-iotests/007 | 1 - tests/qemu-iotests/008 | 1 - tests/qemu-iotests/009 | 1 - tests/qemu-iotests/010 | 1 - tests/qemu-iotests/011 | 1 - tests/qemu-iotests/012 | 1 - tests/qemu-iotests/013 | 1 - tests/qemu-iotests/014 | 1 - tests/qemu-iotests/015 | 1 - tests/qemu-iotests/017 | 1 - tests/qemu-iotests/018 | 1 - tests/qemu-iotests/019 | 1 - tests/qemu-iotests/020 | 1 - tests/qemu-iotests/021 | 1 - tests/qemu-iotests/022 | 1 - tests/qemu-iotests/023 | 1 - tests/qemu-iotests/024 | 1 - tests/qemu-iotests/025 | 1 - tests/qemu-iotests/026 | 1 - tests/qemu-iotests/027 | 1 - tests/qemu-iotests/028 | 1 - tests/qemu-iotests/029 | 1 - tests/qemu-iotests/031 | 1 - tests/qemu-iotests/032 | 1 - tests/qemu-iotests/033 | 1 - tests/qemu-iotests/034 | 1 - tests/qemu-iotests/035 | 1 - tests/qemu-iotests/036 | 1 - tests/qemu-iotests/037 | 1 - tests/qemu-iotests/038 | 1 - tests/qemu-iotests/039 | 1 - tests/qemu-iotests/042 | 1 - tests/qemu-iotests/043 | 1 - tests/qemu-iotests/046 | 1 - tests/qemu-iotests/047 | 1 - tests/qemu-iotests/049 | 1 - tests/qemu-iotests/050 | 1 - tests/qemu-iotests/051 | 1 - tests/qemu-iotests/052 | 1 - tests/qemu-iotests/053 | 1 - tests/qemu-iotests/054 | 1 - tests/qemu-iotests/058 | 1 - tests/qemu-iotests/059 | 1 - tests/qemu-iotests/060 | 1 - tests/qemu-iotests/061 | 1 - tests/qemu-iotests/062 | 1 - tests/qemu-iotests/063 | 1 - tests/qemu-iotests/064 | 1 - tests/qemu-iotests/066 | 1 - tests/qemu-iotests/067 | 1 - tests/qemu-iotests/068 | 1 - tests/qemu-iotests/069 | 1 - tests/qemu-iotests/070 | 1 - tests/qemu-iotests/071 | 1 - tests/qemu-iotests/072 | 1 - tests/qemu-iotests/073 | 1 - tests/qemu-iotests/075 | 1 - tests/qemu-iotests/076 | 1 - tests/qemu-iotests/077 | 1 - tests/qemu-iotests/078 | 1 - tests/qemu-iotests/079 | 1 - tests/qemu-iotests/080 | 1 - tests/qemu-iotests/081 | 1 - tests/qemu-iotests/082 | 1 - tests/qemu-iotests/083 | 1 - tests/qemu-iotests/084 | 1 - tests/qemu-iotests/085 | 1 - tests/qemu-iotests/086 | 1 - tests/qemu-iotests/087 | 1 - tests/qemu-iotests/088 | 1 - tests/qemu-iotests/089 | 1 - tests/qemu-iotests/090 | 1 - tests/qemu-iotests/091 | 1 - tests/qemu-iotests/092 | 1 - tests/qemu-iotests/094 | 1 - tests/qemu-iotests/095 | 1 - tests/qemu-iotests/097 | 1 - tests/qemu-iotests/098 | 1 - tests/qemu-iotests/099 | 1 - tests/qemu-iotests/101 | 1 - tests/qemu-iotests/102 | 1 - tests/qemu-iotests/103 | 1 - tests/qemu-iotests/104 | 1 - tests/qemu-iotests/105 | 1 - tests/qemu-iotests/106 | 1 - tests/qemu-iotests/107 | 1 - tests/qemu-iotests/108 | 1 - tests/qemu-iotests/109 | 1 - tests/qemu-iotests/110 | 1 - tests/qemu-iotests/111 | 1 - tests/qemu-iotests/112 | 1 - tests/qemu-iotests/113 | 1 - tests/qemu-iotests/114 | 1 - tests/qemu-iotests/115 | 1 - tests/qemu-iotests/116 | 1 - tests/qemu-iotests/117 | 1 - tests/qemu-iotests/119 | 1 - tests/qemu-iotests/120 | 1 - tests/qemu-iotests/121 | 1 - tests/qemu-iotests/122 | 1 - tests/qemu-iotests/123 | 1 - tests/qemu-iotests/125 | 1 - tests/qemu-iotests/126 | 1 - tests/qemu-iotests/127 | 1 - tests/qemu-iotests/128 | 1 - tests/qemu-iotests/130 | 1 - tests/qemu-iotests/131 | 1 - tests/qemu-iotests/133 | 1 - tests/qemu-iotests/134 | 1 - tests/qemu-iotests/135 | 1 - tests/qemu-iotests/137 | 1 - tests/qemu-iotests/138 | 1 - tests/qemu-iotests/140 | 1 - tests/qemu-iotests/141 | 1 - tests/qemu-iotests/142 | 1 - tests/qemu-iotests/143 | 1 - tests/qemu-iotests/144 | 1 - tests/qemu-iotests/145 | 1 - tests/qemu-iotests/146 | 1 - tests/qemu-iotests/150 | 1 - tests/qemu-iotests/153 | 1 - tests/qemu-iotests/154 | 1 - tests/qemu-iotests/156 | 1 - tests/qemu-iotests/157 | 1 - tests/qemu-iotests/158 | 1 - tests/qemu-iotests/159 | 1 - tests/qemu-iotests/160 | 1
Re: [Qemu-devel] [PATCH] qemu-iotests: convert `pwd` and $(pwd) to $PWD
Hi, Eric
On 10/23/18 4:21 PM, Eric Blake wrote:
On 10/22/18 2:48 PM, Mao Zhongyi wrote:
The subject line says "what", but the commit body should say "why". My
suggestion:
POSIX requires $PWD to be reliable, and we expect all shells used by
qemu scripts to be relatively close to POSIX. Thus, it is smarter to
avoid forking the pwd executable for something that is already available
in the environment.
If it was done mechanically, it may also help to capture the command you
used to drive the change (sed or otherwise), to make it easier for
someone backporting this patch to rerun the same steps to regenerate the
patch for a different set of files on the backport.
Suggested-by: Eric Blake
Signed-off-by: Mao Zhongyi
---
+++ b/tests/qemu-iotests/001
@@ -24,7 +24,7 @@ owner=h...@lst.de
seq=`basename $0`
echo "QA output created by $seq"
-here=`pwd`
+here=$PWD
As this is a mechanical search-and-replace, this is fine. However,
git grep '\$here' tests/qemu-iotests
has 0 hits, which means we are setting a variable that has no use. A
good followup patch would be to delete all of the 'here=...' lines as
dead code. Or even do that first, and then this patch second, for less
churn.
+++ b/tests/qemu-iotests/check
@@ -80,17 +80,17 @@ _full_imgfmt_details()
_full_platform_details()
{
- os=`uname -s`
- host=`hostname -s`
- kernel=`uname -r`
- platform=`uname -m`
+ os=$(uname -s)
+ host=$(hostname -s)
+ kernel=$(uname -r)
+ platform=$(uname -m)
echo "$os/$platform $host $kernel"
These changes are unrelated to the commit subject. Please split them
into a separate commit...
}
# $1 = prog to look for
set_prog_path()
{
- p=`command -v $1 2> /dev/null`
+ p=$(command -v $1 2> /dev/null)
if [ -n "$p" -a -x "$p" ]; then
type -p "$p"
else
@@ -99,7 +99,7 @@ set_prog_path()
}
if [ -z "$TEST_DIR" ]; then
- TEST_DIR=`pwd`/scratch
+ TEST_DIR=$PWD/scratch
fi
...This hunk is okay, but the rest of the file is not.
+++ b/tests/qemu-iotests/common.config
@@ -21,11 +21,11 @@ export LANG=C
PATH=".:$PATH"
-HOSTOS=`uname -s`
-arch=`uname -m`
+HOSTOS=$(uname -s)
+arch=$(uname -m)
Another file with too many hunks.
Looking forward to v2.
Thank you very much for your kind reply, it's updated
in v2, please review. :)
Thanks,
mao
Re: [Qemu-devel] [PATCH v3 2/2] po/Makefile: Modern shell scripting (use $()instead of ``)
On 10/22/18 6:59 PM, Thomas Huth wrote: On 2018-10-22 09:48, Mao Zhongyi wrote: Various shell files contain a mix between obsolete `` and modern $(); It would be nice to convert to using $() everywhere. Cc: phi...@redhat.com Cc: peter.mayd...@linaro.org Cc: th...@redhat.com Cc: s...@weilnetz.de Signed-off-by: Mao Zhongyi Signed-off-by: Thomas Huth That last line should be "Reviewed-by:" instead - but that can be fixed when the patch is picked up, no need to resend just because of this nit. OK, just because I didn't explicitly get R-b. :) Thanks, Mao Thomas
Re: [Qemu-devel] [PATCH v3 0/3] use object link instead of qdev property
Hi, Gerd On 10/19/18 4:04 PM, Gerd Hoffmann wrote: On Mon, Oct 15, 2018 at 11:26:39AM +0800, Mao Zhongyi wrote: According to qdev-properties.h, properties of pointer type should be avoid, so convert qdev property to link, Whilst we are here, also update some hardcoded strings with already defineded macros. Patch series breaks "make check". Already fixed it in v4, please review. Thanks, mao cheers, Gerd
Re: [Qemu-devel] [PATCH v2 1/3] qemu-iotests: Modern shellscripting(use $() instead of ``)
Hi, Eric On 10/18/18 11:28 AM, Eric Blake wrote: On 10/17/18 10:17 PM, Mao Zhongyi wrote: Various shell files contain a mix between obsolete `` and modern $(); It would be nice to convert to using $() everywhere. `pwd` and `basename $0` are in 231 files under directory tests/qemu-iotests, so replaced it with the following: sed -i 's/`pwd`/$(pwd)/g' $(git grep -l "\`pwd\`") No. Instead, I'd rather a separate patch that does: s/`pwd`/$PWD/ s/\$(pwd)/$PWD/ since POSIX requires $PWD to be sane, and thus save us a wasted forked process. I got you, thank you very much. sed -i 's/`basename $0`/$(basename $0)/g' $(git grep -l "basename \$0") A small amount of the rest is manually modified. Cc: kw...@redhat.com Cc: mre...@redhat.com Cc: qemu-bl...@nongnu.org Signed-off-by: Mao Zhongyi --- +++ b/tests/qemu-iotests/001 @@ -21,10 +21,10 @@ # creator owner=h...@lst.de -seq=`basename $0` +seq=$(basename $0) echo "QA output created by $seq" -here=`pwd` +here=$(pwd) status=1 # failure is the default! At one point, someone (Jeff?) proposed a cleanup patch that got rid of a lot of cruft in iotests, including the fact that scripts that don't use $seq don't need to assign seq=$(basename $0). We should probably revive that rather than just making pointless churn on stuff that is garbage anyways. But I don't have time to look up a URL to that older series at the moment. I think I might have found this patchset, but it was a long time ago. https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg04056.html I will remove this patch from this series and resend a separate patch to replace `pwd` and "$(pwd)" with $PWD. Am I right? Thanks, Mao
Re: [Qemu-devel] [PATCH 3/3] po/Makefile: Modern shell scripting (use $() insteadof ``)
On 10/17/18 5:54 PM, Thomas Huth wrote: On 2018-10-17 11:44, Mao Zhongyi wrote: Various shell files contain a mix between obsolete `` and modern $(); It would be nice to convert to using $() everywhere. Cc: phi...@redhat.com Cc: peter.mayd...@linaro.org Cc: th...@redhat.com Cc: s...@weilnetz.de Signed-off-by: Mao Zhongyi --- po/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/po/Makefile b/po/Makefile index e47e262ee6..10605e8eb3 100644 --- a/po/Makefile +++ b/po/Makefile @@ -36,7 +36,7 @@ clean: install: $(OBJS) for obj in $(OBJS); do \ - base=`basename $$obj .mo`; \ + base=$(basename $$obj .mo); \ You're changing a Makefile here, so you need to "escape" the "$" by doubling it: base=$$(basename $$obj .mo); \ OK, I see your point. Thanks, Mao Thomas
Re: [Qemu-devel] [PATCH 1/3] archive-source.sh: Modern shellscripting (use $() instead of ``)
Hi, Eric On 10/15/18 11:44 PM, Eric Blake wrote: On 10/15/18 1:51 AM, Mao Zhongyi wrote: Various shell files contain a mix between obsolete `` and modern $(); use of `` is only required when using /bin/sh on Solaris. It would be nice to convert to using $() everywhere, or at least in all bash scripts, as well as in all scripts that are known to not be run on Solaris. Did you have a particular program you used to find these, or just grep? Just grep in scripts dir, I also entered the file to make sure I didn't missing anything. In addition, I will send a separated patch to replace all obsolete `` in the source tree. Also, when sending a series, don't forget a 0/3 cover letter. OK, I see. :) Thanks, Mao Signed-off-by: Mao Zhongyi --- scripts/archive-source.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Re: [Qemu-devel] [PATCH 1/3] archive-source.sh: Modern shellscripting (use $() instead of ``)
On 10/15/18 3:07 PM, Thomas Huth wrote:
On 2018-10-15 08:51, Mao Zhongyi wrote:
Various shell files contain a mix between obsolete `` and
modern $(); use of `` is only required when using /bin/sh
on Solaris. It would be nice to convert to using $()
everywhere, or at least in all bash scripts, as well as in
all scripts that are known to not be run on Solaris.
FWIW, I think we do not have to worry about Solaris' /bin/sh here
anymore. Somebody tried to compile on Solaris a couple of weeks ago, and
found out that you need a proper POSIX-compliant shell for compiling
QEMU, so /bin/sh can not be used here anymore anyway.
Thanks for your quick review and clarification. I got it.
In addition, I fount that I didn't completely replace `` in
git-submodule.sh, so I plan to resend it later.
Thanks,
Mao
Signed-off-by: Mao Zhongyi
---
scripts/archive-source.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
index 4e63774f9a..62bd22578b 100755
--- a/scripts/archive-source.sh
+++ b/scripts/archive-source.sh
@@ -18,7 +18,7 @@ if test $# -lt 1; then
error "Usage: $0 "
fi
-tar_file=`realpath "$1"`
+tar_file=$(realpath "$1")
list_file="${tar_file}.list"
vroot_dir="${tar_file}.vroot"
@@ -34,7 +34,7 @@ if git diff-index --quiet HEAD -- &>/dev/null
then
HEAD=HEAD
else
-HEAD=`git stash create`
+HEAD=$(git stash create)
fi
git clone --shared . "$vroot_dir"
test $? -ne 0 && error "failed to clone into '$vroot_dir'"
Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH v2 2/3] audio: use object link instead ofqdevproperty to pass wm8750 reference
Hi, Philippe
On 10/13/18 2:40 AM, Philippe Mathieu-Daudé wrote:
Hi Mao,
On 12/10/2018 14:30, Philippe Mathieu-Daudé wrote:
Cc'ing Eduardo and Thomas.
On 12/10/2018 13:51, maozy wrote:
Hi, Philippe
On 10/12/18 5:53 PM, Philippe Mathieu-Daudé wrote:
Hi Mao,
On 12/10/2018 10:30, Mao Zhongyi wrote:
According to qdev-properties.h, properties of pointer type should
be avoided, it seems a link type property is a good substitution.
Cc: Jan Kiszka
Cc: Peter Maydell
Cc: Gerd Hoffmann
To: qemu-...@nongnu.org
Signed-off-by: Mao Zhongyi
---
hw/arm/musicpal.c | 3 ++-
hw/audio/marvell_88w8618.c | 14 ++
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 3dafb41b0b..ac266f9253 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1695,7 +1695,8 @@ static void musicpal_init(MachineState *machine)
wm8750_dev = i2c_create_slave(i2c, TYPE_WM8750, MP_WM_ADDR);
dev = qdev_create(NULL, "mv88w8618_audio");
s = SYS_BUS_DEVICE(dev);
- qdev_prop_set_ptr(dev, TYPE_WM8750, wm8750_dev);
+ object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
+ TYPE_WM8750, NULL);
qdev_init_nofail(dev);
sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
index cf6ce6979b..baab4a3d53 100644
--- a/hw/audio/marvell_88w8618.c
+++ b/hw/audio/marvell_88w8618.c
@@ -15,6 +15,7 @@
#include "hw/i2c/i2c.h"
#include "hw/audio/wm8750.h"
#include "audio/audio.h"
+#include "qapi/error.h"
#define MP_AUDIO_SIZE 0x1000
@@ -252,6 +253,11 @@ static void mv88w8618_audio_init(Object *obj)
memory_region_init_io(>iomem, obj, _audio_ops, s,
"audio", MP_AUDIO_SIZE);
sysbus_init_mmio(dev, >iomem);
+
+ object_property_add_link(OBJECT(dev), "mv88w8618", TYPE_WM8750,
+ (Object **) >wm,
+ qdev_prop_allow_set_link_before_realize,
+ 0, _abort);
}
static void mv88w8618_audio_realize(DeviceState *dev, Error **errp)
@@ -279,11 +285,6 @@ static const VMStateDescription
mv88w8618_audio_vmsd = {
}
};
-static Property mv88w8618_audio_properties[] = {
- DEFINE_PROP_PTR(TYPE_WM8750, mv88w8618_audio_state, wm),
- {/* end of list */},
-};
-
static void mv88w8618_audio_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -291,9 +292,6 @@ static void
mv88w8618_audio_class_init(ObjectClass *klass, void *data)
dc->realize = mv88w8618_audio_realize;
dc->reset = mv88w8618_audio_reset;
dc->vmsd = _audio_vmsd;
- dc->props = mv88w8618_audio_properties;
- /* Reason: pointer property "wm8750" */
- dc->user_creatable = false;
Having a link property isn't it the same restriction?
This task can found in
https://wiki.qemu.org/Contribute/BiteSizedTasks#Device_models
Convert qdev pointer properties (defined with DEFINE_PROP_PTR) to QOM
links. Example: commit 873b4d3.
I agree with the QOM conversion (the rest of this patch), what I'm
wondering is if declaring this device now user_creatable is correct. I
don't think we can set link property from command line, but maybe I'm
wrong because I never investigate/tried to do it.
Maybe devices having link property are automatically marked as
user_creatable = false, then your change would be correct (except you
should explicit that in the commit message).
I'll post another mail on the list to ask about that.
On https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02531.html
Peter answered:
I think whether a device with a link property is
user creatable might depend on what the property is
for and whether the device has a useful fallback for
"link not connected".
So if you look the realize function, there is no check on the linked object:
static void mv88w8618_audio_realize(DeviceState *dev, Error **errp)
{
mv88w8618_audio_state *s = MV88W8618_AUDIO(dev);
wm8750_data_req_set(s->wm, mv88w8618_audio_callback, s);
}
Then the called function dereference the WM8750State:
void wm8750_data_req_set(DeviceState *dev, data_req_cb *data_req,
void *opaque)
{
WM8750State *s = WM8750(dev);
s->data_req = data_req;
s->opaque = opaque;
}
So this patch is incorrect. You should either keep the
'dc->user_creatable = false' line, or add a check in the realize()
function, as here:
static void bcm2836_realize(DeviceState *dev, Error **errp)
{
Object *obj;
Error *err = NULL;
obj = object_property_get_link(OBJECT(dev), "ram", );
if (ob
Re: [Qemu-devel] [PATCH v2 2/3] audio: use object link instead ofqdev property to pass wm8750 reference
Hi, Philippe
On 10/12/18 5:53 PM, Philippe Mathieu-Daudé wrote:
Hi Mao,
On 12/10/2018 10:30, Mao Zhongyi wrote:
According to qdev-properties.h, properties of pointer type should
be avoided, it seems a link type property is a good substitution.
Cc: Jan Kiszka
Cc: Peter Maydell
Cc: Gerd Hoffmann
To: qemu-...@nongnu.org
Signed-off-by: Mao Zhongyi
---
hw/arm/musicpal.c | 3 ++-
hw/audio/marvell_88w8618.c | 14 ++
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 3dafb41b0b..ac266f9253 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1695,7 +1695,8 @@ static void musicpal_init(MachineState *machine)
wm8750_dev = i2c_create_slave(i2c, TYPE_WM8750, MP_WM_ADDR);
dev = qdev_create(NULL, "mv88w8618_audio");
s = SYS_BUS_DEVICE(dev);
-qdev_prop_set_ptr(dev, TYPE_WM8750, wm8750_dev);
+object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
+ TYPE_WM8750, NULL);
qdev_init_nofail(dev);
sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
index cf6ce6979b..baab4a3d53 100644
--- a/hw/audio/marvell_88w8618.c
+++ b/hw/audio/marvell_88w8618.c
@@ -15,6 +15,7 @@
#include "hw/i2c/i2c.h"
#include "hw/audio/wm8750.h"
#include "audio/audio.h"
+#include "qapi/error.h"
#define MP_AUDIO_SIZE 0x1000
@@ -252,6 +253,11 @@ static void mv88w8618_audio_init(Object *obj)
memory_region_init_io(>iomem, obj, _audio_ops, s,
"audio", MP_AUDIO_SIZE);
sysbus_init_mmio(dev, >iomem);
+
+object_property_add_link(OBJECT(dev), "mv88w8618", TYPE_WM8750,
+ (Object **) >wm,
+ qdev_prop_allow_set_link_before_realize,
+ 0, _abort);
}
static void mv88w8618_audio_realize(DeviceState *dev, Error **errp)
@@ -279,11 +285,6 @@ static const VMStateDescription mv88w8618_audio_vmsd = {
}
};
-static Property mv88w8618_audio_properties[] = {
-DEFINE_PROP_PTR(TYPE_WM8750, mv88w8618_audio_state, wm),
-{/* end of list */},
-};
-
static void mv88w8618_audio_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -291,9 +292,6 @@ static void mv88w8618_audio_class_init(ObjectClass *klass,
void *data)
dc->realize = mv88w8618_audio_realize;
dc->reset = mv88w8618_audio_reset;
dc->vmsd = _audio_vmsd;
-dc->props = mv88w8618_audio_properties;
-/* Reason: pointer property "wm8750" */
-dc->user_creatable = false;
Having a link property isn't it the same restriction?
This task can found in
https://wiki.qemu.org/Contribute/BiteSizedTasks#Device_models
Convert qdev pointer properties (defined with DEFINE_PROP_PTR) to QOM
links. Example: commit 873b4d3.
Thanks,
Mao
}
static const TypeInfo mv88w8618_audio_info = {
Re: [Qemu-devel] [PATCH v2 1/3] audio: use TYPE_WM8750 instead of ahardcoded string
Hi, Philippe
On 10/12/18 5:50 PM, Philippe Mathieu-Daudé wrote:
On 12/10/2018 10:30, Mao Zhongyi wrote:
Cc: Jan Kiszka
Cc: Peter Maydell
Cc: Gerd Hoffmann
To: qemu-...@nongnu.org
"To: qemu-...@nongnu.org" is probably not relevant in the commit message.
The Linux kernel describes the 'Cc:' line in the "Submitting patches:
the essential guide to getting your code into the kernel" document as:
If a person has had the opportunity to comment on a patch, but has not
provided such comments, you may optionally add a Cc: tag to the patch.
This is the only tag which might be added without an explicit action
by the person it names - but it should indicate that this person was
copied on the patch. This tag documents that potentially interested
parties have been included in the discussion.
I got it, thank you very much for the detailed explanation. I will
remove it. :)
Thanks,
Mao
Signed-off-by: Mao Zhongyi
Reviewed-by: Philippe Mathieu-Daudé
---
hw/arm/musicpal.c | 2 +-
hw/audio/marvell_88w8618.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index c807010e83..3dafb41b0b 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1695,7 +1695,7 @@ static void musicpal_init(MachineState *machine)
wm8750_dev = i2c_create_slave(i2c, TYPE_WM8750, MP_WM_ADDR);
dev = qdev_create(NULL, "mv88w8618_audio");
s = SYS_BUS_DEVICE(dev);
-qdev_prop_set_ptr(dev, "wm8750", wm8750_dev);
+qdev_prop_set_ptr(dev, TYPE_WM8750, wm8750_dev);
qdev_init_nofail(dev);
sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
index e546892d3c..cf6ce6979b 100644
--- a/hw/audio/marvell_88w8618.c
+++ b/hw/audio/marvell_88w8618.c
@@ -280,7 +280,7 @@ static const VMStateDescription mv88w8618_audio_vmsd = {
};
static Property mv88w8618_audio_properties[] = {
-DEFINE_PROP_PTR("wm8750", mv88w8618_audio_state, wm),
+DEFINE_PROP_PTR(TYPE_WM8750, mv88w8618_audio_state, wm),
{/* end of list */},
};
Re: [Qemu-devel] [PATCH v2 3/3] audio: use TYPE_MV88W8618_AUDIO instead of hardcoded string
Sorry for the noise, there is something wrong with this patch, I will fix it and resend this patchset. Thanks, Mao On 10/12/18 4:30 PM, Mao Zhongyi wrote: Cc: Jan Kiszka Cc: Philippe Mathieu-Daudé Cc: Peter Maydell To: qemu-...@nongnu.org Signed-off-by: Mao Zhongyi --- hw/arm/musicpal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c index ac266f9253..9648b3af44 100644 --- a/hw/arm/musicpal.c +++ b/hw/arm/musicpal.c @@ -1693,7 +1693,7 @@ static void musicpal_init(MachineState *machine) } wm8750_dev = i2c_create_slave(i2c, TYPE_WM8750, MP_WM_ADDR); -dev = qdev_create(NULL, "mv88w8618_audio"); +dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO); s = SYS_BUS_DEVICE(dev); object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev), TYPE_WM8750, NULL);
Re: [Qemu-devel] [PATCH 4/4] audio: use existing macros istead ofhardcoded strings
On 10/11/18 7:05 PM, Peter Maydell wrote:
On 11 October 2018 at 11:45, Philippe Mathieu-Daudé wrote:
On 11/10/2018 11:00, Mao Zhongyi wrote:
Cc: Jan Kiszka
Cc: Peter Maydell
Cc: Gerd Hoffmann
To: qemu-...@nongnu.org
Signed-off-by: Mao Zhongyi
---
hw/arm/musicpal.c | 16
hw/audio/marvell_88w8618.c | 3 +--
include/hw/audio/wm8750.h | 1 +
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index ac266f9253..6425f1d50f 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -406,7 +406,7 @@ static void mv88w8618_eth_realize(DeviceState *dev, Error
**errp)
}
static const VMStateDescription mv88w8618_eth_vmsd = {
-.name = "mv88w8618_eth",
+.name = TYPE_MV88W8618_ETH,
I understand TYPE_device name might changed, but once used,
VMStateDescription shouldn't, else this would break migration.
Thus I wouldn't recommend using TYPE_name in VMStateDescription.name.
Since I'm not sure I cc'ed the migration maintainers.
Yes, that's the usual approach -- the vmstate struct names
aren't inherently the same as the QOM type names, and so
we keep them decoupled to avoid accidentally breaking migration.
OK, I got it, will fix it in the next, thanks for the clarification.
thanks
Mao
thanks
-- PMM
Re: [Qemu-devel] [PATCH 1/4] wm8750: remove duplicate macro
On 10/11/18 6:39 PM, Philippe Mathieu-Daudé wrote:
Hi Mao,
On 11/10/2018 11:00, Mao Zhongyi wrote:
The header file wm8750.h contains '#define TYPE_WM8750 "wm8750"'
macro, but '#define CODEC "wm8750"' macro is redefined in wm8750.c,
just remove the local CODEC macro and replace it with TYPE_WM8750.
Cc: Gerd Hoffmann
Signed-off-by: Mao Zhongyi
---
hw/audio/wm8750.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
index f4aa838f62..4be3602079 100644
--- a/hw/audio/wm8750.c
+++ b/hw/audio/wm8750.c
@@ -15,8 +15,6 @@
#define IN_PORT_N 3
#define OUT_PORT_N3
-#define CODEC "wm8750"
-
typedef struct {
int adc;
int adc_hz;
@@ -204,11 +202,11 @@ static void wm8750_set_format(WM8750State *s)
in_fmt.fmt = AUD_FMT_S16;
s->adc_voice[0] = AUD_open_in(>card, s->adc_voice[0],
-CODEC ".input1", s, wm8750_audio_in_cb, _fmt);
+TYPE_WM8750 ".input1", s, wm8750_audio_in_cb, _fmt);
I don't think this is correct. The TYPE_name could change, but the CODEC
shouldn't change. Both definitions are different.
OK, I will remove this patch.
Thanks,
Mao
Regards,
Phil.
s->adc_voice[1] = AUD_open_in(>card, s->adc_voice[1],
-CODEC ".input2", s, wm8750_audio_in_cb, _fmt);
+TYPE_WM8750 ".input2", s, wm8750_audio_in_cb, _fmt);
s->adc_voice[2] = AUD_open_in(>card, s->adc_voice[2],
-CODEC ".input3", s, wm8750_audio_in_cb, _fmt);
+TYPE_WM8750 ".input3", s, wm8750_audio_in_cb, _fmt);
/* Setup output */
out_fmt.endianness = 0;
@@ -217,12 +215,12 @@ static void wm8750_set_format(WM8750State *s)
out_fmt.fmt = AUD_FMT_S16;
s->dac_voice[0] = AUD_open_out(>card, s->dac_voice[0],
-CODEC ".speaker", s, wm8750_audio_out_cb, _fmt);
+TYPE_WM8750 ".speaker", s, wm8750_audio_out_cb, _fmt);
s->dac_voice[1] = AUD_open_out(>card, s->dac_voice[1],
-CODEC ".headphone", s, wm8750_audio_out_cb, _fmt);
+TYPE_WM8750 ".headphone", s, wm8750_audio_out_cb,
_fmt);
/* MONOMIX is also in stereo for simplicity */
s->dac_voice[2] = AUD_open_out(>card, s->dac_voice[2],
-CODEC ".monomix", s, wm8750_audio_out_cb, _fmt);
+TYPE_WM8750 ".monomix", s, wm8750_audio_out_cb, _fmt);
/* no sense emulating OUT3 which is a mix of other outputs */
wm8750_vol_update(s);
@@ -584,7 +582,7 @@ static int wm8750_post_load(void *opaque, int version_id)
}
static const VMStateDescription vmstate_wm8750 = {
-.name = CODEC,
+.name = TYPE_WM8750,
.version_id = 0,
.minimum_version_id = 0,
.pre_save = wm8750_pre_save,
@@ -621,7 +619,7 @@ static void wm8750_realize(DeviceState *dev, Error **errp)
{
WM8750State *s = WM8750(dev);
-AUD_register_card(CODEC, >card);
+AUD_register_card(TYPE_WM8750, >card);
wm8750_reset(I2C_SLAVE(s));
}
