[Qemu-devel] [PATCH 0/3] gtk: add Devices menu

[Qemu-devel] [PATCH 0/3] gtk: add Devices menu

[Anthony Liguori]

This small series adds a devices menu to the GTK UI allowing
a user to change or eject a removable block device from the menus.

[Qemu-devel] [PATCH 1/3] ide: add drive-id property

[Anthony Liguori]

This returns a string similar to what the guest would display in
something like Linux's /dev/disk/by-id/ path.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/ide/qdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 8a9a891..94b1664 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -270,6 +270,20 @@ static const TypeInfo ide_drive_info = {
     .class_init    = ide_drive_class_init,
 };
 
+static char *ide_device_get_model(Object *obj, Error **errp)
+{
+    IDEDevice *dev = IDE_DEVICE(obj);
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
+    IDEState *s = bus->ifs + dev->unit;
+
+    return g_strdup_printf("%s %s", s->drive_model_str, s->drive_serial_str);
+}
+
+static void ide_device_initfn(Object *obj)
+{
+    object_property_add_str(obj, "drive-id", ide_device_get_model, NULL, NULL);
+}
+
 static void ide_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
@@ -285,6 +299,7 @@ static const TypeInfo ide_device_type_info = {
     .abstract = true,
     .class_size = sizeof(IDEDeviceClass),
     .class_init = ide_device_class_init,
+    .instance_init = ide_device_initfn,
 };
 
 static void ide_register_types(void)
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1/3] ide: add drive-id property

[Andreas Färber]

Am 26.04.2013 21:43, schrieb Anthony Liguori:
> This returns a string similar to what the guest would display in
> something like Linux's /dev/disk/by-id/ path.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/ide/qdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 8a9a891..94b1664 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -270,6 +270,20 @@ static const TypeInfo ide_drive_info = {
>      .class_init    = ide_drive_class_init,
>  };
>  
> +static char *ide_device_get_model(Object *obj, Error **errp)
> +{
> +    IDEDevice *dev = IDE_DEVICE(obj);
> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);

IDEBus *bus = IDE_BUS(qdev_get_parent_bus(DEVICE(obj)));

You're breaking your own rules. :)

> +    IDEState *s = bus->ifs + dev->unit;
> +
> +    return g_strdup_printf("%s %s", s->drive_model_str, s->drive_serial_str);
> +}
> +
> +static void ide_device_initfn(Object *obj)
> +{
> +    object_property_add_str(obj, "drive-id", ide_device_get_model, NULL, NULL);
> +}
> +
>  static void ide_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
> @@ -285,6 +299,7 @@ static const TypeInfo ide_device_type_info = {
>      .abstract = true,
>      .class_size = sizeof(IDEDeviceClass),
>      .class_init = ide_device_class_init,
> +    .instance_init = ide_device_initfn,

You recently said ..._initfn was only a workaround for avoiding ..._init
vs. ..._initfn name conflicts. I've therefore started changing new code
to that pattern. There is no conflicting ide_device_init here.

I don't really care which suffix we choose, but consistency is good.

>  };
>  
>  static void ide_register_types(void)

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] ide: add drive-id property

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 26.04.2013 21:43, schrieb Anthony Liguori:
>> This returns a string similar to what the guest would display in
>> something like Linux's /dev/disk/by-id/ path.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/ide/qdev.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>> 
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index 8a9a891..94b1664 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -270,6 +270,20 @@ static const TypeInfo ide_drive_info = {
>>      .class_init    = ide_drive_class_init,
>>  };
>>  
>> +static char *ide_device_get_model(Object *obj, Error **errp)
>> +{
>> +    IDEDevice *dev = IDE_DEVICE(obj);
>> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
>
> IDEBus *bus = IDE_BUS(qdev_get_parent_bus(DEVICE(obj)));
>
> You're breaking your own rules. :)

Yeah, I copy/pasted, I'll clean up.

>
>> +    IDEState *s = bus->ifs + dev->unit;
>> +
>> +    return g_strdup_printf("%s %s", s->drive_model_str, s->drive_serial_str);
>> +}
>> +
>> +static void ide_device_initfn(Object *obj)
>> +{
>> +    object_property_add_str(obj, "drive-id", ide_device_get_model, NULL, NULL);
>> +}
>> +
>>  static void ide_device_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *k = DEVICE_CLASS(klass);
>> @@ -285,6 +299,7 @@ static const TypeInfo ide_device_type_info = {
>>      .abstract = true,
>>      .class_size = sizeof(IDEDeviceClass),
>>      .class_init = ide_device_class_init,
>> +    .instance_init = ide_device_initfn,
>
> You recently said ..._initfn was only a workaround for avoiding ..._init
> vs. ..._initfn name conflicts. I've therefore started changing new code
> to that pattern. There is no conflicting ide_device_init here.
>
> I don't really care which suffix we choose, but consistency is good.

I'm happy with using initfn consistently but I really don't care that
much.

Regards,

Anthony Liguori

>
>>  };
>>  
>>  static void ide_register_types(void)
>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 2/3] monitor: add notifier list for monitor events

[Anthony Liguori]

This lets us register for events internally within QEMU.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 include/qapi/qmp/qevents.h | 21 +++++++++++++++++++++
 monitor.c                  | 15 +++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 include/qapi/qmp/qevents.h

diff --git a/include/qapi/qmp/qevents.h b/include/qapi/qmp/qevents.h
new file mode 100644
index 0000000..2a91fd0
--- /dev/null
+++ b/include/qapi/qmp/qevents.h
@@ -0,0 +1,21 @@
+/*
+ * QEvent Support
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QMP_EVENTS_H
+#define QMP_EVENTS_H
+
+#include "qemu/notify.h"
+
+void qmp_add_event_notifier(Notifier *notifier);
+void qmp_del_event_notifier(Notifier *notifier);
+
+#endif
diff --git a/monitor.c b/monitor.c
index 332abe7..5168ea5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -56,6 +56,7 @@
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/json-parser.h"
+#include "qapi/qmp/qevents.h"
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "trace.h"
@@ -502,6 +503,19 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 MonitorEventState monitor_event_state[QEVENT_MAX];
 QemuMutex monitor_event_state_lock;
 
+static NotifierList qmp_event_notifier_list =
+    NOTIFIER_LIST_INITIALIZER(qmp_event_notifier_list);
+
+void qmp_add_event_notifier(Notifier *notifier)
+{
+    notifier_list_add(&qmp_event_notifier_list, notifier);
+}
+
+void qmp_del_event_notifier(Notifier *notifier)
+{
+    notifier_remove(notifier);
+}
+
 /*
  * Emits the event to every monitor instance
  */
