[virt-tools-list] [PATCH virt-viewer 03/10] ovirt-foreign-menu: Fetch ISO names using GTask API
Eduardo Lima (Etrunko)
etrunko at redhat.com
Tue Jan 17 19:52:04 UTC 2017
On 17/01/17 13:57, Christophe Fergeau wrote:
> On Fri, Jan 13, 2017 at 07:11:05PM -0200, Eduardo Lima (Etrunko) wrote:
>> Similar to the previous commit, the ISO dialog will fetch the result
>> asynchronously, rather than relying on the "notify::files" signal from
>> OvirtForeignMenu object. It also enables error to be shown if anything
>> goes wrong.
>>
>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>> ---
>> src/ovirt-foreign-menu.c | 159 +++++++++++++++++++++++++----------------------
>> src/ovirt-foreign-menu.h | 9 ++-
>> src/remote-viewer.c | 45 ++++++++++++--
>> 3 files changed, 135 insertions(+), 78 deletions(-)
>>
>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
>> index 366259a..6892f0d 100644
>> --- a/src/ovirt-foreign-menu.c
>> +++ b/src/ovirt-foreign-menu.c
>> @@ -40,13 +40,13 @@ typedef enum {
>> STATE_ISOS
>> } OvirtForeignMenuState;
>>
>> -static void ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu, OvirtForeignMenuState state);
>> -static void ovirt_foreign_menu_fetch_api_async(OvirtForeignMenu *menu);
>> -static void ovirt_foreign_menu_fetch_vm_async(OvirtForeignMenu *menu);
>> -static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu);
>> -static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu);
>> -static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu *menu);
>> -static gboolean ovirt_foreign_menu_refresh_iso_list(gpointer user_data);
>> +static void ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu, GTask *task, OvirtForeignMenuState state);
>> +static void ovirt_foreign_menu_fetch_api_async(OvirtForeignMenu *menu, GTask *task);
>> +static void ovirt_foreign_menu_fetch_vm_async(OvirtForeignMenu *menu, GTask *task);
>> +static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu, GTask *task);
>> +static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu, GTask *task);
>> +static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu *menu, GTask *task);
>> +static void ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu, GTask *task);
>
> Wondering if it would make sense to pass the GTask around as an
> OvirtForeignMenu member?
>
Not really sure, I don't know if there can be more than one task running
at the same time, such as one for fetching and another for setting, so
in this case it would get overwritten.
>>
>> G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
>>
>> @@ -273,11 +273,9 @@ OvirtForeignMenu* ovirt_foreign_menu_new(OvirtProxy *proxy)
>>
>> static void
>> ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
>> + GTask *task,
>> OvirtForeignMenuState current_state)
>> {
>> - g_return_if_fail(current_state >= STATE_0);
>> - g_return_if_fail(current_state < STATE_ISOS);
>> -
>
> Why drop this?
At this point we are already in the async task and the error case will
be dealt in default below.
>
>> /* Each state will check if the member is initialized, falling directly to
>> * the next one if so. If not, the callback for the asynchronous call will
>> * be responsible for calling is function again with the next state as
>> @@ -286,26 +284,26 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
>> switch (current_state + 1) {
>> case STATE_API:
>> if (menu->priv->api == NULL) {
>> - ovirt_foreign_menu_fetch_api_async(menu);
>> + ovirt_foreign_menu_fetch_api_async(menu, task);
>> break;
>> }
>> case STATE_VM:
>> if (menu->priv->vm == NULL) {
>> - ovirt_foreign_menu_fetch_vm_async(menu);
>> + ovirt_foreign_menu_fetch_vm_async(menu, task);
>> break;
>> }
>> case STATE_STORAGE_DOMAIN:
>> if (menu->priv->files == NULL) {
>> - ovirt_foreign_menu_fetch_storage_domain_async(menu);
>> + ovirt_foreign_menu_fetch_storage_domain_async(menu, task);
>> break;
>> }
>> case STATE_VM_CDROM:
>> if (menu->priv->cdrom == NULL) {
>> - ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
>> + ovirt_foreign_menu_fetch_vm_cdrom_async(menu, task);
>> break;
>> }
>> case STATE_CDROM_FILE:
>> - ovirt_foreign_menu_refresh_cdrom_file_async(menu);
>> + ovirt_foreign_menu_refresh_cdrom_file_async(menu, task);
>> break;
>> case STATE_ISOS:
>> g_warn_if_fail(menu->priv->api != NULL);
>> @@ -313,18 +311,35 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
>> g_warn_if_fail(menu->priv->files != NULL);
>> g_warn_if_fail(menu->priv->cdrom != NULL);
>>
>> - ovirt_foreign_menu_refresh_iso_list(menu);
>> + ovirt_foreign_menu_fetch_iso_list_async(menu, task);
>> break;
>> default:
>> g_warn_if_reached();
>> + g_task_return_new_error(task, OVIRT_ERROR, OVIRT_ERROR_FAILED,
>> + "Invalid state: %d", current_state);
>> + g_object_unref(task);
>> }
>> }
>>
>>
>> void
>> -ovirt_foreign_menu_start(OvirtForeignMenu *menu)
>> +ovirt_foreign_menu_fetch_iso_names_async(OvirtForeignMenu *menu,
>> + GCancellable *cancellable,
>> + GAsyncReadyCallback callback,
>> + gpointer user_data)
>> {
>> - ovirt_foreign_menu_next_async_step(menu, STATE_0);
>> + GTask *task = g_task_new(menu, cancellable, callback, user_data);
>> + ovirt_foreign_menu_next_async_step(menu, task, STATE_0);
>> +}
>> +
>> +
>> +GList *
>> +ovirt_foreign_menu_fetch_iso_names_finish(OvirtForeignMenu *foreign_menu,
>> + GAsyncResult *result,
>> + GError **error)
>> +{
>> + g_return_val_if_fail(OVIRT_IS_FOREIGN_MENU(foreign_menu), NULL);
>> + return g_task_propagate_pointer(G_TASK(result), error);
>> }
>>
>>
>> @@ -545,7 +560,6 @@ static void ovirt_foreign_menu_set_files(OvirtForeignMenu *menu,
>>
>> g_list_free_full(menu->priv->iso_names, (GDestroyNotify)g_free);
>> menu->priv->iso_names = sorted_files;
>> - g_object_notify(G_OBJECT(menu), "files");
>> }
>>
>>
>> @@ -553,14 +567,16 @@ static void cdrom_file_refreshed_cb(GObject *source_object,
>> GAsyncResult *result,
>> gpointer user_data)
>> {
>> + GTask *task = G_TASK(user_data);
>> + OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(g_task_get_source_object(task));
>> OvirtResource *cdrom = OVIRT_RESOURCE(source_object);
>> - OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data);
>> GError *error = NULL;
>>
>> ovirt_resource_refresh_finish(cdrom, result, &error);
>> if (error != NULL) {
>> g_warning("failed to refresh cdrom content: %s", error->message);
>> - g_clear_error(&error);
>> + g_task_return_error(task, error);
>> + g_object_unref(task);
>> return;
>> }
>>
>> @@ -571,22 +587,22 @@ static void cdrom_file_refreshed_cb(GObject *source_object,
>> "file", &menu->priv->current_iso_name,
>> NULL);
>> }
>> - g_object_notify(G_OBJECT(menu), "file");
>
> Are you sure this should be removed in this patch?
>
>> if (menu->priv->cdrom != NULL) {
>> - ovirt_foreign_menu_next_async_step(menu, STATE_CDROM_FILE);
>> + ovirt_foreign_menu_next_async_step(menu, task, STATE_CDROM_FILE);
>> } else {
>> g_debug("Could not find VM cdrom through oVirt REST API");
>
> I think you need a g_task_return_error() here.
>
I only added g_task_return error() call to where there was already an
error set in the previous code, but you are right, this way we can get
to the exact point where the error happens.
>> }
>> }
>>
>>
>> -static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu *menu)
>> +static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu *menu,
>> + GTask *task)
>> {
>> g_return_if_fail(OVIRT_IS_RESOURCE(menu->priv->cdrom));
>>
>> ovirt_resource_refresh_async(OVIRT_RESOURCE(menu->priv->cdrom),
>> menu->priv->proxy, NULL,
>> - cdrom_file_refreshed_cb, menu);
>> + cdrom_file_refreshed_cb, task);
>> }
>>
>>
>> @@ -596,7 +612,8 @@ static void cdroms_fetched_cb(GObject *source_object,
>> {
>> GHashTable *cdroms;
>> OvirtCollection *cdrom_collection = OVIRT_COLLECTION(source_object);
>> - OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data);
>> + GTask *task = G_TASK(user_data);
>> + OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(g_task_get_source_object(task));
>> GHashTableIter iter;
>> OvirtCdrom *cdrom;
>> GError *error = NULL;
>> @@ -604,7 +621,8 @@ static void cdroms_fetched_cb(GObject *source_object,
>> ovirt_collection_fetch_finish(cdrom_collection, result, &error);
>> if (error != NULL) {
>> g_warning("failed to fetch cdrom collection: %s", error->message);
>> - g_clear_error(&error);
>> + g_task_return_error(task, error);
>> + g_object_unref(task);
>> return;
>> }
>>
>> @@ -626,20 +644,21 @@ static void cdroms_fetched_cb(GObject *source_object,
>> }
>>
>> if (menu->priv->cdrom != NULL) {
>> - ovirt_foreign_menu_next_async_step(menu, STATE_VM_CDROM);
>> + ovirt_foreign_menu_next_async_step(menu, task, STATE_VM_CDROM);
>> } else {
>> g_debug("Could not find VM cdrom through oVirt REST API");
>
> I think you need a g_task_return_error() here too.
>
>> }
>> }
>>
>>
>> -static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu)
>> +static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu,
>> + GTask *task)
>> {
>> OvirtCollection *cdrom_collection;
>>
>> cdrom_collection = ovirt_vm_get_cdroms(menu->priv->vm);
>> ovirt_collection_fetch_async(cdrom_collection, menu->priv->proxy, NULL,
>> - cdroms_fetched_cb, menu);
>> + cdroms_fetched_cb, task);
>> }
>>
>>
>> @@ -648,7 +667,8 @@ static void storage_domains_fetched_cb(GObject *source_object,
>> gpointer user_data)
>> {
>> GError *error = NULL;
>> - OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data);
>> + GTask *task = G_TASK(user_data);
>> + OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(g_task_get_source_object(task));
>> OvirtCollection *collection = OVIRT_COLLECTION(source_object);
>> GHashTableIter iter;
>> OvirtStorageDomain *domain;
>> @@ -656,7 +676,8 @@ static void storage_domains_fetched_cb(GObject *source_object,
>> ovirt_collection_fetch_finish(collection, result, &error);
>> if (error != NULL) {
>> g_warning("failed to fetch storage domains: %s", error->message);
>> - g_clear_error(&error);
>> + g_task_return_error(task, error);
>> + g_object_unref(task);
>> return;
>> }
>>
>> @@ -687,21 +708,21 @@ static void storage_domains_fetched_cb(GObject *source_object,
>> }
>>
>> if (menu->priv->files != NULL) {
>> - ovirt_foreign_menu_next_async_step(menu, STATE_STORAGE_DOMAIN);
>> + ovirt_foreign_menu_next_async_step(menu, task, STATE_STORAGE_DOMAIN);
>> } else {
>> g_debug("Could not find iso file collection");
>
> g_task_return_error()
>
>> }
>> }
>>
>>
>> -static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu)
>> +static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu,
>> + GTask *task)
>> {
>> - OvirtCollection *collection;
>> + OvirtCollection *collection = ovirt_api_get_storage_domains(menu->priv->api);
>>
>> g_debug("Start fetching oVirt REST collection");
>> - collection = ovirt_api_get_storage_domains(menu->priv->api);
>> ovirt_collection_fetch_async(collection, menu->priv->proxy, NULL,
>> - storage_domains_fetched_cb, menu);
>> + storage_domains_fetched_cb, task);
>> }
>>
>>
>> @@ -710,16 +731,17 @@ static void vms_fetched_cb(GObject *source_object,
>> gpointer user_data)
>> {
>> GError *error = NULL;
>> - OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data);
>> - OvirtCollection *collection;
>> + GTask *task = G_TASK(user_data);
>> + OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(g_task_get_source_object(task));
>> + OvirtCollection *collection = OVIRT_COLLECTION(source_object);
>> GHashTableIter iter;
>> OvirtVm *vm;
>>
>> - collection = OVIRT_COLLECTION(source_object);
>> ovirt_collection_fetch_finish(collection, result, &error);
>> if (error != NULL) {
>> g_debug("failed to fetch VM list: %s", error->message);
>> - g_clear_error(&error);
>> + g_task_return_error(task, error);
>> + g_object_unref(task);
>> return;
>> }
>>
>> @@ -736,14 +758,15 @@ static void vms_fetched_cb(GObject *source_object,
>> g_free(guid);
>> }
>> if (menu->priv->vm != NULL) {
>> - ovirt_foreign_menu_next_async_step(menu, STATE_VM);
>> + ovirt_foreign_menu_next_async_step(menu, task, STATE_VM);
>> } else {
>> g_warning("failed to find a VM with guid \"%s\"", menu->priv->vm_guid);
>
> g_task_return_error()
>
>
> Reviewed-by: Christophe Fergeau <cfergeau at redhat.com>
>
I will add the missing g_task_return_error() calls and send another
version soon.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com
More information about the virt-tools-list
mailing list