Re: [Qemu-devel] [PATCH 2/3] migration: Allow "device add" options to only add migratable devices

[Ashijeet Acharya]

On Thu, Dec 15, 2016 at 9:49 PM, Dr. David Alan Gilbert
 wrote:
> * Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
>> On Thu, Dec 15, 2016 at 9:35 PM, Dr. David Alan Gilbert
>>  wrote:
>> > * Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
>> >> Introduce checks for the unmigratable flag in the VMStateDescription
>> >> structs of respective devices when user attempts to add them. If the
>> >> "--only-migratable" was specified, all unmigratable devices will
>> >> rightly fail to add. This feature is made compatible for both "-device"
>> >> and "-usbdevice" command line options and covers their hmp and qmp
>> >> counterparts as well.
>> >>
>> >> Signed-off-by: Ashijeet Acharya 
>> >> ---
>> >>  hw/usb/bus.c   | 15 +++
>> >>  qdev-monitor.c |  9 +
>> >>  2 files changed, 24 insertions(+)
>> >>
>> >> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
>> >> index 25913ad..8796788 100644
>> >> --- a/hw/usb/bus.c
>> >> +++ b/hw/usb/bus.c
>> >> @@ -8,6 +8,7 @@
>> >>  #include "monitor/monitor.h"
>> >>  #include "trace.h"
>> >>  #include "qemu/cutils.h"
>> >> +#include "migration/migration.h"
>> >>
>> >>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int 
>> >> indent);
>> >>
>> >> @@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
>> >>  const char *params;
>> >>  int len;
>> >>  USBDevice *dev;
>> >> +ObjectClass *klass;
>> >> +DeviceClass *dc;
>> >>
>> >>  params = strchr(cmdline,':');
>> >>  if (params) {
>> >> @@ -720,6 +723,18 @@ USBDevice *usbdevice_create(const char *cmdline)
>> >>  return NULL;
>> >>  }
>> >>
>> >> +klass = object_class_by_name(f->name);
>> >
>> >> +dc = DEVICE_CLASS(klass);
>> >
>> > Would qdev_get_device_class work here instead of that pair?
>>
>> No, because I believe that is defined as static in qdev_monitor.c
>> which was one of the reasons I faced a difficulty with this one.
>
> Hmm, I wonder if Markus can suggest a better way; qdev_get_device_class
> seems a lot more complex dealing iwth aliases and stuff, if we need that
> type of stuff.

Okay. I have grepped around for other uses of the macro DEVICE_CLASS
which is mostly during the initialization of the various devices and
we are not dealing with aliases probably because the device name is
rightly there. But similarly here, I am trusting the device name
stored in "f->name" more so after following the function call flow to
the lowest level and finding a similar call to object_class_by_name()
to get the ObjectClass of the device which passes the same name as
stored in f->name.