@@ -512,6 +526,7 @@ monitor_protocol_event_emit(MonitorEvent event,
     Monitor *mon;
 
     trace_monitor_protocol_event_emit(event, data);
+    notifier_list_notify(&qmp_event_notifier_list, data);
     QLIST_FOREACH(mon, &mon_list, entry) {
         if (monitor_ctrl_mode(mon) && qmp_cmd_mode(mon)) {
             monitor_json_emitter(mon, data);
-- 
1.8.0

[Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

To generate this menu, we first walk the composition tree to
find any device with a 'drive' property.  We then record these
devices and the BlockDriverState that they are associated with.

Then we use query-block to get the BDS state for each of the
discovered devices.

This code doesn't handle hot-plug yet but it should deal nicely
with someone using the human monitor.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 ui/gtk.c | 302 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 302 insertions(+)

diff --git a/ui/gtk.c b/ui/gtk.c
index e12f228..c420914 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -64,6 +64,7 @@
 #include "x_keymap.h"
 #include "keymaps.h"
 #include "sysemu/char.h"
+#include "qapi/qmp/qevents.h"
 
 //#define DEBUG_GTK
 
@@ -139,6 +140,10 @@ typedef struct GtkDisplayState
     GtkWidget *grab_on_hover_item;
     GtkWidget *vga_item;
 
+    GtkWidget *devices_menu_item;
+    GtkWidget *devices_menu;
+    GHashTable *devices_map;
+
     int nb_vcs;
     VirtualConsole vc[MAX_VCS];
 
@@ -165,6 +170,8 @@ typedef struct GtkDisplayState
     bool external_pause_update;
 
     bool modifier_pressed[ARRAY_SIZE(modifier_keycode)];
+
+    Notifier event_notifier;
 } GtkDisplayState;
 
 static GtkDisplayState *global_state;
@@ -1382,6 +1389,296 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState *s, GtkAccelGroup *accel_g
     return view_menu;
 }
 
+typedef void (DeviceIterFunc)(const char *path, const char *property, void *opaque);
+
+static void device_foreach_proptype(const char *path, const char *proptype,
+                                    DeviceIterFunc *fn, void *opaque)
+{
+    ObjectPropertyInfoList *prop_list, *iter;
+
+    prop_list = qmp_qom_list(path, NULL);
+    for (iter = prop_list; iter; iter = iter->next) {
+        ObjectPropertyInfo *prop = iter->value;
+
+        if (strstart(prop->type, "child<", NULL)) {
+            char *subpath;
+            subpath = g_strdup_printf("%s/%s", path, prop->name);
+            device_foreach_proptype(subpath, proptype, fn, opaque);
+            g_free(subpath);
+        } else if (strcmp(prop->type, proptype) == 0) {
+            fn(path, prop->name, opaque);
+        }
+    }
+    qapi_free_ObjectPropertyInfoList(prop_list);
+}
+
+typedef enum DiskType
+{
+    DT_NORMAL,
+    DT_CDROM,
+    DT_FLOPPY,
+} DiskType;
+
+typedef struct BlockDeviceMenu
+{
+    GtkWidget *entry;
+    GtkWidget *submenu;
+    GtkWidget *eject;
+    GtkWidget *change;
+
+    char *name;
+    char *path;
+    char *desc;
+    DiskType disk_type;
+} BlockDeviceMenu;
+
+static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo *info)
+{
+    bool value;
+    const char *label = _("<No media>");
+
+    value = info->has_inserted && !info->locked;
+    gtk_widget_set_sensitive(bdm->eject, value);
+
+    value = !info->locked;
+    gtk_widget_set_sensitive(bdm->change, value);
+
+    if (info->has_inserted) {
+        label = info->inserted->file;
+        if (strlen(label) > 32) {
+            char *new_label;
+
+            new_label = strrchr(label, '/');
+            if (new_label) {
+                label = new_label + 1;
+            }
+        }
+    }
+
+    gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->change), label);
+}
+
+static void gd_update_block_menus(GtkDisplayState *s)
+{
+    BlockInfoList *block_list, *iter;
+
+    block_list = qmp_query_block(NULL);
+    for (iter = block_list; iter; iter = iter->next) {
+        BlockInfo *info = iter->value;
+        BlockDeviceMenu *bdm;
+
+        if (!info->removable) {
+            continue;
+        }
+
+        bdm = g_hash_table_lookup(s->devices_map, info->device);
+        if (!bdm) {
+            continue;
+        }
+
+        gd_block_device_menu_update(bdm, info);
+    }
+    qapi_free_BlockInfoList(block_list);
+}
+
+static void gd_enum_disk(const char *path, const char *proptype, void *opaque)
+{
+    GtkDisplayState *s = opaque;
+    Object *obj;
+    char *block_id;
+
+    obj = object_resolve_path(path, NULL);
+    g_assert(obj != NULL);
+
+    block_id = object_property_get_str(obj, proptype, NULL);
+    if (strcmp(block_id, "") != 0) {
+        BlockDeviceMenu *bdm;
+        DiskType disk_type;
+        char *type;
+        char *desc = NULL;
+
+        type = object_property_get_str(obj, "type", NULL);
+
+        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
+            desc = object_property_get_str(obj, "drive-id", NULL);
+        } else {
+            desc = g_strdup(type);
+        }
+
+        if (strcmp(type, "ide-cd") == 0) {
+            disk_type = DT_CDROM;
+        } else if (strcmp(type, "isa-fdc") == 0) {
+            disk_type = DT_FLOPPY;
+        } else {
+            disk_type = DT_NORMAL;
+        }
+
+        bdm = g_malloc0(sizeof(*bdm));
+        bdm->name = g_strdup(block_id);
+        bdm->path = g_strdup(path);
+        bdm->desc = desc;
+        bdm->disk_type = disk_type;
+
+        g_free(type);
+
+        g_hash_table_insert(s->devices_map, bdm->name, bdm);
+    }
+    g_free(block_id);
+}
+
+static void gd_error(GtkDisplayState *s, const char *fmt, ...)
+{
+    GtkWidget *dialog;
+    char *message;
+    va_list ap;
+
+    va_start(ap, fmt);
+    message = g_strdup_vprintf(fmt, ap);
+    va_end(ap);
+
+    dialog = gtk_message_dialog_new(GTK_WINDOW(s->window), GTK_DIALOG_DESTROY_WITH_PARENT,
+                                    GTK_MESSAGE_ERROR, GTK_BUTTONS_OK,
+                                    "%s", message);
+
+    g_free(message);
+
+    gtk_widget_show_all(dialog);
+
+    g_signal_connect_swapped(dialog, "response", G_CALLBACK(gtk_widget_destroy), dialog);
+}
+
+static void gd_menu_bdm_change_response(GtkDialog *dialog, gint response_id,
+                                        gpointer opaque)
+{
+    BlockDeviceMenu *bdm = opaque;
+
+    if (response_id == GTK_RESPONSE_ACCEPT) {
+        char *filename;
+        Error *local_err = NULL;
+
+        filename = gtk_file_chooser_get_filename(GTK_FILE_CHOOSER(dialog));
+
+        qmp_change(bdm->name, filename, false, NULL, &local_err);
+        if (local_err) {
+            gd_error(global_state,
+                     _("Block device change failed: %s"),
+                     error_get_pretty(local_err));
+            error_free(local_err);
+        }
+
+        g_free(filename);
+    }
+
+    gtk_widget_destroy(GTK_WIDGET(dialog));
+}
+
+static void gd_menu_bdm_change(GtkMenuItem *item, void *opaque)
+{
+    GtkDisplayState *s = global_state;
+    BlockDeviceMenu *bdm = opaque;
+    GtkWidget *chooser;
+
+    chooser = gtk_file_chooser_dialog_new(_("Choose Image File"),
+                                          GTK_WINDOW(s->window),
+                                          GTK_FILE_CHOOSER_ACTION_OPEN,
+                                          GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
+                                          GTK_STOCK_OPEN, GTK_RESPONSE_ACCEPT,
+                                          NULL);
+
+    g_signal_connect(chooser, "response",
+                     G_CALLBACK(gd_menu_bdm_change_response), bdm);
+
+    gtk_widget_show_all(chooser);
+}
+
+static void gd_menu_bdm_eject(GtkMenuItem *item, void *opaque)
+{
+    BlockDeviceMenu *bdm = opaque;
+
+    qmp_eject(bdm->name, false, false, NULL);
+}
+
+static void gd_event_notify(Notifier *notifier, void *data)
+{
+    QDict *event = qobject_to_qdict(data);
+    const char *event_str;
+
+    event_str = qdict_get_str(event, "event");
+    if (strcmp(event_str, "DEVICE_TRAY_MOVED") == 0) {
+        gd_update_block_menus(global_state);
+    }
+}
+
+static GtkWidget *gd_create_menu_devices(GtkDisplayState *s, GtkAccelGroup *accel_group)
+{
+    GtkWidget *devices_menu;
+    BlockInfoList *block_list, *iter;
+
+    s->event_notifier.notify = gd_event_notify;
+    qmp_add_event_notifier(&s->event_notifier);
+    s->devices_map = g_hash_table_new(g_str_hash, g_str_equal);
+
+    /* We first search for devices that has a drive property. */
+    device_foreach_proptype("/", "drive", gd_enum_disk, s);
+
+    devices_menu = gtk_menu_new();
+    gtk_menu_set_accel_group(GTK_MENU(devices_menu), accel_group);
+
+    block_list = qmp_query_block(NULL);
+    for (iter = block_list; iter; iter = iter->next) {
+        BlockInfo *info = iter->value;
+        BlockDeviceMenu *bdm;
+        const char *stock_id = NULL;
+
+        /* Only show removable devices */
+        if (!info->removable) {
+            continue;
+        }
+
+        /* Only show devices were we could identify the actual device */
+        bdm = g_hash_table_lookup(s->devices_map, info->device);
+        if (!bdm) {
+            continue;
+        }
+
+        bdm->submenu = gtk_menu_new();
+        bdm->change = gtk_image_menu_item_new_from_stock(GTK_STOCK_OPEN, NULL);
+        gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->change), _("<No media>"));
+        bdm->eject = gtk_image_menu_item_new_from_stock(GTK_STOCK_CLOSE, NULL);
+        gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->eject), _("Eject"));
+        gtk_menu_shell_append(GTK_MENU_SHELL(bdm->submenu), bdm->change);
+        gtk_menu_shell_append(GTK_MENU_SHELL(bdm->submenu), bdm->eject);
+
+        g_signal_connect(bdm->change, "activate",
+                         G_CALLBACK(gd_menu_bdm_change), bdm);
+        g_signal_connect(bdm->eject, "activate",
+                         G_CALLBACK(gd_menu_bdm_eject), bdm);
+
+        switch (bdm->disk_type) {
+        case DT_NORMAL:
+            stock_id = GTK_STOCK_HARDDISK;
+            break;
+        case DT_CDROM:
+            stock_id = GTK_STOCK_CDROM;
+            break;
+        case DT_FLOPPY:
+            stock_id = GTK_STOCK_FLOPPY;
+            break;
+        }
+
+        bdm->entry = gtk_image_menu_item_new_from_stock(stock_id, NULL);
+        gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->entry), bdm->desc);
+        gtk_menu_item_set_submenu(GTK_MENU_ITEM(bdm->entry), bdm->submenu);
+        gtk_menu_shell_append(GTK_MENU_SHELL(devices_menu), bdm->entry);
+
+        gd_block_device_menu_update(bdm, info);
+    }
+
+    qapi_free_BlockInfoList(block_list);
+
+    return devices_menu;
+}
+
 static void gd_create_menus(GtkDisplayState *s)
 {
     GtkAccelGroup *accel_group;
@@ -1389,6 +1686,7 @@ static void gd_create_menus(GtkDisplayState *s)
     accel_group = gtk_accel_group_new();
     s->machine_menu = gd_create_menu_machine(s, accel_group);
     s->view_menu = gd_create_menu_view(s, accel_group);
+    s->devices_menu = gd_create_menu_devices(s, accel_group);
 
     s->machine_menu_item = gtk_menu_item_new_with_mnemonic(_("_Machine"));
     gtk_menu_item_set_submenu(GTK_MENU_ITEM(s->machine_menu_item),
@@ -1399,6 +1697,10 @@ static void gd_create_menus(GtkDisplayState *s)
     gtk_menu_item_set_submenu(GTK_MENU_ITEM(s->view_menu_item), s->view_menu);
     gtk_menu_shell_append(GTK_MENU_SHELL(s->menu_bar), s->view_menu_item);
 
+    s->devices_menu_item = gtk_menu_item_new_with_mnemonic(_("_Devices"));
+    gtk_menu_item_set_submenu(GTK_MENU_ITEM(s->devices_menu_item), s->devices_menu);
+    gtk_menu_shell_append(GTK_MENU_SHELL(s->menu_bar), s->devices_menu_item);
+
     g_object_set_data(G_OBJECT(s->window), "accel_group", accel_group);
     gtk_window_add_accel_group(GTK_WINDOW(s->window), accel_group);
     s->accel_group = accel_group;
-- 
1.8.0

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Kevin Wolf]

Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
> To generate this menu, we first walk the composition tree to
> find any device with a 'drive' property.  We then record these
> devices and the BlockDriverState that they are associated with.
> 
> Then we use query-block to get the BDS state for each of the
> discovered devices.
> 
> This code doesn't handle hot-plug yet but it should deal nicely
> with someone using the human monitor.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

I haven't checked what causes it, but with this patch applied I get a
screenful of GTK error messages when I exit qemu with Alt-F4.

> +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo *info)
> +{
> +    bool value;
> +    const char *label = _("<No media>");
> +
> +    value = info->has_inserted && !info->locked;

Shouldn't the actual value of info->inserted play a role as well?

> +    gtk_widget_set_sensitive(bdm->eject, value);
> +
> +    value = !info->locked;
> +    gtk_widget_set_sensitive(bdm->change, value);
> +
> +    if (info->has_inserted) {
> +        label = info->inserted->file;
> +        if (strlen(label) > 32) {
> +            char *new_label;
> +
> +            new_label = strrchr(label, '/');
> +            if (new_label) {
> +                label = new_label + 1;
> +            }
> +        }
> +    }
> +
> +    gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->change), label);
> +}

