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

Re: [libvirt] [PATCH] qemu: Allow the user to specify vendor and product for disk



On 2012年11月08日 16:19, Martin Kletzander wrote:
On 11/08/2012 02:52 AM, Dave Allan wrote:
On Thu, Nov 08, 2012 at 09:28:06AM +0800, Osier Yang wrote:
On 2012年11月08日 05:04, Dave Allan wrote:
On Wed, Nov 07, 2012 at 11:24:43PM +0800, Osier Yang wrote:
On 2012年11月05日 21:34, Martin Kletzander wrote:
On 11/05/2012 08:04 AM, Osier Yang wrote:
QEMU supports to set vendor and product strings for disk since
1.2.0 (only scsi-disk, scsi-hd, scsi-cd support it), this patch
exposes it with new XML elements<vendor>    and<product>    of disk
device.
---
  docs/formatdomain.html.in                          |   10 +++++
  docs/schemas/domaincommon.rng                      |   10 +++++
  src/conf/domain_conf.c                             |   30 ++++++++++++++++
  src/conf/domain_conf.h                             |    2 +
  src/qemu/qemu_command.c                            |   29 ++++++++++++++++
  .../qemuxml2argv-disk-scsi-disk-vpd.args           |   13 +++++++
  .../qemuxml2argv-disk-scsi-disk-vpd.xml            |   36 ++++++++++++++++++++
  tests/qemuxml2argvtest.c                           |    4 ++
  8 files changed, 134 insertions(+), 0 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-vpd.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c8da33d..cc9e871 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1657,6 +1657,16 @@
          of 16 hexadecimal digits.
          <span class='since'>Since 0.10.1</span>
        </dd>
+<dt><code>vendor</code></dt>
+<dd>If present, this element specifies the vendor of a virtual hard
+        disk or CD-ROM device. It's a not more than 8 bytes alphanumeric string.

Last sentence doesn't make sense, I suggest changing it to either: "It
can't be longer than 8 alphanumeric characters." or simply "Maximum 8
alphanumeric characters."

Okay, honestly, I wasn't comfortable with the sentence, but can't
get a better one at that time, :-) I will change your advise a bit:

It must not be longer than 8 alphanumeric characters.

Not to be pedantic, but what happens if you hand it doublebyte
characters?

Ah, good question, though QEMU will truncate the string to 8 bytes
anyway, should we do some translation in libvirt? the problem is
how to map the doublebytes vendors/product to single bytes ones,
is there some public specification available? /me starts to think
if it's snake leg (or breaking fly on the wheel) to do the checking
if it's too heavy.

Mostly I was curious about how the code handled doublebyte characters,
but now that I think about it, the SCSI spec mandates 8 bytes of
ASCII[1], but it sounds like qemu is already enforcing that, so I
think it's fine just to let it be freeform and note the spec's
requirement in the documentation.


I'd say if QEMU starts (and truncates or whatever) with invalid vendor
name, there's no problem for us to leave the responsibility on the user,
but OTOH when QEMU won't like what it's doing and will eventually
fix/change that, our machines may not start.  So if we know the
limitation (and it is very easy one), why don't we just check (and
mention it in the docs) that we accept maximum 8 (16) alphanumeric
characters, checking just [A-Za-z0-9] instead of doing some 'isalnum()'
magic.  Check is easy, QEMU will be always satisfied with the option, I
see it as a win-win.


Not really, QEMU won't be happy anyway if it really changes the limits.
But I agree with adding the checking, as per the SCSI spec mandates
8/16 bytes. We don't need to care about the doublebytes then. And a good
reason for the hecking is error out earlier is always good than QEMU
process fails to start. And it's not likely QEMU will change the limits
to violate the spec.

Regards,
Osier


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