Re: [Qemu-block] [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[Markus Armbruster]

John Snow js...@redhat.com writes:

 We want to change the current default drive type,
 but to be kind, we need to allow users to specify
 the old drive type somehow.

Uh, what is *this* commit about?  as far as I can tell, it adds drive
type properties (not a default drive type option), but doesn't put
them to use, yet.


 Signed-off-by: John Snow js...@redhat.com
 ---
  hw/block/fdc.c   | 13 +
  hw/core/qdev-properties.c| 12 
  include/hw/block/fdc.h   |  7 +--
  include/hw/qdev-properties.h |  1 +
  qapi/block.json  | 15 +++
  5 files changed, 42 insertions(+), 6 deletions(-)

 diff --git a/hw/block/fdc.c b/hw/block/fdc.c
 index cdf9e09..1023a01 100644
 --- a/hw/block/fdc.c
 +++ b/hw/block/fdc.c
 @@ -67,6 +67,8 @@ typedef struct FDFormat {
  FDriveRate rate;
  } FDFormat;
  
 +#define FDRIVE_DEFAULT FDRIVE_DRV_144
 +
  static const FDFormat fd_formats[] = {
  /* First entry is default format */
  /* 1.44 MB 31/2 floppy disks */
 @@ -578,6 +580,9 @@ struct FDCtrl {
  /* Timers state */
  uint8_t timer0;
  uint8_t timer1;
 +
 +FDriveType defaultA;
 +FDriveType defaultB;
  };
  
  #define TYPE_SYSBUS_FDC base-sysbus-fdc
 @@ -2423,6 +2428,10 @@ static Property isa_fdc_properties[] = {
  DEFINE_PROP_DRIVE(driveB, FDCtrlISABus, state.drives[1].blk),
  DEFINE_PROP_BIT(check_media_rate, FDCtrlISABus, state.check_media_rate,
  0, true),
 +DEFINE_PROP_DEFAULT(defaultA, FDCtrlISABus, state.defaultA,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
 +DEFINE_PROP_DEFAULT(defaultB, FDCtrlISABus, state.defaultB,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
  DEFINE_PROP_END_OF_LIST(),
  };
  
 @@ -2471,6 +2480,10 @@ static const VMStateDescription vmstate_sysbus_fdc ={
  static Property sysbus_fdc_properties[] = {
  DEFINE_PROP_DRIVE(driveA, FDCtrlSysBus, state.drives[0].blk),
  DEFINE_PROP_DRIVE(driveB, FDCtrlSysBus, state.drives[1].blk),
 +DEFINE_PROP_DEFAULT(defaultA, FDCtrlSysBus, state.defaultA,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
 +DEFINE_PROP_DEFAULT(defaultB, FDCtrlSysBus, state.defaultB,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
  DEFINE_PROP_END_OF_LIST(),
  };
  

defaultA is not exactly a self-documenting name for a property
selecting the drive type.  Even equally laconic typeA feels better.

 diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
 index 47c1e8f..7ff8030 100644
 --- a/hw/core/qdev-properties.c
 +++ b/hw/core/qdev-properties.c
 @@ -7,6 +7,7 @@
  #include net/hub.h
  #include qapi/visitor.h
  #include sysemu/char.h
 +#include hw/block/fdc.h

Why do you need to add the include?

  
  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
Error **errp)
 @@ -543,6 +544,17 @@ PropertyInfo qdev_prop_bios_chs_trans = {
  .set = set_enum,
  };
  
 +/* --- FDC default drive types */
 +
 +PropertyInfo qdev_prop_fdc_drive_type = {
 +.name = FdcDriveType,
 +.description = FDC drive type, 
 +   none/120/144/288,
 +.enum_table = FDRIVE_DRV_lookup,
 +.get = get_enum,
 +.set = set_enum
 +};
 +
  /* --- pci address --- */
  
  /*
 diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
 index d48b2f8..f727027 100644
 --- a/include/hw/block/fdc.h
 +++ b/include/hw/block/fdc.h
 @@ -6,12 +6,7 @@
  /* fdc.c */
  #define MAX_FD 2
  
 -typedef enum FDriveType {
 -FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 35 drive  */
 -FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 35 drive  */
 -FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 525 drive */
 -FDRIVE_DRV_NONE = 0x03,   /* No drive connected */
 -} FDriveType;
 +typedef FDRIVE_DRV FDriveType;

See below.

  
  #define TYPE_ISA_FDC isa-fdc
  
 diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
 index 0cfff1c..0872b41 100644
 --- a/include/hw/qdev-properties.h
 +++ b/include/hw/qdev-properties.h
 @@ -20,6 +20,7 @@ extern PropertyInfo qdev_prop_ptr;
  extern PropertyInfo qdev_prop_macaddr;
  extern PropertyInfo qdev_prop_losttickpolicy;
  extern PropertyInfo qdev_prop_bios_chs_trans;
 +extern PropertyInfo qdev_prop_fdc_drive_type;
  extern PropertyInfo qdev_prop_drive;
  extern PropertyInfo qdev_prop_netdev;
  extern PropertyInfo qdev_prop_vlan;
 diff --git a/qapi/block.json b/qapi/block.json
 index aad645c..0c747a1 100644
 --- a/qapi/block.json
 +++ b/qapi/block.json
 @@ -40,6 +40,21 @@
'data': ['auto', 'none', 'lba', 'large', 'rechs']}
  
  ##
 +# FDRIVE_DRV:

Stick in the usual @, please, just for consistency.

FDRIVE_DRV is an unusual name for a QAPI type.  I guess you chose it so
only the type name changes, but the enumeration constants stay the same.
You then hide away the type name change with a typedef in 

Re: [Qemu-block] [Qemu-devel] [PATCH] opts: produce valid command line in qemu_opts_print

[Kővágó Zoltán]

2015-07-03 15:01 keltezéssel, Markus Armbruster írta:

Kővágó, Zoltán dirty.ice...@gmail.com writes:


This will let us print options in a format that the user would actually
write it on the command line (foo=bar,baz=asd,etc=def), without
prepending a spurious comma at the beginning of the list, or quoting
values unnecessarily.  This patch provides the following changes:
* write and id=, if the option has an id
* do not print separator before the first element
* do not quote string arguments
* properly escape commas (,) for QEMU

Signed-off-by: Kővágó, Zoltán dirty.ice...@gmail.com
---

Changes from patch submitted as part of `qapi flattening':
* no longer tries to do proper escaping for the shell

  block.c|  2 +-
  util/qemu-option.c | 30 +++---
  2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 7e130cc..78a5304 100644
--- a/block.c
+++ b/block.c
@@ -3813,7 +3813,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
  }

  if (!quiet) {
-printf(Formatting '%s', fmt=%s, filename, fmt);
+printf(Formatting '%s', fmt=%s , filename, fmt);
  qemu_opts_print(opts,  );
  puts();
  }
diff --git a/util/qemu-option.c b/util/qemu-option.c
index efe9d27..55ef172 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -730,14 +730,36 @@ void qemu_opts_del(QemuOpts *opts)
  g_free(opts);
  }

-void qemu_opts_print(QemuOpts *opts, const char *sep)
+/* print value, escaping any commas in value */
+static void escaped_print(const char *value)


Have you searched the tree for existing code doing this?


I didn't found any function like that.


+{
+const char *ptr;
+
+for (ptr = value; *ptr; ++ptr) {
+if (*ptr == ',') {
+printf(,,);
+} else {
+putchar(*ptr);
+}
+}


Slightly simpler:

 if (*ptr == ',') {
 putchar(',');
 }
 putchar(*ptr);

Matter of taste.


I also like it better that way. Should I send a v2 patch?


+}
+
+void qemu_opts_print(QemuOpts *opts, const char *separator)
  {
  QemuOpt *opt;
  QemuOptDesc *desc = opts-list-desc;
+const char *sep = ;
+
+if (opts-id) {
+printf(id=%s, opts-id); /* passed id_wellformed - no commas */
+sep = separator;
+}

  if (desc[0].name == NULL) {
  QTAILQ_FOREACH(opt, opts-head, next) {
-printf(%s%s=\%s\, sep, opt-name, opt-str);
+printf(%s%s=, sep, opt-name);
+escaped_print(opt-str);
+sep = separator;
  }
  return;
  }
@@ -750,13 +772,15 @@ void qemu_opts_print(QemuOpts *opts, const char *sep)
  continue;
  }
  if (desc-type == QEMU_OPT_STRING) {
-printf(%s%s='%s', sep, desc-name, value);
+printf(%s%s=, sep, desc-name);
+escaped_print(value);
  } else if ((desc-type == QEMU_OPT_SIZE ||
  desc-type == QEMU_OPT_NUMBER)  opt) {
  printf(%s%s=% PRId64, sep, desc-name, opt-value.uint);
  } else {
  printf(%s%s=%s, sep, desc-name, value);
  }
+sep = separator;
  }
  }


Reviewed-by: Markus Armbruster arm...@redhat.com

Re: [Qemu-block] [Qemu-devel] [RFC 08/10] fdc: refactor pick_geometry

[Markus Armbruster]

John Snow js...@redhat.com writes:

 This one is the crazy one.

I'm afraid I don't have the mental capacity to properly review this one
right now.

From what I've understood of your series so far, it's a strict decrease
of fdc craziness.  Thanks!

Re: [Qemu-block] [Qemu-devel] [PATCH] opts: produce valid command line in qemu_opts_print

[Markus Armbruster]

Kővágó, Zoltán dirty.ice...@gmail.com writes:

 This will let us print options in a format that the user would actually
 write it on the command line (foo=bar,baz=asd,etc=def), without
 prepending a spurious comma at the beginning of the list, or quoting
 values unnecessarily.  This patch provides the following changes:
 * write and id=, if the option has an id
 * do not print separator before the first element
 * do not quote string arguments
 * properly escape commas (,) for QEMU

 Signed-off-by: Kővágó, Zoltán dirty.ice...@gmail.com
 ---

 Changes from patch submitted as part of `qapi flattening':
 * no longer tries to do proper escaping for the shell

  block.c|  2 +-
  util/qemu-option.c | 30 +++---
  2 files changed, 28 insertions(+), 4 deletions(-)

 diff --git a/block.c b/block.c
 index 7e130cc..78a5304 100644
 --- a/block.c
 +++ b/block.c
 @@ -3813,7 +3813,7 @@ void bdrv_img_create(const char *filename, const char 
 *fmt,
  }
  
  if (!quiet) {
 -printf(Formatting '%s', fmt=%s, filename, fmt);
 +printf(Formatting '%s', fmt=%s , filename, fmt);
  qemu_opts_print(opts,  );
  puts();
  }
 diff --git a/util/qemu-option.c b/util/qemu-option.c
 index efe9d27..55ef172 100644
 --- a/util/qemu-option.c
 +++ b/util/qemu-option.c
 @@ -730,14 +730,36 @@ void qemu_opts_del(QemuOpts *opts)
  g_free(opts);
  }
  
 -void qemu_opts_print(QemuOpts *opts, const char *sep)
 +/* print value, escaping any commas in value */
 +static void escaped_print(const char *value)

Have you searched the tree for existing code doing this?

 +{
 +const char *ptr;
 +
 +for (ptr = value; *ptr; ++ptr) {
 +if (*ptr == ',') {
 +printf(,,);
 +} else {
 +putchar(*ptr);
 +}
 +}

Slightly simpler:

if (*ptr == ',') {
putchar(',');
}
putchar(*ptr);

Matter of taste.

 +}
 +
 +void qemu_opts_print(QemuOpts *opts, const char *separator)
  {
  QemuOpt *opt;
  QemuOptDesc *desc = opts-list-desc;
 +const char *sep = ;
 +
 +if (opts-id) {
 +printf(id=%s, opts-id); /* passed id_wellformed - no commas */
 +sep = separator;
 +}
  
  if (desc[0].name == NULL) {
  QTAILQ_FOREACH(opt, opts-head, next) {
 -printf(%s%s=\%s\, sep, opt-name, opt-str);
 +printf(%s%s=, sep, opt-name);
 +escaped_print(opt-str);
 +sep = separator;
  }
  return;
  }
 @@ -750,13 +772,15 @@ void qemu_opts_print(QemuOpts *opts, const char *sep)
  continue;
  }
  if (desc-type == QEMU_OPT_STRING) {
 -printf(%s%s='%s', sep, desc-name, value);
 +printf(%s%s=, sep, desc-name);
 +escaped_print(value);
  } else if ((desc-type == QEMU_OPT_SIZE ||
  desc-type == QEMU_OPT_NUMBER)  opt) {
  printf(%s%s=% PRId64, sep, desc-name, opt-value.uint);
  } else {
  printf(%s%s=%s, sep, desc-name, value);
  }
 +sep = separator;
  }
  }

Reviewed-by: Markus Armbruster arm...@redhat.com

Re: [Qemu-block] [PATCH COLO-BLOCK v7 16/17] quorum: implement block driver interfaces for block replication

[Alberto Garcia]

On Tue 30 Jun 2015 05:34:44 AM CEST, Wen Congyang wrote:

 +static void quorum_start_replication(BlockDriverState *bs, ReplicationMode 
 mode,
 + Error **errp)
 +{
 +BDRVQuorumState *s = bs-opaque;
 +int count = 0, i, index;
 +Error *local_err = NULL;
 +
 +/*
 + * TODO: support REPLICATION_MODE_SECONDARY if we allow secondary
 + * QEMU becoming primary QEMU.
 + */
 +if (mode != REPLICATION_MODE_PRIMARY) {
 +error_setg(errp, Invalid parameter 'mode');
 +return;
 +}
 +
 +if (s-read_pattern != QUORUM_READ_PATTERN_FIFO) {
 +error_setg(errp, Invalid parameter 'read pattern');
 +return;
 +}

Those error messages seem a bit confusing. As I understand it, what's
wrong is the value of the parameter, not the parameter itself.

A more descriptive error message would be something along the lines of
The only supported replication mode for this operation is 'primary',
The only supported read pattern for this operation is 'fifo'.

It doesn't need to be those exact words, but the idea is that the
message explains what's wrong.

 +if (count == 0) {
 +/* No child supports block replication */
 +error_setg(errp, this feature or command is not currently 
 supported);
 +} else if (count  1) {
 +for (i = 0; i  s-num_children; i++) {
 +bdrv_stop_replication(s-bs[i], false, NULL);
 +}
 +error_setg(errp, too many children support block replication);
 +} else {
 +s-replication_index = index;
 +}
 +}

Not very important, but the previous error messages start with uppercase
and these are all lowercase.

 +error_setg(errp, Block replication is not started);

It sounds a bit odd to me, any native speaker can confirm?

Comments about the messages aside, the code looks good to me, hence

Reviewed-by: Alberto Garcia be...@igalia.com

Berto

Re: [Qemu-block] [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[Markus Armbruster]

John Snow js...@redhat.com writes:

 We want to change the current default drive type,
 but to be kind, we need to allow users to specify
 the old drive type somehow.

 Signed-off-by: John Snow js...@redhat.com
 ---
  hw/block/fdc.c   | 13 +
  hw/core/qdev-properties.c| 12 
  include/hw/block/fdc.h   |  7 +--
  include/hw/qdev-properties.h |  1 +
  qapi/block.json  | 15 +++
  5 files changed, 42 insertions(+), 6 deletions(-)

 diff --git a/hw/block/fdc.c b/hw/block/fdc.c
 index cdf9e09..1023a01 100644
 --- a/hw/block/fdc.c
 +++ b/hw/block/fdc.c
 @@ -67,6 +67,8 @@ typedef struct FDFormat {
  FDriveRate rate;
  } FDFormat;
  
 +#define FDRIVE_DEFAULT FDRIVE_DRV_144
 +
  static const FDFormat fd_formats[] = {
  /* First entry is default format */
  /* 1.44 MB 31/2 floppy disks */
 @@ -578,6 +580,9 @@ struct FDCtrl {
  /* Timers state */
  uint8_t timer0;
  uint8_t timer1;
 +
 +FDriveType defaultA;
 +FDriveType defaultB;
  };
  
  #define TYPE_SYSBUS_FDC base-sysbus-fdc
 @@ -2423,6 +2428,10 @@ static Property isa_fdc_properties[] = {
  DEFINE_PROP_DRIVE(driveB, FDCtrlISABus, state.drives[1].blk),
  DEFINE_PROP_BIT(check_media_rate, FDCtrlISABus, state.check_media_rate,
  0, true),
 +DEFINE_PROP_DEFAULT(defaultA, FDCtrlISABus, state.defaultA,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
 +DEFINE_PROP_DEFAULT(defaultB, FDCtrlISABus, state.defaultB,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
  DEFINE_PROP_END_OF_LIST(),
  };
  
 @@ -2471,6 +2480,10 @@ static const VMStateDescription vmstate_sysbus_fdc ={
  static Property sysbus_fdc_properties[] = {
  DEFINE_PROP_DRIVE(driveA, FDCtrlSysBus, state.drives[0].blk),
  DEFINE_PROP_DRIVE(driveB, FDCtrlSysBus, state.drives[1].blk),
 +DEFINE_PROP_DEFAULT(defaultA, FDCtrlSysBus, state.defaultA,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
 +DEFINE_PROP_DEFAULT(defaultB, FDCtrlSysBus, state.defaultB,
 +FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, 
 FDriveType),
  DEFINE_PROP_END_OF_LIST(),
  };
  

You missed sun4m_fdc_properties[].

[...]

Re: [Qemu-block] [Qemu-devel] [RFC 05/10] fdc: refactor pick_geometry

[Markus Armbruster]

John Snow js...@redhat.com writes:

 Lessen the number of parameters it takes.

 Signed-off-by: John Snow js...@redhat.com

Haven't reviewed in detail, but: YES, please!

Re: [Qemu-block] [PATCH COLO-BLOCK v7 00/17] Block replication for continuous checkpoints

[Dr. David Alan Gilbert]

* Wen Congyang (we...@cn.fujitsu.com) wrote:
 Block replication is a very important feature which is used for
 continuous checkpoints(for example: COLO).
 
 Usage:
 Please refer to docs/block-replication.txt
 
 You can get the patch here:
 https://github.com/wencongyang/qemu-colo/commits/block-replication-v7
 
 You can get ths patch with framework here:
 https://github.com/wencongyang/qemu-colo/commits/colo_framework_v7.2

Hi,
  I seem to be having problems with the new listed syntax on the wiki;
on the secondary I'm getting the error

 Block format 'replication' used by device 'virtio0' doesn't support the option 
'export'

./try/bin/qemu-system-x86_64 -enable-kvm -nographic \
 -boot c -m 4096 -smp 4 -S \
 -name debug-threads=on -trace events=trace-file \
 -netdev tap,id=hn0,script=$PWD/ifup-slave,\
downscript=no,colo_script=$PWD/qemu/scripts/colo-proxy-script.sh,colo_nicname=em4
 \
 -device e1000,mac=9c:da:4d:1c:b5:89,id=net-pci0,netdev=hn0 \
 -device virtio-rng-pci \
 -drive 
if=none,driver=raw,file=/home/localvms/bugzilla.raw,id=colo1,cache=none,aio=native
 \
 -drive 
if=virtio,driver=replication,mode=secondary,export=colo1,throttling.bps-total-max=7000,\
file.file.filename=$TMPDISKS/colo-active-disk.qcow2,\
file.driver=qcow2,\
file.backing.file.filename=$TMPDISKS/colo-hidden-disk.qcow2,\
file.backing.driver=qcow2,\
file.backing.backing.backing_reference=colo1,\
file.backing.allow-write-backing-file=on \
 -incoming tcp:0:

This is using 6fd6ce32 from the colo_framework_v7.2  tag.

What have I missed?

Dave
P.S. the name 'file.backing.backing.backing_reference' is not nice!

 
 TODO:
 1. Continuous block replication. It will be started after basic functions
are accepted.
 
 Changs Log:
 V7:
 1. Implement adding/removing quorum child. Remove the option non-connect.
 2. Simplify the backing refrence option according to Stefan Hajnoczi's 
 suggestion
 V6:
 1. Rebase to the newest qemu.
 V5:
 1. Address the comments from Gong Lei
 2. Speed the failover up. The secondary vm can take over very quickly even
if there are too many I/O requests.
 V4:
 1. Introduce a new driver replication to avoid touch nbd and qcow2.
 V3:
 1: use error_setg() instead of error_set()
 2. Add a new block job API
 3. Active disk, hidden disk and nbd target uses the same AioContext
 4. Add a testcase to test new hbitmap API
 V2:
 1. Redesign the secondary qemu(use image-fleecing)
 2. Use Error objects to return error message
 3. Address the comments from Max Reitz and Eric Blake
 
 
 Wen Congyang (17):
   Add new block driver interface to add/delete a BDS's child
   quorum: implement block driver interfaces add/delete a BDS's child
   hmp: add monitor command to add/remove a child
   introduce a new API qemu_opts_absorb_qdict_by_index()
   quorum: allow ignoring child errors
   introduce a new API to enable/disable attach device model
   introduce a new API to check if blk is attached
   block: make bdrv_put_ref_bh_schedule() as a public API
   Backup: clear all bitmap when doing block checkpoint
   allow writing to the backing file
   Allow creating backup jobs when opening BDS
   block: Allow references for backing files
   docs: block replication's description
   Add new block driver interfaces to control block replication
   skip nbd_target when starting block replication
   quorum: implement block driver interfaces for block replication
   Implement new driver for block replication
 
  block.c| 198 +-
  block/Makefile.objs|   3 +-
  block/backup.c |  13 ++
  block/block-backend.c  |  33 +++
  block/quorum.c | 244 ++-
  block/replication.c| 443 
 +
  blockdev.c |  90 ++---
  blockjob.c |  10 +
  docs/block-replication.txt | 179 +
  hmp-commands.hx|  28 +++
  include/block/block.h  |  11 +
  include/block/block_int.h  |  19 ++
  include/block/blockjob.h   |  12 ++
  include/qemu/option.h  |   2 +
  include/sysemu/block-backend.h |   3 +
  include/sysemu/blockdev.h  |   2 +
  qapi/block.json|  16 ++
  util/qemu-option.c |  44 
  18 files changed, 1303 insertions(+), 47 deletions(-)
  create mode 100644 block/replication.c
  create mode 100644 docs/block-replication.txt
 
 -- 
 2.4.3
 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[John Snow]

On 07/03/2015 09:34 AM, Markus Armbruster wrote:
 John Snow js...@redhat.com writes:
 
 Respect the default drive type as proffered via the CLI.

 This patch overloads the drive out parameter of pick_geometry
 to be used as a default hint which is offered by fd_revalidate.

 Signed-off-by: John Snow js...@redhat.com
 ---
  hw/block/fdc.c | 27 +--
  1 file changed, 25 insertions(+), 2 deletions(-)

 diff --git a/hw/block/fdc.c b/hw/block/fdc.c
 index 1023a01..87fd770 100644
 --- a/hw/block/fdc.c
 +++ b/hw/block/fdc.c
 @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int 
 *nb_heads,
  /* Pick a default drive type if there's no media inserted AND we have
   * not yet announced our drive type to the CMOS. */
  if (!blk_is_inserted(blk)  drive_in == FDRIVE_DRV_NONE) {
 -parse = fd_formats[0];
 +for (i = 0; ; i++) {
 +parse = fd_formats[i];
 +if ((parse-drive == FDRIVE_DRV_NONE) ||
 +(parse-drive == *drive)) {
 +break;
 +}
 +}
  goto out;
  }
  
 
 Before the patch: if we have no medium, and drv-drive (a.k.a. drive_in)
 is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5 1.44MiB).
 

My commit message is actually wrong, the code actually picks format #1,
but that's only useful for choosing the drive type while there's no
floppy. As soon as one is inserted it will re-validate to the closest
1.44MB type.

 Afterwards: pick first one matching the value of
 get_default_drive_type(), i.e. the value of the new property.
 
 So the property is really a default type, which applies only when we
 start without a medium in the drive.
 
 Is that what we want?
 

It is a minimal change that just allows you to configure what it
defaults back to. It's a 'weak' setting.

Was that a good call? hmm...

 When I specifically ask for a 5.25 1.2MiB drive, I'd be rather
 surprised when it silently morphs into a 3.5 2.88MiB drive just because
 I've forced a funny medium in before startup.
 

Ah, here it is. I can just add typeA and typeB properties instead of
defaultA/B, and force the drive into that role.

 The obvious way to do drive types is selecting one with a property,
 defaulting to the most common type, i.e. standard 3.5 1.44Mib.
 

Hm?

Not sure I follow, but the goal here is to use the 2.88MB type, because
it can accept both kinds of diskettes, so it has the greatest
compatibility for floppy images (including the virtio driver diskette
which is a 2.88MB type.)

The problem is the 1.44MB drive type and the current inflexibility of
the revalidate+pick_geometry algorithm that doesn't allow you to change
from 1.44 to 2.88 or vice versa.

 To preserve backward compatibility, we need a way to say pick one for
 the medium, else pick standard, and we need to make it the default.  At
 least for old machine types.
 
 Opinions?
 

Machines prior to (let's say 2.5 here) should use something close to the
old behavior: Choose 1.44MB if no diskette, pick the best match (by
sector count) otherwise.

New machines apply nearly the same behavior, except they opt for 2.88MB
as the default.

New properties allow users to override the default-selection behavior
and/or just force a drive type.

 @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
  fd_seek(drv, 0, 0, 1, 1);
  }
  
 +static FDriveType get_default_drive_type(FDrive *drv);
 +
  /* Revalidate a disk drive after a disk change */
  static void fd_revalidate(FDrive *drv)
  {
  int nb_heads, max_track, last_sect, ro;
 -FDriveType drive;
 +FDriveType drive = get_default_drive_type(drv);
  FDriveRate rate;
  
  FLOPPY_DPRINTF(revalidate\n);
 @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
  int32_t bootindexB;
  } FDCtrlISABus;
  
 +static FDriveType get_default_drive_type(FDrive *drv)
 +{
 +FDCtrl *fdctrl = drv-fdctrl;
 +
 +if (0  MAX_FD  (fdctrl-drives[0] == drv)) {
 +return fdctrl-defaultA;
 +}
 +
 +if (1  MAX_FD  (fdctrl-drives[1] == drv)) {
 +return fdctrl-defaultB;
 +}
 +
 +return FDRIVE_DEFAULT;
 +}
 +
  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
  {
  FDCtrl *fdctrl = opaque;
 
 Why do you need to guard with MAX_FD?  If MAX_FD  2, surely the
 properties don't exist, and fdctrl-drives[i] still has its initial
 value FDRIVE_DEFAULT, doesn't it?
 

Why would the properties not exist?

Re: [Qemu-block] [Qemu-devel] [RFC 08/10] fdc: refactor pick_geometry

[John Snow]

On 07/03/2015 09:45 AM, Markus Armbruster wrote:
 John Snow js...@redhat.com writes:
 
 This one is the crazy one.
 
 I'm afraid I don't have the mental capacity to properly review this one
 right now.
 
 From what I've understood of your series so far, it's a strict decrease
 of fdc craziness.  Thanks!
 

Yup, this is the one that definitely changes the behavior a little, and
the hard part in reviewing this is (I hope) understanding how the old
code works.

Hopefully the new version is a little more explicit in how it guesses.

Thanks again for looking.

--js

Re: [Qemu-block] NVMe volatile write cache fixes

[Christoph Hellwig]

On Wed, Jun 17, 2015 at 02:24:06PM +0200, Kevin Wolf wrote:
 Am 17.06.2015 um 13:59 hat Christoph Hellwig geschrieben:
  Thank Eric.
  
  Kevin,
  
  do you want me to resend the series with these cover letter/cc fixes or
  is it okay this time?
 
 It's not worth a respin. I'm just waiting for an Acked-by (or whatever
 other outcome of a review) from Keith before I merge this.

Given that you got the Ack a while ago is there any chance to get this data
integrity fix in before qemu 2.4?