[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 03/11] conf: Parse and for the tray attribute

On 03/23/2012 05:29 PM, Daniel Veillard wrote:
On Fri, Mar 23, 2012 at 05:48:32PM +0800, Osier Yang wrote:
On 03/23/2012 04:05 PM, Daniel Veillard wrote:
On Wed, Mar 14, 2012 at 11:26:47PM +0800, Osier Yang wrote:
  The "tray" is only allowed for removable disks, i.e. CDROM and
  Floppy disks.

  As the value for "tray" defaults to "closed", lots of tests are
  updated to include "tray='closed'" in the disk target XML.
   I tend to think "tray='closed'" should be ommited since it's the
default value, no need to serialize it out systematically

Not sure which way is better, but from my point of view,
it's more explicit for a user's eyes to see what the tray
status is, and when updating the device, changing the value
of "tray" between "open" and "closed" is more sensiable than
"adding tray='open'" and "removing tray='open'" (supposing
QEMU will support CD-ROM and Floppy device updating with
tray status in future, e.g. a qemu monitor command "change-tray",
and one uses "virsh edit" to change the XML).

But I acknowledge all these reasons are weak somehow, :-)
so I'm fine with either.

By the way, it seems to me it's not consistent on whether
to print the default value for tags or not, some tags do,
some not. Such as rawio='no' for disk won't be printed,
and managed='no' for hostdevs will be printed.

   Well in this case I would expect tray="closed" 99% of the
time (if not more as on a physical machine !) so to me the
only data worth noticing is tray="open", and actually not
dumping tray="closed" makes the fact that the tray is open
more ovious IMHO.

   So yeah I really think it's better even from an human POV
(well I'm firmly convinced that users should never see XML
  dumps but that's a different story :-)

I pushed with no tray='closed' in domain XML, with indention
nits fixed incidentally. Thanks for the revewing.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]