On 15/05/23 8:46 pm, Het Gala wrote:
On 15/05/23 8:16 pm, Juan Quintela wrote:
Het Gala <het.g...@nutanix.com> wrote:
On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
Socket transport backend for 'migrate'/'migrate-incoming' QAPIs
accept
new wire protocol of MigrateAddress struct.
It is achived by parsing 'uri' string and storing migration
parameters
required for socket connection into well defined SocketAddress
struct.
Suggested-by: Aravind Retnakaran <aravind.retnaka...@nutanix.com>
Signed-off-by: Het Gala <het.g...@nutanix.com>
---
migration/exec.c | 4 ++--
migration/exec.h | 4 ++++
migration/migration.c | 44
+++++++++++++++++++++++++++++++------------
migration/socket.c | 34 +++++----------------------------
migration/socket.h | 7 ++++---
5 files changed, 47 insertions(+), 46 deletions(-)
diff --git a/migration/exec.c b/migration/exec.c
index 2bf882bbe1..c4a3293246 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -27,7 +27,6 @@
#include "qemu/cutils.h"
#ifdef WIN32
-const char *exec_get_cmd_path(void);
const char *exec_get_cmd_path(void)
Maintainers please advice. If I want to see thatthe build is proper, how
to enable WIN32 support on a CentOS guest operating system (what all
dependencies to install, what to configure for a build which supports
WIN32) ? Any pointers ?
Even this, I will shift it to the 2nd patch, where I need to move
exec_get_cmd_path() out accross other file (migration.c).
great.
{
g_autofree char *detected_path = g_new(char, MAX_PATH);
@@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
}
#endif
-void exec_start_outgoing_migration(MigrationState *s, const char
*command, Error **errp)
+void exec_start_outgoing_migration(MigrationState *s, const char
*command,
+ Error **errp)
{
QIOChannel *ioc;
Sure, Juan. Will change this in the 2nd patch itself instead of
here. I am not very convinved why should we have a different patch all
together for this, because we are just using this code outside this
file in my opinion? But if you still think so, I can make a different
patch for that.
It is up to you.
For now, I will keep it with 2nd patch. If any other maintainer also
thinks that it should be a separate patch all together of windows
support for exec, I will make a new patch for that. But thankyou for
your suggestion 😁
Juan, I get your point. But I think, we won't be needing local_err at
all, if I use g_autoptr for 'channel' and 'saddr' is a part of
'channel'. Let me have a v2 patchset and if it is still not
convinving, we can have a discussion on this.
THis leaks 'channel', and free's 'saddr' which actually belongs
to channel.
With my comments on the previous patch suggesting g_autoptr for
'channel', we don't need any free calls for 'saddr' or 'channel'.
Right. With g_autoptr used for freeing 'channel' in last patch, we
wont have to worry about freeing 'saddr' at all. Thanks Daniel
if (local_err) {
qapi_free_SocketAddress(saddr);
error_propagate(errp, local_err);
return;
}
And after changing the position for assigning 'saddr' and using
g_autoptr for 'channel' I believe we can get rid of 'local_error'
variable too and replace it with 'errp'. Please suggest if I am
missing something here. TIA!
great. That is much better.
Ack.
-void socket_start_outgoing_migration(MigrationState *s,
- const char *str,
- Error **errp)
-{
- Error *err = NULL;
- SocketAddress *saddr = socket_parse(str, &err);
- if (!err) {
- socket_start_outgoing_migration_internal(s, saddr, &err);
- }
- error_propagate(errp, err);
-}
-
Actually Juan. I don't need this function at all, because parsing of
uri into socketAddress using socket_parse is already done. So there is
no use of having this function in the first place, so I decided to
delete this fucntion all together. Same with incoming function.
What I mean is that this code was already there. And it was doing it
wrong. The parts that I corrected you were using this pattern, chcking
that err was NULL, intsead of cheking that saddr is NULL.
Yes, I get your point. But that function is useless if socket_parse()
function is not needed. So have omitted the function all together as
socket parsing is already done in earlier patches.
Later, Juan.
Regards,
Het Gala