> +static void gd_enum_disk(const char *path, const char *proptype, void *opaque)
> +{
> +    GtkDisplayState *s = opaque;
> +    Object *obj;
> +    char *block_id;
> +
> +    obj = object_resolve_path(path, NULL);
> +    g_assert(obj != NULL);
> +
> +    block_id = object_property_get_str(obj, proptype, NULL);
> +    if (strcmp(block_id, "") != 0) {
> +        BlockDeviceMenu *bdm;
> +        DiskType disk_type;
> +        char *type;
> +        char *desc = NULL;
> +
> +        type = object_property_get_str(obj, "type", NULL);
> +
> +        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
> +            desc = object_property_get_str(obj, "drive-id", NULL);
> +        } else {
> +            desc = g_strdup(type);
> +        }

Ugh. Comparing the device name to an incomplete set of strings here and
then figuring out for each what the specific way for this device is to
create a nice string sounds like a bad idea.

Why can't all devices just expose a property with a human-readable
string? We'll need it for more than just the disk change menus.

And then, of course, the question is still what a good human-readable
string is. A serial number generated by qemu, as we now get by default
for the CD-ROM, probably isn't. Something like "ATAPI CD-ROM at secondary
master" would probably be more helpful in this case.

> +
> +        if (strcmp(type, "ide-cd") == 0) {
> +            disk_type = DT_CDROM;
> +        } else if (strcmp(type, "isa-fdc") == 0) {
> +            disk_type = DT_FLOPPY;
> +        } else {
> +            disk_type = DT_NORMAL;
> +        }

Same thing here, comparing against strings is a hack. Devices should
probably have a property that says what kind of device they are.

> +        bdm = g_malloc0(sizeof(*bdm));
> +        bdm->name = g_strdup(block_id);
> +        bdm->path = g_strdup(path);
> +        bdm->desc = desc;
> +        bdm->disk_type = disk_type;
> +
> +        g_free(type);
> +
> +        g_hash_table_insert(s->devices_map, bdm->name, bdm);
> +    }
> +    g_free(block_id);
> +}

