[Libvirt-cim] [PATCH 1 of 2] Add calls into VSMigrationService to utilize new MigrationIndication provider

[Jay Gagnon]

Dan Smith wrote:
> However, as Jay points out, I think that the indication should be
> typed per-platform as usual.  So, should we type the job to match,
> since we'll need to persist the information for typing the indication
> as well?
>
>   
I'm no Heidi, but it seems like we should be prefixing this.  We prefix 
every other class we create, and if somebody doesn't want to deal with 
the Xen/KVM distinction they are welcome to subscribe to/filter 
on/whatever they want the CIM super class and get both, right?

As to where the prefix comes from, I think Kaitlin was on to something 
with the ref that comes in from migrate_do, although I think it can be 
even easier than she said.  We don't actually need to attach the prefix 
to the migration_job structure; we're already attaching the classname, 
so all we need is a call to class_prefix_name and we've got a prefix.
>
> JG> @@ -297,12 +319,50 @@ static void migrate_job_set_state(struct
> JG>          CMSetProperty(inst, "Status",
> JG>                        (CMPIValue *)status, CMPI_chars);
>
> JG> +        CU_DEBUG("Creating indication.");
> JG> +        /* Prefix needs to be dynamic */
> JG> +        ind = get_typed_instance(_BROKER,
> JG> +                                 "Xen",
> JG> +                                 "ComputerSystemMigrationIndication",
> JG> +                                 job->ref_ns);
> JG> +        /* Prefix needs to be dynamic */
> JG> +        if (ind == NULL) {
> JG> +                CU_DEBUG("Failed to create ind, type '%s:%s_%s'", 
> JG> +                         job->ref_ns,
> JG> +                         "Xen",
> JG> +                         "ComputerSystemMigrationIndication");
> JG> +        }
> JG> +
> JG> +        /* Need to copy job inst before attaching as PreviousInstance because 
> JG> +           otherwise the changes we are about to make to job inst are made 
> JG> +           to PreviousInstance as well. */
> JG> +        s = cu_dup_instance(_BROKER, inst, &prev_inst);
>
> I didn't quite understand from the other patch, but I don't think this
> is what you want to do.  You create an instance with a set of
> properties set (like CCN) with the get_typed_instance() call, but then
> just overwrite the ObjectPath of it here.
>
> I think the semantics of the cu_dup_instance() call should be that it
> creates you a new instance.  Something like this maybe:
>
>   new_inst = cu_dup_instance(_BROKER, src_inst, &s);
>   
As was already brought to light by my first run at cu_dup_instance, I 
get easily turned around when I start working with actual instances and 
such, so I'd like to make sure I follow this.  Let's say I fix up 
cu_dup_instance so that it creates a new instance using the existing 
instances ObjectPath, then copies all the properties correctly.  Then I 
wouldn't have to call get_typed_instance in the first place, because 
everything that it does is already taken care of, right?
> JG> +        if (s.rc != CMPI_RC_OK || prev_inst == NULL) {
> JG> +                CU_DEBUG("dup_instance failed (%i:%s)", s.rc, s.msg);
> JG> +                return;
> JG> +        }
> JG> +
> JG> +        CU_DEBUG("Setting PreviousInstance");
> JG> +        CMSetProperty(ind, "PreviousInstance", 
> JG> +                      (CMPIValue *)&prev_inst, CMPI_instance);
> JG> +
> JG>          CU_DEBUG("Modifying job %s (%i:%s)", job->uuid, state, status);
>
> JG>          s = CBModifyInstance(_BROKER, job->context, op, inst, NULL);
> JG>          if (s.rc != CMPI_RC_OK)
> JG>                  CU_DEBUG("Failed to update job instance: %s",
> JG>                           CMGetCharPtr(s.msg));
> JG> +
> JG> +        CU_DEBUG("Setting SourceInstance");
> JG> +        CMSetProperty(ind, "SourceInstance",
> JG> +                      (CMPIValue *)&inst, CMPI_instance);
> JG> +
> JG> +        rc = raise_indication(job->context,
> JG> +                              "ComputerSystemMigrationIndication",
> JG> +                              job->ref_ns,
> JG> +                              ind);
> JG> +        if (!rc)
> JG> +                CU_DEBUG("Failed to raise indication");
> JG>  }
>
> These changes make the migrate_job_set_state() function quite a bit
> longer.  Can we break out these out into a separate function?  If it
> just returns an indication instance, then maybe something like this
> could work:
>
>   ind = prepare_indication(job_inst);
>
>   /* Existing job_inst update code */
>
>   raise_indication(ind, job_inst);
>
> The prepare call would dup the old instance and create the indication
> instance, and the raise call would add the updated version and do the
> actual raise.  What do you think?
>   
Sure that's fine.  I didn't think too much about code structure when I 
was in "wait is that gonna work?" mode; should have cleaned up a little 
more before I sent it out.


-- 

-Jay