Ashijeet
>
> Dave
>
>> > (I was thinking you needed to check klass to make sure it wasn't null)
>>
>> Right. I will include the check in v2.
>>
>> Ashijeet
>> >
>> >> +if (only_migratable) {
>> >> +if (dc->vmsd->unmigratable) {
>> >> +error_report("Device %s is not migratable, but 
>> >> --only-migratable "
>> >> + "was specified", f->name);
>> >> +return NULL;
>> >> +}
>> >> +}
>> >> +
>> >>  if (f->usbdevice_init) {
>> >>  dev = f->usbdevice_init(bus, params);
>> >>  } else {
>> >> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> >> index c73410c..81d01df 100644
>> >> --- a/qdev-monitor.c
>> >> +++ b/qdev-monitor.c
>> >> @@ -29,6 +29,7 @@
>> >>  #include "qemu/error-report.h"
>> >>  #include "qemu/help_option.h"
>> >>  #include "sysemu/block-backend.h"
>> >> +#include "migration/migration.h"
>> >>
>> >>  /*
>> >>   * Aliases were a bad idea from the start.  Let's keep them
>> >> @@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> >> **errp)
>> >>  return NULL;
>> >>  }
>> >>
>> >> +if (only_migratable) {
>> >> +if (dc->vmsd->unmigratable) {
>> >> +error_setg(errp, "Device %s is not migratable, but "
>> >> +   "--only-migratable was specified", driver);
>> >> +return NULL;
>> >> +}
>> >> +}
>> >> +
>> >>  /* find bus */
>> >>  path = qemu_opt_get(opts, "bus");
>> >>  if (path != NULL) {
>> >> --
>> >> 2.6.2
>> >>
>> > --
>> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Allow "device add" options to only add migratable devices

[Ashijeet Acharya]

On Thu, Dec 15, 2016 at 9:35 PM, Dr. David Alan Gilbert
 wrote:
> * Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
>> Introduce checks for the unmigratable flag in the VMStateDescription
>> structs of respective devices when user attempts to add them. If the
>> "--only-migratable" was specified, all unmigratable devices will
>> rightly fail to add. This feature is made compatible for both "-device"
>> and "-usbdevice" command line options and covers their hmp and qmp
>> counterparts as well.
>>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>>  hw/usb/bus.c   | 15 +++
>>  qdev-monitor.c |  9 +
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
>> index 25913ad..8796788 100644
>> --- a/hw/usb/bus.c
>> +++ b/hw/usb/bus.c
>> @@ -8,6 +8,7 @@
>>  #include "monitor/monitor.h"
>>  #include "trace.h"
>>  #include "qemu/cutils.h"
>> +#include "migration/migration.h"
>>
>>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>>
>> @@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
>>  const char *params;
>>  int len;
>>  USBDevice *dev;
>> +ObjectClass *klass;
>> +DeviceClass *dc;
>>
>>  params = strchr(cmdline,':');
>>  if (params) {
>> @@ -720,6 +723,18 @@ USBDevice *usbdevice_create(const char *cmdline)
>>  return NULL;
>>  }
>>
>> +klass = object_class_by_name(f->name);
>
>> +dc = DEVICE_CLASS(klass);
>
> Would qdev_get_device_class work here instead of that pair?

No, because I believe that is defined as static in qdev_monitor.c
which was one of the reasons I faced a difficulty with this one.

> (I was thinking you needed to check klass to make sure it wasn't null)

Right. I will include the check in v2.

Ashijeet
>
>> +if (only_migratable) {
>> +if (dc->vmsd->unmigratable) {
>> +error_report("Device %s is not migratable, but 
>> --only-migratable "
>> + "was specified", f->name);
>> +return NULL;
>> +}
>> +}
>> +
>>  if (f->usbdevice_init) {
>>  dev = f->usbdevice_init(bus, params);
>>  } else {
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index c73410c..81d01df 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -29,6 +29,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/help_option.h"
>>  #include "sysemu/block-backend.h"
>> +#include "migration/migration.h"
>>
>>  /*
>>   * Aliases were a bad idea from the start.  Let's keep them
>> @@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>  return NULL;
>>  }
>>
>> +if (only_migratable) {
>> +if (dc->vmsd->unmigratable) {
>> +error_setg(errp, "Device %s is not migratable, but "
>> +   "--only-migratable was specified", driver);
>> +return NULL;
>> +}
>> +}
>> +
>>  /* find bus */
>>  path = qemu_opt_get(opts, "bus");
>>  if (path != NULL) {
>> --
>> 2.6.2
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Allow "device add" options to only add migratable devices

[Dr. David Alan Gilbert]

* Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
> On Thu, Dec 15, 2016 at 9:35 PM, Dr. David Alan Gilbert
>  wrote:
> > * Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
> >> Introduce checks for the unmigratable flag in the VMStateDescription
> >> structs of respective devices when user attempts to add them. If the
> >> "--only-migratable" was specified, all unmigratable devices will
> >> rightly fail to add. This feature is made compatible for both "-device"
> >> and "-usbdevice" command line options and covers their hmp and qmp
> >> counterparts as well.
> >>
> >> Signed-off-by: Ashijeet Acharya 
> >> ---
> >>  hw/usb/bus.c   | 15 +++
> >>  qdev-monitor.c |  9 +
> >>  2 files changed, 24 insertions(+)
> >>
> >> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> >> index 25913ad..8796788 100644
> >> --- a/hw/usb/bus.c
> >> +++ b/hw/usb/bus.c
> >> @@ -8,6 +8,7 @@
> >>  #include "monitor/monitor.h"
> >>  #include "trace.h"
> >>  #include "qemu/cutils.h"
> >> +#include "migration/migration.h"
> >>
> >>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int 
> >> indent);
> >>
> >> @@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
> >>  const char *params;
> >>  int len;
> >>  USBDevice *dev;
> >> +ObjectClass *klass;
> >> +DeviceClass *dc;
> >>
> >>  params = strchr(cmdline,':');
> >>  if (params) {
> >> @@ -720,6 +723,18 @@ USBDevice *usbdevice_create(const char *cmdline)
> >>  return NULL;
> >>  }
> >>
> >> +klass = object_class_by_name(f->name);
> >
> >> +dc = DEVICE_CLASS(klass);
> >
> > Would qdev_get_device_class work here instead of that pair?
> 
> No, because I believe that is defined as static in qdev_monitor.c
> which was one of the reasons I faced a difficulty with this one.

Hmm, I wonder if Markus can suggest a better way; qdev_get_device_class
seems a lot more complex dealing iwth aliases and stuff, if we need that
type of stuff.

Dave

> > (I was thinking you needed to check klass to make sure it wasn't null)
> 
> Right. I will include the check in v2.
> 
> Ashijeet
> >
> >> +if (only_migratable) {
> >> +if (dc->vmsd->unmigratable) {
> >> +error_report("Device %s is not migratable, but 
> >> --only-migratable "
> >> + "was specified", f->name);
> >> +return NULL;
> >> +}
> >> +}
> >> +
> >>  if (f->usbdevice_init) {
> >>  dev = f->usbdevice_init(bus, params);
> >>  } else {
> >> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >> index c73410c..81d01df 100644
> >> --- a/qdev-monitor.c
> >> +++ b/qdev-monitor.c
> >> @@ -29,6 +29,7 @@
> >>  #include "qemu/error-report.h"
> >>  #include "qemu/help_option.h"
> >>  #include "sysemu/block-backend.h"
> >> +#include "migration/migration.h"
> >>
> >>  /*
> >>   * Aliases were a bad idea from the start.  Let's keep them
> >> @@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> >> **errp)
> >>  return NULL;
> >>  }
> >>
> >> +if (only_migratable) {
> >> +if (dc->vmsd->unmigratable) {
> >> +error_setg(errp, "Device %s is not migratable, but "
> >> +   "--only-migratable was specified", driver);
> >> +return NULL;
> >> +}
> >> +}
> >> +
> >>  /* find bus */
> >>  path = qemu_opt_get(opts, "bus");
> >>  if (path != NULL) {
> >> --
> >> 2.6.2
> >>
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Allow "device add" options to only add migratable devices

[Dr. David Alan Gilbert]

* Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
> Introduce checks for the unmigratable flag in the VMStateDescription
> structs of respective devices when user attempts to add them. If the
> "--only-migratable" was specified, all unmigratable devices will
> rightly fail to add. This feature is made compatible for both "-device"
> and "-usbdevice" command line options and covers their hmp and qmp
> counterparts as well.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  hw/usb/bus.c   | 15 +++
>  qdev-monitor.c |  9 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 25913ad..8796788 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -8,6 +8,7 @@
>  #include "monitor/monitor.h"
>  #include "trace.h"
>  #include "qemu/cutils.h"
> +#include "migration/migration.h"
>  
>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>  
> @@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
>  const char *params;
>  int len;
>  USBDevice *dev;
> +ObjectClass *klass;
> +DeviceClass *dc;
>  
>  params = strchr(cmdline,':');
>  if (params) {
> @@ -720,6 +723,18 @@ USBDevice *usbdevice_create(const char *cmdline)
>  return NULL;
>  }
>  
> +klass = object_class_by_name(f->name);

> +dc = DEVICE_CLASS(klass);

Would qdev_get_device_class work here instead of that pair?
(I was thinking you needed to check klass to make sure it wasn't null)

> +if (only_migratable) {
> +if (dc->vmsd->unmigratable) {
> +error_report("Device %s is not migratable, but --only-migratable 
> "
> + "was specified", f->name);
> +return NULL;
> +}
> +}
> +
>  if (f->usbdevice_init) {
>  dev = f->usbdevice_init(bus, params);
>  } else {
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index c73410c..81d01df 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -29,6 +29,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/help_option.h"
>  #include "sysemu/block-backend.h"
> +#include "migration/migration.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>  return NULL;
>  }
>  
> +if (only_migratable) {
> +if (dc->vmsd->unmigratable) {
> +error_setg(errp, "Device %s is not migratable, but "
> +   "--only-migratable was specified", driver);
> +return NULL;
> +}
> +}
> +
>  /* find bus */
>  path = qemu_opt_get(opts, "bus");
>  if (path != NULL) {
> -- 
> 2.6.2
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/3] migration: Allow "device add" options to only add migratable devices

[Ashijeet Acharya]

Introduce checks for the unmigratable flag in the VMStateDescription
structs of respective devices when user attempts to add them. If the
"--only-migratable" was specified, all unmigratable devices will
rightly fail to add. This feature is made compatible for both "-device"
and "-usbdevice" command line options and covers their hmp and qmp
counterparts as well.

Signed-off-by: Ashijeet Acharya 
---
 hw/usb/bus.c   | 15 +++
 qdev-monitor.c |  9 +
 2 files changed, 24 insertions(+)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 25913ad..8796788 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -8,6 +8,7 @@
 #include "monitor/monitor.h"
 #include "trace.h"
 #include "qemu/cutils.h"
+#include "migration/migration.h"
 
 static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
 
@@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
 const char *params;
 int len;
 USBDevice *dev;
+ObjectClass *klass;
+DeviceClass *dc;
 
 params = strchr(cmdline,':');
 if (params) {
@@ -720,6 +723,18 @@ USBDevice *usbdevice_create(const char *cmdline)
 return NULL;
 }
 
+klass = object_class_by_name(f->name);
+
+dc = DEVICE_CLASS(klass);
+
+if (only_migratable) {
+if (dc->vmsd->unmigratable) {
+error_report("Device %s is not migratable, but --only-migratable "
+ "was specified", f->name);
+return NULL;
+}
+}
+
 if (f->usbdevice_init) {
 dev = f->usbdevice_init(bus, params);
 } else {
diff --git a/qdev-monitor.c b/qdev-monitor.c
index c73410c..81d01df 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -29,6 +29,7 @@
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
 #include "sysemu/block-backend.h"
+#include "migration/migration.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 return NULL;
 }
 
+if (only_migratable) {
+if (dc->vmsd->unmigratable) {
+error_setg(errp, "Device %s is not migratable, but "
+   "--only-migratable was specified", driver);
+return NULL;
+}
+}
+
 /* find bus */
 path = qemu_opt_get(opts, "bus");
 if (path != NULL) {
-- 
2.6.2