Kevin

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
>> To generate this menu, we first walk the composition tree to
>> find any device with a 'drive' property.  We then record these
>> devices and the BlockDriverState that they are associated with.
>> 
>> Then we use query-block to get the BDS state for each of the
>> discovered devices.
>> 
>> This code doesn't handle hot-plug yet but it should deal nicely
>> with someone using the human monitor.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> I haven't checked what causes it, but with this patch applied I get a
> screenful of GTK error messages when I exit qemu with Alt-F4.

I think I know what this is.  I assume we're getting an event after the
window is no longer realized.

>> +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo *info)
>> +{
>> +    bool value;
>> +    const char *label = _("<No media>");
>> +
>> +    value = info->has_inserted && !info->locked;
>
> Shouldn't the actual value of info->inserted play a role as well?

inserted contains information about the inserted disk but doesn't
contain a boolean to indicate that the device is inserted.

My understanding is that the existance of the inserted structure is what
indicates that the device is inserted.

>> +    gtk_widget_set_sensitive(bdm->eject, value);
>> +
>> +    value = !info->locked;
>> +    gtk_widget_set_sensitive(bdm->change, value);
>> +
>> +    if (info->has_inserted) {
>> +        label = info->inserted->file;
>> +        if (strlen(label) > 32) {
>> +            char *new_label;
>> +
>> +            new_label = strrchr(label, '/');
>> +            if (new_label) {
>> +                label = new_label + 1;
>> +            }
>> +        }
>> +    }
>> +
>> +    gtk_menu_item_set_label(GTK_MENU_ITEM(bdm->change), label);
>> +}
>
>> +static void gd_enum_disk(const char *path, const char *proptype, void *opaque)
>> +{
>> +    GtkDisplayState *s = opaque;
>> +    Object *obj;
>> +    char *block_id;
>> +
>> +    obj = object_resolve_path(path, NULL);
>> +    g_assert(obj != NULL);
>> +
>> +    block_id = object_property_get_str(obj, proptype, NULL);
>> +    if (strcmp(block_id, "") != 0) {
>> +        BlockDeviceMenu *bdm;
>> +        DiskType disk_type;
>> +        char *type;
>> +        char *desc = NULL;
>> +
>> +        type = object_property_get_str(obj, "type", NULL);
>> +
>> +        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
>> +            desc = object_property_get_str(obj, "drive-id", NULL);
>> +        } else {
>> +            desc = g_strdup(type);
>> +        }
>
> Ugh. Comparing the device name to an incomplete set of strings here and
> then figuring out for each what the specific way for this device is to
> create a nice string sounds like a bad idea.
>
> Why can't all devices just expose a property with a human-readable
> string? We'll need it for more than just the disk change menus.

I thought about this, there are a few concerns.  The first is that you
might lose consistency across devices.  The second is i18n.

I would like to show USB device separately from IDE devices (even if
it's a USB CDROM).  I want the menu to look something like this:

QEMU DVD-ROM QM003 ->
Floppy Disk        ->
---------------------
USB Devices        ->
                      USB Tablet                       ->
                      -----------------------------------
                      Description of USB Host Device 1 ->
                      Description of USB Host Device 2 ->
                      Description of USB Host Device 3 ->

Such that you can also do USB host device pass through via the menus.

>From an i18n point of view, I would expect 'Floppy Disk' to be
translated.  I wouldn't expect 'QEMU DVD-ROM QM003' to be translated
though since this is how the device is described within the guest.

> And then, of course, the question is still what a good human-readable
> string is. A serial number generated by qemu, as we now get by default
> for the CD-ROM, probably isn't. Something like "ATAPI CD-ROM at secondary
> master" would probably be more helpful in this case.

I was going for how the device would be described in the guest such that
if a user is looking at Device Manager in Windows or /dev/disk/by-id/ in
Linux that there would be a clear association.

>> +
>> +        if (strcmp(type, "ide-cd") == 0) {
>> +            disk_type = DT_CDROM;
>> +        } else if (strcmp(type, "isa-fdc") == 0) {
>> +            disk_type = DT_FLOPPY;
>> +        } else {
>> +            disk_type = DT_NORMAL;
>> +        }
>
> Same thing here, comparing against strings is a hack. Devices should
> probably have a property that says what kind of device they are.

Ack, this is nasty.  I would like to eliminate this.  There is a type
field in BlockInfo but:

# @type: This field is returned only for compatibility reasons, it should
#        not be used (always returns 'unknown')

I vaguely remember this happening but I don't remember the specific
reason why.  I would definitely prefer that we filled out type
correctly.

I think Markus was involved in this.  Markus or Luiz, do you remember
the story here?

Regards,

Anthony Liguori

>
>> +        bdm = g_malloc0(sizeof(*bdm));
>> +        bdm->name = g_strdup(block_id);
>> +        bdm->path = g_strdup(path);
>> +        bdm->desc = desc;
>> +        bdm->disk_type = disk_type;
>> +
>> +        g_free(type);
>> +
>> +        g_hash_table_insert(s->devices_map, bdm->name, bdm);
>> +    }
>> +    g_free(block_id);
>> +}
>
> Kevin

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Luiz Capitulino]

