Preparation for new QEMU migration states

[libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted

Posted by Jiri Denemark 7 years, 6 months ago

When adding a new job state it's useful to let the compiler complain
about places where we need to think about what to do with the new
state.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 72edbb667..c3f9c38b2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
         return 0;
 
  error:
-    /* state can not be active or completed at this point */
-    if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
-        jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
+    switch (jobInfo->status) {
+    case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
+    case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
         /* The migration was aborted by us rather than QEMU itself. */
         jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
         return -2;
-    } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+
+    case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
+        /* Something failed after QEMU already finished the migration. */
         jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
         return -1;
-    } else {
+
+    case QEMU_DOMAIN_JOB_STATUS_FAILED:
+    case QEMU_DOMAIN_JOB_STATUS_CANCELED:
+        /* QEMU aborted the migration. */
         return -1;
+
+    case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+    case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
+    case QEMU_DOMAIN_JOB_STATUS_NONE:
+        /* Impossible. */
+        break;
     }
+
+    return -1;
 }
 
 
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted

Posted by Marc Hartmayer 7 years, 6 months ago

On Thu, Oct 19, 2017 at 03:56 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
> When adding a new job state it's useful to let the compiler complain
> about places where we need to think about what to do with the new
> state.
>
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 72edbb667..c3f9c38b2 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
>          return 0;
>
>   error:
> -    /* state can not be active or completed at this point */
> -    if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
> -        jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> +    switch (jobInfo->status) {
> +    case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
> +    case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
>          /* The migration was aborted by us rather than QEMU itself. */
>          jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
>          return -2;
> -    } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
> +
> +    case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
> +        /* Something failed after QEMU already finished the migration. */
>          jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
>          return -1;
> -    } else {
> +
> +    case QEMU_DOMAIN_JOB_STATUS_FAILED:
> +    case QEMU_DOMAIN_JOB_STATUS_CANCELED:
> +        /* QEMU aborted the migration. */
>          return -1;
> +
> +    case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
> +    case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
> +    case QEMU_DOMAIN_JOB_STATUS_NONE:
> +        /* Impossible. */
> +        break;
>      }
> +
> +    return -1;
>  }

I think you have to add ATTRIBUTE_FALLTHROUGH for the intended
fallthroughs (e.g. for gcc 7).

>
>
> --
> 2.14.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted

Posted by Jiri Denemark 7 years, 6 months ago

On Fri, Oct 20, 2017 at 11:28:37 +0200, Marc Hartmayer wrote:
> On Thu, Oct 19, 2017 at 03:56 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
> > When adding a new job state it's useful to let the compiler complain
> > about places where we need to think about what to do with the new
> > state.
> >
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 72edbb667..c3f9c38b2 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
> >          return 0;
> >
> >   error:
> > -    /* state can not be active or completed at this point */
> > -    if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
> > -        jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> > +    switch (jobInfo->status) {
> > +    case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
> > +    case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
> >          /* The migration was aborted by us rather than QEMU itself. */
> >          jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
> >          return -2;
> > -    } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
> > +
> > +    case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
> > +        /* Something failed after QEMU already finished the migration. */
> >          jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
> >          return -1;
> > -    } else {
> > +
> > +    case QEMU_DOMAIN_JOB_STATUS_FAILED:
> > +    case QEMU_DOMAIN_JOB_STATUS_CANCELED:
> > +        /* QEMU aborted the migration. */
> >          return -1;
> > +
> > +    case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
> > +    case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
> > +    case QEMU_DOMAIN_JOB_STATUS_NONE:
> > +        /* Impossible. */
> > +        break;
> >      }
> > +
> > +    return -1;
> >  }
> 
> I think you have to add ATTRIBUTE_FALLTHROUGH for the intended
> fallthroughs (e.g. for gcc 7).

This should only be needed if there was any code between the cases.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted

Posted by Marc Hartmayer 7 years, 6 months ago

On Fri, Oct 20, 2017 at 12:12 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
> On Fri, Oct 20, 2017 at 11:28:37 +0200, Marc Hartmayer wrote:
>> On Thu, Oct 19, 2017 at 03:56 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
>> > When adding a new job state it's useful to let the compiler complain
>> > about places where we need to think about what to do with the new
>> > state.
>> >
>> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>> > ---
>> >  src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
>> >  1 file changed, 18 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> > index 72edbb667..c3f9c38b2 100644
>> > --- a/src/qemu/qemu_migration.c
>> > +++ b/src/qemu/qemu_migration.c
>> > @@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
>> >          return 0;
>> >
>> >   error:
>> > -    /* state can not be active or completed at this point */
>> > -    if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
>> > -        jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
>> > +    switch (jobInfo->status) {
>> > +    case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
>> > +    case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
>> >          /* The migration was aborted by us rather than QEMU itself. */
>> >          jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
>> >          return -2;
>> > -    } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
>> > +
>> > +    case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
>> > +        /* Something failed after QEMU already finished the migration. */
>> >          jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
>> >          return -1;
>> > -    } else {
>> > +
>> > +    case QEMU_DOMAIN_JOB_STATUS_FAILED:
>> > +    case QEMU_DOMAIN_JOB_STATUS_CANCELED:
>> > +        /* QEMU aborted the migration. */
>> >          return -1;
>> > +
>> > +    case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
>> > +    case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
>> > +    case QEMU_DOMAIN_JOB_STATUS_NONE:
>> > +        /* Impossible. */
>> > +        break;
>> >      }
>> > +
>> > +    return -1;
>> >  }
>> 
>> I think you have to add ATTRIBUTE_FALLTHROUGH for the intended
>> fallthroughs (e.g. for gcc 7).
>
> This should only be needed if there was any code between the cases.

Thanks for the information.

>
> Jirka
>
-- 
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted
[libvirt] [PATCH 2/7] qemu: Refactor qemuMigrationRun a bit
[libvirt] [PATCH 3/7] qemu: Split cleanup and error code in qemuMigrationRun
[libvirt] [PATCH 4/7] qemu: Unite error handling in qemuMigrationRun
[libvirt] [PATCH 5/7] qemu: Don't misuse "ret" in qemuMigrationRun
[libvirt] [PATCH 6/7] qemu: Consistently use exit_monitor in qemuMigrationRun
[libvirt] [PATCH 7/7] qemu: Set correct job status when qemuMigrationRun fails