[virt-tools-list] [virt-viewer 2/7] ovirt: Add OvirtForeignMenu class
Marc-André Lureau
mlureau at redhat.com
Thu Apr 17 15:23:00 UTC 2014
----- Original Message -----
> On Thu, Apr 17, 2014 at 01:26:09PM +0200, Marc-André Lureau wrote:
> > On Wed, Apr 16, 2014 at 6:59 PM, Christophe Fergeau
> > <cfergeau at redhat.com>wrote:
> > > +
> > > +
> > > +static void
> > > +ovirt_foreign_menu_set_property(GObject *object, guint property_id,
> > > + const GValue *value
> > > G_GNUC_UNUSED,
> > > GParamSpec *pspec)
> > > +{
> > > + OvirtForeignMenu *self = OVIRT_FOREIGN_MENU(object);
> > > + OvirtForeignMenuPrivate *priv = self->priv;
> > > +
> > > + switch (property_id) {
> > > + case PROP_PROXY:
> > > + if (priv->proxy != NULL) {
> > > + g_object_unref(priv->proxy);
> > > + }
> > >
> >
> > newly written code can really benefit g_clear_object() / g_clear_pointer()
> > - true for all the remaining if { unref ; foo = NULL } all over.
>
> I'm not particularly fond of g_clear_object()/g_clear_pointer() and the not
> really needed atomic operations that come with them.
They make the code really more pleasant, and they are used in the rest of vv, so it would nice to follow that style.
>
> [snip]
> > > +
> > > +
> > > +static void
> > > +ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
> > > + OvirtForeignMenuState current_state)
> > > +{
> > > + current_state++;
> > >
> >
> > I would add a few g_return_if_fail() check about current_state values.
>
> You mean, check if it does not grow too small/too big?
>
yes
> >
> >
> > > +
> > > + if (current_state == STATE_STORAGE_DOMAIN) {
> > > + if (menu->priv->files == NULL) {
> > > + ovirt_foreign_menu_fetch_storage_domain_async(menu);
> > > + } else {
> > > + current_state++;
> > > + }
> > > + }
> > > +
> > > + if (current_state == STATE_VM_CDROM) {
> > > + if (menu->priv->cdrom == NULL) {
> > > + ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
> > > + } else {
> > > + current_state++;
> > > + }
> > > + }
> > > +
> > > + if (current_state == STATE_ISOS) {
> > > + g_warn_if_fail(menu->priv->api != NULL);
> > > + g_warn_if_fail(menu->priv->vm != NULL);
> > > + g_warn_if_fail(menu->priv->files != NULL);
> > > + g_warn_if_fail(menu->priv->cdrom != NULL);
> > > +
> > > + ovirt_foreign_menu_refresh_iso_list(menu);
> > > + }
> > > +}
> > > +
> > > +
> > > +void
> > > +ovirt_foreign_menu_start(OvirtForeignMenu *menu)
> > > +{
> > > + ovirt_foreign_menu_next_async_step(menu, STATE_0);
> > >
> >
> > Why not calling all the async simultaneously? If they need to be chained,
> > isn't it simpler to do it through the normal code flow?
>
> They need to be chained, gathering the chaining there make the order more
> obvious, make it easier to add more steps to the chain, I think that's why
> I went with this approach (I wrote that code a bit too long ago to remember
> all the details :( .
ok, I am not really fond of this state style though, but fine.
>
> >
> >
> > > +}
> > > +
> > > +
> > > +static void updated_cdrom_cb(GObject *source_object,
> > > + GAsyncResult *result,
> > > + G_GNUC_UNUSED gpointer user_data)
> > > +{
> > > + GError *error = NULL;
> > > + ovirt_cdrom_update_finish(OVIRT_CDROM(source_object),
> > > + result, &error);
> > > + g_debug("Finished updating cdrom content");
> > > + if (error != NULL) {
> > > + g_debug("Failed to update cdrom resource: %s", error->message);
> > >
> >
> > doesn't this deserve a warning? (same for similar errors)
>
> Fair enough, I've changed all _finish errors to be reported with a
> g_warning().
>
> > > +
> > > +static char *
> > > +ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu *foreign_menu)
> > > +{
> > > + char *name;
> > > +
> > > + if (foreign_menu->priv->cdrom == NULL) {
> > > + return NULL;
> > > + }
> > > +
> > > + g_object_get(G_OBJECT(foreign_menu->priv->cdrom), "file", &name,
> > > NULL);
> > >
> >
> > g_object_get()/set() don't need explicit cast.
>
> This gives us a warning if the object is corrupt for some reason. I've
> changed the various occurrences in this file.
>
> >
> >
> > > + return name;
> > > +}
> > > +
> > > +
> > > +GtkWidget *ovirt_foreign_menu_get_gtk_menu(OvirtForeignMenu
> > > *foreign_menu)
> > > +{
> > > + GtkWidget *gtk_menu;
> > > + GList *it;
> > > + char *current_iso;
> > > +
> > > + g_debug("Creating GtkMenu for foreign menu");
> > > + current_iso = ovirt_foreign_menu_get_current_iso_name(foreign_menu);
> > >
> >
> > Could be NULL,
>
> I think the code handles a NULL current_iso just fine.
>
> >
> >
> > > + gtk_menu = gtk_menu_new();
> > > + for (it = foreign_menu->priv->iso_names; it != NULL; it = it->next)
> > > {
> > > + GtkWidget *menuitem;
> > > +
> > > + menuitem = gtk_check_menu_item_new_with_label((char *)it->data);
> > > + if (g_strcmp0((char *)it->data, current_iso) == 0) {
> > >
> >
> > why not use safer and clearer g_str_equal() ? (same all over)
>
> I agree with more readable, but safer? g_str_equal does not handle NULL
> args, g_strcmp0 does.
You are right, sorry. So forget that.
>
> > > +static void iso_list_fetched_cb(GObject *source_object,
> > > + GAsyncResult *result,
> > > + gpointer user_data)
> > > +{
> > > + OvirtCollection *collection = OVIRT_COLLECTION(source_object);
> > > + GList *files;
> > > + GError *error = NULL;
> > > +
> > > + ovirt_collection_fetch_finish(collection, result, &error);
> > > + files =
> > > g_hash_table_get_values(ovirt_collection_get_resources(collection));
> > > + if (error != NULL) {
> > > + g_debug("failed to fetch files for ISO storage domain: %s",
> > > + error->message);
> > > + g_clear_error(&error);
> > > + return;
> > > + } else {
> > > + ovirt_foreign_menu_set_files(OVIRT_FOREIGN_MENU(user_data),
> > > files);
> > > + }
> > > + g_list_free(files);
> > > +
> > > + g_timeout_add_seconds(15, ovirt_foreign_menu_refresh_iso_list,
> > > user_data);
> > >
> >
> > Depending on the load of the server, this could be quite heavy. Is this the
> > recommended value by ovirt folks?
>
> I didn't ask them, I'll check. Hopefully at some point it will be possible
> to subscribe to change notifications rather than polling ;)
>
That's a fairly complicated topic for HTTP based protocols/servers, I am afraid.
> Thanks for the review,
>
> Christophe
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
More information about the virt-tools-list
mailing list