On Thu, 02 May 2013 08:41:50 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> >> +
> >> +        if (strcmp(type, "ide-cd") == 0) {
> >> +            disk_type = DT_CDROM;
> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
> >> +            disk_type = DT_FLOPPY;
> >> +        } else {
> >> +            disk_type = DT_NORMAL;
> >> +        }
> >
> > Same thing here, comparing against strings is a hack. Devices should
> > probably have a property that says what kind of device they are.
> 
> Ack, this is nasty.  I would like to eliminate this.  There is a type
> field in BlockInfo but:
> 
> # @type: This field is returned only for compatibility reasons, it should
> #        not be used (always returns 'unknown')
> 
> I vaguely remember this happening but I don't remember the specific
> reason why.  I would definitely prefer that we filled out type
> correctly.
> 
> I think Markus was involved in this.  Markus or Luiz, do you remember
> the story here?

IIRC, we had a type field which was a string and Markus eliminated it
because it was unreliable. I was afraid that dropping fields from a QMP
output would be incompatible, so Markus maintained the field but it's
always set to 'unknown'.

It seems totally fine to me to have a new field with the device type
as an enum, but of course it has to be reliable.

PS: For more information about the reliableness of this field please
    contact Markus :)

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Kevin Wolf]

Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
> >> +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo *info)
> >> +{
> >> +    bool value;
> >> +    const char *label = _("<No media>");
> >> +
> >> +    value = info->has_inserted && !info->locked;
> >
> > Shouldn't the actual value of info->inserted play a role as well?
> 
> inserted contains information about the inserted disk but doesn't
> contain a boolean to indicate that the device is inserted.
> 
> My understanding is that the existance of the inserted structure is what
> indicates that the device is inserted.

Sorry, my bad, I should have looked up the schema. I was indeed assuming
that it's a boolean.

> >> +static void gd_enum_disk(const char *path, const char *proptype, void *opaque)
> >> +{
> >> +    GtkDisplayState *s = opaque;
> >> +    Object *obj;
> >> +    char *block_id;
> >> +
> >> +    obj = object_resolve_path(path, NULL);
> >> +    g_assert(obj != NULL);
> >> +
> >> +    block_id = object_property_get_str(obj, proptype, NULL);
> >> +    if (strcmp(block_id, "") != 0) {
> >> +        BlockDeviceMenu *bdm;
> >> +        DiskType disk_type;
> >> +        char *type;
> >> +        char *desc = NULL;
> >> +
> >> +        type = object_property_get_str(obj, "type", NULL);
> >> +
> >> +        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
> >> +            desc = object_property_get_str(obj, "drive-id", NULL);
> >> +        } else {
> >> +            desc = g_strdup(type);
> >> +        }
> >
> > Ugh. Comparing the device name to an incomplete set of strings here and
> > then figuring out for each what the specific way for this device is to
> > create a nice string sounds like a bad idea.
> >
> > Why can't all devices just expose a property with a human-readable
> > string? We'll need it for more than just the disk change menus.
> 
> I thought about this, there are a few concerns.  The first is that you
> might lose consistency across devices.  The second is i18n.

What's the kind of consistency that you lose? I guess I could see the
point (even if not agree) if it was about creating the string here vs.
in each device, as the centralised strings would be more likely to use
the same pattern, but you already have this part in the IDE device.

The i18n point I don't buy. Avoiding i18n by choosing cryptic device
names that can't be translated isn't a good strategy. But if you do have
translations, it doesn't matter whether you have them in the GUI or in
the device itself, except that the latter could be used outside the
GTK frontend, too.

> I would like to show USB device separately from IDE devices (even if
> it's a USB CDROM).  I want the menu to look something like this:
> 
> QEMU DVD-ROM QM003 ->
> Floppy Disk        ->
> ---------------------
> USB Devices        ->
>                       USB Tablet                       ->
>                       -----------------------------------
>                       Description of USB Host Device 1 ->
>                       Description of USB Host Device 2 ->
>                       Description of USB Host Device 3 ->
> 
> Such that you can also do USB host device pass through via the menus.
> 
> From an i18n point of view, I would expect 'Floppy Disk' to be
> translated.  I wouldn't expect 'QEMU DVD-ROM QM003' to be translated
> though since this is how the device is described within the guest.

Note that there can be two floppy drives. Currently both will show up as
"isa-fdc".

I also wonder how other buses fit in your menu structure, like a SCSI
CD-ROM, or even PCI hotplug. Your proposal isn't quite consistent in
itself because it treats USB devices different from IDE or floppy
devices. (That said, I agree that CD and floppy should be accessible
from the top level. But maybe usb-storage should be as well. It's not
quite clear to me how things would work out best.)

Another inconsistency is that you want to have "USB Tablet" there,
because USB has a product description as well. Should this be "QEMU USB
Tablet"?

Anyway, none of this is actually an argument against having a property
for the human-readable name in the device, it's mostly about what should
be in there.

> > And then, of course, the question is still what a good human-readable
> > string is. A serial number generated by qemu, as we now get by default
> > for the CD-ROM, probably isn't. Something like "ATAPI CD-ROM at secondary
> > master" would probably be more helpful in this case.
> 
> I was going for how the device would be described in the guest such that
> if a user is looking at Device Manager in Windows or /dev/disk/by-id/ in
> Linux that there would be a clear association.

I see. We should probably display this somewhere, but it might not
always be the right name to interact with the user.

> >> +
> >> +        if (strcmp(type, "ide-cd") == 0) {
> >> +            disk_type = DT_CDROM;
> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
> >> +            disk_type = DT_FLOPPY;
> >> +        } else {
> >> +            disk_type = DT_NORMAL;
> >> +        }
> >
> > Same thing here, comparing against strings is a hack. Devices should
> > probably have a property that says what kind of device they are.
> 
> Ack, this is nasty.  I would like to eliminate this.  There is a type
> field in BlockInfo but:
> 
> # @type: This field is returned only for compatibility reasons, it should
> #        not be used (always returns 'unknown')
> 
> I vaguely remember this happening but I don't remember the specific
> reason why.  I would definitely prefer that we filled out type
> correctly.
> 
> I think Markus was involved in this.  Markus or Luiz, do you remember
> the story here?

The reason is that BlockInfo is about the backend and it simply doesn't
know (ever since we introduced if=none, this was buggy, so we just
abandoned it at some point). We would have to ask the device, not the
block layer.

Kevin

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
[...]
>> >> +
>> >> +        if (strcmp(type, "ide-cd") == 0) {
>> >> +            disk_type = DT_CDROM;
>> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
>> >> +            disk_type = DT_FLOPPY;
>> >> +        } else {
>> >> +            disk_type = DT_NORMAL;
>> >> +        }
>> >
>> > Same thing here, comparing against strings is a hack. Devices should
>> > probably have a property that says what kind of device they are.
>> 
>> Ack, this is nasty.  I would like to eliminate this.  There is a type
>> field in BlockInfo but:
>> 
>> # @type: This field is returned only for compatibility reasons, it should
>> #        not be used (always returns 'unknown')
>> 
>> I vaguely remember this happening but I don't remember the specific
>> reason why.  I would definitely prefer that we filled out type
>> correctly.
>> 
>> I think Markus was involved in this.  Markus or Luiz, do you remember
>> the story here?
>
> The reason is that BlockInfo is about the backend and it simply doesn't
> know (ever since we introduced if=none, this was buggy, so we just
> abandoned it at some point). We would have to ask the device, not the
> block layer.

Correct.

commit d8aeeb31d53a07a0cce36c7bcf53684953c2e445
Author: Markus Armbruster <armbru@redhat.com>
Date:   Mon May 16 15:04:55 2011 +0200

    block QMP: Deprecate query-block's "type", drop info block's "type="
    
    query-block's specification documents response member "type" with
    values "hd", "cdrom", "floppy", "unknown".
    
    Its value is unreliable: a block device used as floppy has type
    "floppy" if created with if=floppy, but type "hd" if created with
    if=none.
    
    That's because with if=none, the type is at best a declaration of
    intent: the drive can be connected to any guest device.  Its type is
    really the guest device's business.  Reporting it here is wrong.
    
    No known user of QMP uses "type".  It's unlikely that any unknown
    users exist, because its value is useless unless you know how the
    block device was created.  But then you also know the true value.
    
    Fixing the broken value risks breaking (hypothetical!) clients that
    somehow rely on the current behavior.  Not fixing the value risks
    breaking (hypothetical!) clients that rely on the value to be
    accurate.  Can't entirely avoid hypothetical lossage.  Change the
    value to be always "unknown".
    
    This makes "info block" always report "type=unknown".  Pointless.
    Change it to not report the type.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> >
>> > Ugh. Comparing the device name to an incomplete set of strings here and
>> > then figuring out for each what the specific way for this device is to
>> > create a nice string sounds like a bad idea.
>> >
>> > Why can't all devices just expose a property with a human-readable
>> > string? We'll need it for more than just the disk change menus.
>> 
>> I thought about this, there are a few concerns.  The first is that you
>> might lose consistency across devices.  The second is i18n.
>
> What's the kind of consistency that you lose? I guess I could see the
> point (even if not agree) if it was about creating the string here vs.
> in each device, as the centralised strings would be more likely to use
> the same pattern, but you already have this part in the IDE device.

Note that these menu items are not descriptions of the device's class
but of the device.  They should be unique.

So for instance, for virtio-blk-pci, we would want something like:

"VirtIO Block Device  [00:03.0]"

If no serial property is defined on it,  If there is a serial property,
then the serial property should be shown vs. the PCI address.

We probably want to show the PCI address consistently for any PCI based
block device.  This is what I mean by consistency.  It's very hard to
enforce outside of a central location.

> The i18n point I don't buy. Avoiding i18n by choosing cryptic device
> names that can't be translated isn't a good strategy. But if you do have
> translations, it doesn't matter whether you have them in the GUI or in
> the device itself, except that the latter could be used outside the
> GTK frontend, too.
>
>> I would like to show USB device separately from IDE devices (even if
>> it's a USB CDROM).  I want the menu to look something like this:
>> 
>> QEMU DVD-ROM QM003 ->
>> Floppy Disk        ->
>> ---------------------
>> USB Devices        ->
>>                       USB Tablet                       ->
>>                       -----------------------------------
>>                       Description of USB Host Device 1 ->
>>                       Description of USB Host Device 2 ->
>>                       Description of USB Host Device 3 ->
>> 
>> Such that you can also do USB host device pass through via the menus.
>> 
>> From an i18n point of view, I would expect 'Floppy Disk' to be
>> translated.  I wouldn't expect 'QEMU DVD-ROM QM003' to be translated
>> though since this is how the device is described within the guest.
>
> Note that there can be two floppy drives. Currently both will show up as
> "isa-fdc".

No, it shows the BlockDriverState name which will always be unique.

> I also wonder how other buses fit in your menu structure, like a SCSI
> CD-ROM,

SCSI CD-ROM would show above the separator.

Note that this is only showing removable devices.  Hotplug isn't
considered here.  I was thinking in the long run we could have another
menu item under USB Devices called "Add/Remove Hardware..." that would
pop up a dialog to allow for hot plug/unplug.

> or even PCI hotplug. Your proposal isn't quite consistent in
> itself because it treats USB devices different from IDE or floppy
> devices.

That's a feature as these are the most common devices.

> (That said, I agree that CD and floppy should be accessible
> from the top level. But maybe usb-storage should be as well. It's not
> quite clear to me how things would work out best.)

I agree re: removable usb-storage.  The goal of the menu is to give
prominence to what the devices are that you would interact with the most.

>
> Another inconsistency is that you want to have "USB Tablet" there,
> because USB has a product description as well. Should this be "QEMU USB
> Tablet"?

Ack.  My intention was for that to be the product description FWIW.

>> >> +
>> >> +        if (strcmp(type, "ide-cd") == 0) {
>> >> +            disk_type = DT_CDROM;
>> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
>> >> +            disk_type = DT_FLOPPY;
>> >> +        } else {
>> >> +            disk_type = DT_NORMAL;
>> >> +        }
>> >
>> > Same thing here, comparing against strings is a hack. Devices should
>> > probably have a property that says what kind of device they are.
>> 
>> Ack, this is nasty.  I would like to eliminate this.  There is a type
>> field in BlockInfo but:
>> 
>> # @type: This field is returned only for compatibility reasons, it should
>> #        not be used (always returns 'unknown')
>> 
>> I vaguely remember this happening but I don't remember the specific
>> reason why.  I would definitely prefer that we filled out type
>> correctly.
>> 
>> I think Markus was involved in this.  Markus or Luiz, do you remember
>> the story here?
>
> The reason is that BlockInfo is about the backend and it simply doesn't
> know (ever since we introduced if=none, this was buggy, so we just
> abandoned it at some point). We would have to ask the device, not the
> block layer.

Yes, this makes sense.  We could introduce an interface that all disks
implemented that returned information about whether it was a CD-ROM,
Floppy, etc.

How does libvirt cope with this today?  I presume they do something
similar to what this patch is doing in terms of hard coding device
names.

Regards,

Anthony Liguori

>
> Kevin

Re: [Qemu-devel] [libvirt] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Daniel P. Berrange]

On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> +
> >> >> +        if (strcmp(type, "ide-cd") == 0) {
> >> >> +            disk_type = DT_CDROM;
> >> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
> >> >> +            disk_type = DT_FLOPPY;
> >> >> +        } else {
> >> >> +            disk_type = DT_NORMAL;
> >> >> +        }
> >> >
> >> > Same thing here, comparing against strings is a hack. Devices should
> >> > probably have a property that says what kind of device they are.
> >> 
> >> Ack, this is nasty.  I would like to eliminate this.  There is a type
> >> field in BlockInfo but:
> >> 
> >> # @type: This field is returned only for compatibility reasons, it should
> >> #        not be used (always returns 'unknown')
> >> 
> >> I vaguely remember this happening but I don't remember the specific
> >> reason why.  I would definitely prefer that we filled out type
> >> correctly.
> >> 
> >> I think Markus was involved in this.  Markus or Luiz, do you remember
> >> the story here?
> >
> > The reason is that BlockInfo is about the backend and it simply doesn't
> > know (ever since we introduced if=none, this was buggy, so we just
> > abandoned it at some point). We would have to ask the device, not the
> > block layer.
> 
> Yes, this makes sense.  We could introduce an interface that all disks
> implemented that returned information about whether it was a CD-ROM,
> Floppy, etc.
> 
> How does libvirt cope with this today?  I presume they do something
> similar to what this patch is doing in terms of hard coding device
> names.

Sorry, not really sure what your question is here - how does libvirt
cope with what exactly ?

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [libvirt] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> >> >> +
>> >> >> +        if (strcmp(type, "ide-cd") == 0) {
>> >> >> +            disk_type = DT_CDROM;
>> >> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
>> >> >> +            disk_type = DT_FLOPPY;
>> >> >> +        } else {
>> >> >> +            disk_type = DT_NORMAL;
>> >> >> +        }
>> >> >
>> >> > Same thing here, comparing against strings is a hack. Devices should
>> >> > probably have a property that says what kind of device they are.
>> >> 
>> >> Ack, this is nasty.  I would like to eliminate this.  There is a type
>> >> field in BlockInfo but:
>> >> 
>> >> # @type: This field is returned only for compatibility reasons, it should
>> >> #        not be used (always returns 'unknown')
>> >> 
>> >> I vaguely remember this happening but I don't remember the specific
>> >> reason why.  I would definitely prefer that we filled out type
>> >> correctly.
>> >> 
>> >> I think Markus was involved in this.  Markus or Luiz, do you remember
>> >> the story here?
>> >
>> > The reason is that BlockInfo is about the backend and it simply doesn't
>> > know (ever since we introduced if=none, this was buggy, so we just
>> > abandoned it at some point). We would have to ask the device, not the
>> > block layer.
>> 
>> Yes, this makes sense.  We could introduce an interface that all disks
>> implemented that returned information about whether it was a CD-ROM,
>> Floppy, etc.
>> 
>> How does libvirt cope with this today?  I presume they do something
>> similar to what this patch is doing in terms of hard coding device
>> names.
>
> Sorry, not really sure what your question is here - how does libvirt
> cope with what exactly ?

Given a device, how do you figure out if it's a cdrom/floppy/whatever
without hard coding a mapping of class name -> device type.

Pretty sure libvirt just has a class name mapping, right?

Regards,

Anthony Liguori

>
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [libvirt] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Daniel P. Berrange]

On Fri, May 03, 2013 at 08:52:59AM -0500, Anthony Liguori wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Thu, May 02, 2013 at 10:40:06AM -0500, Anthony Liguori wrote:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> >> +
> >> >> >> +        if (strcmp(type, "ide-cd") == 0) {
> >> >> >> +            disk_type = DT_CDROM;
> >> >> >> +        } else if (strcmp(type, "isa-fdc") == 0) {
> >> >> >> +            disk_type = DT_FLOPPY;
> >> >> >> +        } else {
> >> >> >> +            disk_type = DT_NORMAL;
> >> >> >> +        }
> >> >> >
> >> >> > Same thing here, comparing against strings is a hack. Devices should
> >> >> > probably have a property that says what kind of device they are.
> >> >> 
> >> >> Ack, this is nasty.  I would like to eliminate this.  There is a type
> >> >> field in BlockInfo but:
> >> >> 
> >> >> # @type: This field is returned only for compatibility reasons, it should
> >> >> #        not be used (always returns 'unknown')
> >> >> 
> >> >> I vaguely remember this happening but I don't remember the specific
> >> >> reason why.  I would definitely prefer that we filled out type
> >> >> correctly.
> >> >> 
> >> >> I think Markus was involved in this.  Markus or Luiz, do you remember
> >> >> the story here?
> >> >
> >> > The reason is that BlockInfo is about the backend and it simply doesn't
> >> > know (ever since we introduced if=none, this was buggy, so we just
> >> > abandoned it at some point). We would have to ask the device, not the
> >> > block layer.
> >> 
> >> Yes, this makes sense.  We could introduce an interface that all disks
> >> implemented that returned information about whether it was a CD-ROM,
> >> Floppy, etc.
> >> 
> >> How does libvirt cope with this today?  I presume they do something
> >> similar to what this patch is doing in terms of hard coding device
> >> names.
> >
> > Sorry, not really sure what your question is here - how does libvirt
> > cope with what exactly ?
> 
> Given a device, how do you figure out if it's a cdrom/floppy/whatever
> without hard coding a mapping of class name -> device type.
> 
> Pretty sure libvirt just has a class name mapping, right?

The only place where we'd ever need todo that is when we reverse engineer
a libvirt XML config from a set of QEMU command line args. For that we
just look at the if=XXX parameter currently. Our reverse engineering code
is currently broken for if=none scenarios, due mostly to our laziness
in writing code to parse the corresponding -device arg.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
>> >> +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo *info)
>> >> +{
>> >> +    bool value;
>> >> +    const char *label = _("<No media>");
>> >> +
>> >> +    value = info->has_inserted && !info->locked;
>> >
>> > Shouldn't the actual value of info->inserted play a role as well?
>> 
>> inserted contains information about the inserted disk but doesn't
>> contain a boolean to indicate that the device is inserted.
>> 
>> My understanding is that the existance of the inserted structure is what
>> indicates that the device is inserted.
>
> Sorry, my bad, I should have looked up the schema. I was indeed assuming
> that it's a boolean.
>
>> >> +static void gd_enum_disk(const char *path, const char *proptype, void *opaque)
>> >> +{
>> >> +    GtkDisplayState *s = opaque;
>> >> +    Object *obj;
>> >> +    char *block_id;
>> >> +
>> >> +    obj = object_resolve_path(path, NULL);
>> >> +    g_assert(obj != NULL);
>> >> +
>> >> +    block_id = object_property_get_str(obj, proptype, NULL);
>> >> +    if (strcmp(block_id, "") != 0) {
>> >> +        BlockDeviceMenu *bdm;
>> >> +        DiskType disk_type;
>> >> +        char *type;
>> >> +        char *desc = NULL;
>> >> +
>> >> +        type = object_property_get_str(obj, "type", NULL);
>> >> +
>> >> +        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
>> >> +            desc = object_property_get_str(obj, "drive-id", NULL);
>> >> +        } else {
>> >> +            desc = g_strdup(type);
>> >> +        }
>> >
>> > Ugh. Comparing the device name to an incomplete set of strings here and
>> > then figuring out for each what the specific way for this device is to
>> > create a nice string sounds like a bad idea.
>> >
>> > Why can't all devices just expose a property with a human-readable
>> > string? We'll need it for more than just the disk change menus.
>> 
>> I thought about this, there are a few concerns.  The first is that you
>> might lose consistency across devices.  The second is i18n.
>
> What's the kind of consistency that you lose? I guess I could see the
> point (even if not agree) if it was about creating the string here vs.
> in each device, as the centralised strings would be more likely to use
> the same pattern, but you already have this part in the IDE device.
>
> The i18n point I don't buy. Avoiding i18n by choosing cryptic device
> names that can't be translated isn't a good strategy. But if you do have
> translations, it doesn't matter whether you have them in the GUI or in
> the device itself, except that the latter could be used outside the
> GTK frontend, too.
>
>> I would like to show USB device separately from IDE devices (even if
>> it's a USB CDROM).  I want the menu to look something like this:
>> 
>> QEMU DVD-ROM QM003 ->
>> Floppy Disk        ->
>> ---------------------
>> USB Devices        ->
>>                       USB Tablet                       ->
>>                       -----------------------------------
>>                       Description of USB Host Device 1 ->
>>                       Description of USB Host Device 2 ->
>>                       Description of USB Host Device 3 ->
>> 
>> Such that you can also do USB host device pass through via the menus.
>> 
>> From an i18n point of view, I would expect 'Floppy Disk' to be
>> translated.  I wouldn't expect 'QEMU DVD-ROM QM003' to be translated
>> though since this is how the device is described within the guest.
>
> Note that there can be two floppy drives. Currently both will show up as
> "isa-fdc".

This is a bug.  My intention was to use block_id as the description, not
type.

Regards,

Anthony Liguori