[virt-tools-list] [PATCH virt-viewer] ovirt-foreign-menu: Factor out code to set file collection

[Eduardo Lima (Etrunko)]

On 8/26/19 4:15 AM, Victor Toso wrote:
> Hi,
> 
> On Fri, Aug 23, 2019 at 11:38:05AM -0300, Eduardo Lima (Etrunko) wrote:
>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>> ---
>>  src/ovirt-foreign-menu.c | 25 +++++++++++++++++--------
>>  1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
>> index c2f43e6..190bb3b 100644
>> --- a/src/ovirt-foreign-menu.c
>> +++ b/src/ovirt-foreign-menu.c
>> @@ -674,6 +674,18 @@ static gboolean storage_domain_validate(OvirtForeignMenu *menu G_GNUC_UNUSED,
>>      return ret;
>>  }
>>  
>> +static gboolean ovirt_foreign_menu_set_file_collection(OvirtForeignMenu *menu, OvirtCollection *file_collection)
>> +{
>> +    g_return_val_if_fail(file_collection != NULL, FALSE);
> 
> I think this is the only additional behavior, if
> ovirt_storage_domain_get_files() returns NULL, we will issue a
> critical warning.
> 
> If that's not intended, just return normally.

This was intended yes, do you think it needs to be silent?

> 
> IMHO, no need to send a v2,
> Acked-by: Victor Toso <victortoso at redhat.com>
> 
>> +
>> +    if (menu->priv->files) {
>> +        g_object_unref(G_OBJECT(menu->priv->files));
>> +    }
>> +    menu->priv->files = g_object_ref(G_OBJECT(file_collection));
>> +    g_debug("Set VM files to %p", menu->priv->files);
>> +    return TRUE;
>> +}
>> +
>>  static void storage_domains_fetched_cb(GObject *source_object,
>>                                         GAsyncResult *result,
>>                                         gpointer user_data)
>> @@ -705,14 +717,11 @@ static void storage_domains_fetched_cb(GObject *source_object,
>>              domain_valid = TRUE;
>>  
>>          file_collection = ovirt_storage_domain_get_files(domain);
>> -        if (file_collection != NULL) {
>> -            if (menu->priv->files) {
>> -                g_object_unref(G_OBJECT(menu->priv->files));
>> -            }
>> -            menu->priv->files = g_object_ref(G_OBJECT(file_collection));
>> -            g_debug("Set VM files to %p", menu->priv->files);
>> -            break;
>> -        }
>> +        if (!ovirt_foreign_menu_set_file_collection(menu, file_collection))
>> +            continue;
>> +
>> +        break; /* There can only be one valid storage domain at a time,
>> +                  no need to iterate more on the list */
>>      }
>>  
>>      if (menu->priv->files != NULL) {
>> -- 
>> 2.21.0
>>
>> _______________________________________________
>> virt-tools-list mailing list
>> virt-tools-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/virt-tools-list


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - Red Hat
etrunko at redhat.com