Re: [Qemu-devel] [PATCH v11 01/15] migration: Set error state in case of error

2018-04-06 Thread Juan Quintela 
Daniel P. Berrange  wrote:
> On Fri, Mar 16, 2018 at 05:49:07PM +, Daniel P. Berrangé wrote:
>> On Fri, Mar 16, 2018 at 12:53:49PM +0100, Juan Quintela wrote:
>> > Signed-off-by: Juan Quintela 
>> > ---
>> >  migration/ram.c | 20 
>> >  1 file changed, 20 insertions(+)
>> > 
>> > diff --git a/migration/ram.c b/migration/ram.c
>> > index 7266351fd0..1b8095a358 100644
>> > --- a/migration/ram.c
>> > +++ b/migration/ram.c
>> > @@ -414,6 +414,16 @@ static void terminate_multifd_send_threads(Error 
>> > *errp)
>> >  {
>> >  int i;
>> >  
>> > +if (errp) {
>> > +MigrationState *s = migrate_get_current();
>> > +migrate_set_error(s, errp);
>> 
>> This doesn't look quiet right. You're checking if 'errp' is a non-NULL,
>> which just tells you if the caller wants to collect the error, not
>> whether an error has happened. For the latter you need
>> 
>>   if (errp && *errp)
>> 
>> seems a little strange though for the caller to pass an error into this
>> method for reporting.
>
> Oh wait, I'm being mislead by the unusual parameter name.
>
> An "errp" name should only ever be used for a "Error **", but we
> only have an "Error *" here.

Copy & Paste O:-)

> So just fix the parameter name to be "err" instead of "errp".

Done.

Thanks,

Re: [Qemu-devel] [PATCH v11 01/15] migration: Set error state in case of error

2018-03-16 Thread Daniel P . Berrangé 
On Fri, Mar 16, 2018 at 05:49:07PM +, Daniel P. Berrangé wrote:
> On Fri, Mar 16, 2018 at 12:53:49PM +0100, Juan Quintela wrote:
> > Signed-off-by: Juan Quintela 
> > ---
> >  migration/ram.c | 20 
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 7266351fd0..1b8095a358 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -414,6 +414,16 @@ static void terminate_multifd_send_threads(Error *errp)
> >  {
> >  int i;
> >  
> > +if (errp) {
> > +MigrationState *s = migrate_get_current();
> > +migrate_set_error(s, errp);
> 
> This doesn't look quiet right. You're checking if 'errp' is a non-NULL,
> which just tells you if the caller wants to collect the error, not
> whether an error has happened. For the latter you need
> 
>   if (errp && *errp)
> 
> seems a little strange though for the caller to pass an error into this
> method for reporting.

Oh wait, I'm being mislead by the unusual parameter name.

An "errp" name should only ever be used for a "Error **", but we
only have an "Error *" here.

So just fix the parameter name to be "err" instead of "errp".


> > +if (s->state == MIGRATION_STATUS_SETUP ||
> > +s->state == MIGRATION_STATUS_ACTIVE) {
> > +migrate_set_state(>state, s->state,
> > +  MIGRATION_STATUS_FAILED);
> > +}
> > +}
> > +
> >  for (i = 0; i < multifd_send_state->count; i++) {
> >  MultiFDSendParams *p = _send_state->params[i];
> >  
> > @@ -514,6 +524,16 @@ static void terminate_multifd_recv_threads(Error *errp)

This parameter name needs fixing too.

These are actually a pre-existing problem in current GIT, so worth fixing
in a separate patch.

> >  {
> >  int i;
> >  
> > +if (errp) {
> > +MigrationState *s = migrate_get_current();
> > +migrate_set_error(s, errp);
> > +if (s->state == MIGRATION_STATUS_SETUP ||
> > +s->state == MIGRATION_STATUS_ACTIVE) {
> > +migrate_set_state(>state, s->state,
> > +  MIGRATION_STATUS_FAILED);
> > +}
> > +}
> > +
> >  for (i = 0; i < multifd_recv_state->count; i++) {
> >  MultiFDRecvParams *p = _recv_state->params[i];
> >  
> > -- 
> > 2.14.3
> > 
> > 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v11 01/15] migration: Set error state in case of error

2018-03-16 Thread Daniel P . Berrangé 
On Fri, Mar 16, 2018 at 12:53:49PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela 
> ---
>  migration/ram.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 7266351fd0..1b8095a358 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -414,6 +414,16 @@ static void terminate_multifd_send_threads(Error *errp)
>  {
>  int i;
>  
> +if (errp) {
> +MigrationState *s = migrate_get_current();
> +migrate_set_error(s, errp);

This doesn't look quiet right. You're checking if 'errp' is a non-NULL,
which just tells you if the caller wants to collect the error, not
whether an error has happened. For the latter you need

  if (errp && *errp)

seems a little strange though for the caller to pass an error into this
method for reporting.

> +if (s->state == MIGRATION_STATUS_SETUP ||
> +s->state == MIGRATION_STATUS_ACTIVE) {
> +migrate_set_state(>state, s->state,
> +  MIGRATION_STATUS_FAILED);
> +}
> +}
> +
>  for (i = 0; i < multifd_send_state->count; i++) {
>  MultiFDSendParams *p = _send_state->params[i];
>  
> @@ -514,6 +524,16 @@ static void terminate_multifd_recv_threads(Error *errp)
>  {
>  int i;
>  
> +if (errp) {
> +MigrationState *s = migrate_get_current();
> +migrate_set_error(s, errp);
> +if (s->state == MIGRATION_STATUS_SETUP ||
> +s->state == MIGRATION_STATUS_ACTIVE) {
> +migrate_set_state(>state, s->state,
> +  MIGRATION_STATUS_FAILED);
> +}
> +}
> +
>  for (i = 0; i < multifd_recv_state->count; i++) {
>  MultiFDRecvParams *p = _recv_state->params[i];
>  
> -- 
> 2.14.3
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH v11 01/15] migration: Set error state in case of error

2018-03-16 Thread Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/ram.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 7266351fd0..1b8095a358 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -414,6 +414,16 @@ static void terminate_multifd_send_threads(Error *errp)
 {
 int i;
 
+if (errp) {
+MigrationState *s = migrate_get_current();
+migrate_set_error(s, errp);
+if (s->state == MIGRATION_STATUS_SETUP ||
+s->state == MIGRATION_STATUS_ACTIVE) {
+migrate_set_state(>state, s->state,
+  MIGRATION_STATUS_FAILED);
+}
+}
+
 for (i = 0; i < multifd_send_state->count; i++) {
 MultiFDSendParams *p = _send_state->params[i];
 
@@ -514,6 +524,16 @@ static void terminate_multifd_recv_threads(Error *errp)
 {
 int i;
 
+if (errp) {
+MigrationState *s = migrate_get_current();
+migrate_set_error(s, errp);
+if (s->state == MIGRATION_STATUS_SETUP ||
+s->state == MIGRATION_STATUS_ACTIVE) {
+migrate_set_state(>state, s->state,
+  MIGRATION_STATUS_FAILED);
+}
+}
+
 for (i = 0; i < multifd_recv_state->count; i++) {
 MultiFDRecvParams *p = _recv_state->params[i];
 
-- 
2.14.3