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

Re: [libvirt] [PATCH v3] Add support for Veritas HyperScale (VxHS) block device protocol




On 01/27/2017 08:43 PM, ashish mittal wrote:
> Thanks for the review!
> 
> My inputs on some of the comments -
> 
> On Wed, Jan 25, 2017 at 7:59 AM, John Ferlan <jferlan redhat com> wrote:
>>
>>
>> On 01/19/2017 09:21 PM, Ashish Mittal wrote:
>>> Sample XML for a vxhs vdisk is as follows:
>>>
>>> <disk type='network' device='disk'>
>>>   <driver name='qemu' type='raw' cache='none'/>
>>>   <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
>>>     <host name='192.168.0.1' port='9999'/>
>>>   </source>
>>>   <backingStore/>
>>>   <target dev='vda' bus='virtio'/>
>>>   <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
>>>   <alias name='virtio-disk0'/>
>>>   <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
>>> function='0x0'/>
>>> </disk>
>>
>> It's still not really clear how someone knows to use the name string.
>> IOW: How would someone know what to supply. Perhaps the libvirt wiki
>> (http://wiki.libvirt.org/page/Main_Page) as opposed to the libvirt docs.
>>
>> I assume that someone using VxHS knows how to get that, but still I find
>> it a "good thing" to be able to have that description somewhere as I can
>> only assume some day it'll come up.
>>
>> But since we don't "require" this for iSCSI, Ceph/RBD, Gluster, etc.
>> it's not something 'required' for this patch. Still something for
>> someone's todo list to get a description on the libvirt wiki. Once you
>> have wiki write access, then you can always keep that data up to date
>> without requiring libvirt patches.
>>
>>>
>>> Signed-off-by: Ashish Mittal <Ashish Mittal veritas com>
>>> ---
>>> v2 changelog:
>>> (1) Added code for JSON parsing of a VxHS vdisk.
>>> (2) Added test case to verify JSON parsing.
>>> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
>>> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
>>>
>>> v3 changelog:
>>> (1) Implemented the modern syntax for VxHS disk specification.
>>> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
>>> (3) Added a negative test case to check failure when multiple hosts
>>>     are specified for a VxHS disk.
>>>
>>>  docs/formatdomain.html.in                          | 15 ++++-
>>>  docs/schemas/domaincommon.rng                      |  1 +
>>>  src/libxl/libxl_conf.c                             |  1 +
>>>  src/qemu/qemu_command.c                            | 68 ++++++++++++++++++++++
>>>  src/qemu/qemu_driver.c                             |  3 +
>>>  src/qemu/qemu_parse_command.c                      | 26 +++++++++
>>>  src/util/virstoragefile.c                          | 63 +++++++++++++++++++-
>>>  src/util/virstoragefile.h                          |  1 +
>>>  src/xenconfig/xen_xl.c                             |  1 +
>>>  ...xml2argv-disk-drive-network-vxhs-multi-host.xml | 35 +++++++++++
>>>  .../qemuxml2argv-disk-drive-network-vxhs.args      | 25 ++++++++
>>>  .../qemuxml2argv-disk-drive-network-vxhs.xml       | 34 +++++++++++
>>>  tests/qemuxml2argvtest.c                           |  2 +
>>>  tests/virstoragetest.c                             | 16 +++++
>>>  14 files changed, 287 insertions(+), 4 deletions(-)
>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml
>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>>>
>>
>> In general things are looking much better - a couple of nits below and 2
>> things to consider/rework...
>>
>> #1. Based on Peter's v2 comments, we don't want to support the
>> older/legacy syntax for VxHS, so it's something that should be removed -
>> although we should check for it being present and fail if found.
>>
> 
> I am testing with changed code to return error if legacy syntax is
> found for VxHS. Also added a test case to check for failure on legacy
> syntax and it seems to pass (test #41 below).
> 
> Then I added a pass test case to check conversion from new native
> syntax to XML (test #40 below). That test fails with error
> 'qemuParseCommandLineDisk:901 : internal error: missing file parameter
> in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...'

The qemu_parse_command.c changes while nice to have weren't even updated
when multiple gluster servers were added (e.g. commit id '' or '7b7da9e28')
Check the changes to add the new s

IOW: This code knows how to parse something like:

-drive
'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,format=raw,if=none,id=drive-virtio-disk1'

but it's clueless for:

-drive file.driver=gluster,file.volume=Volume3,file.path=/Image.qcow2,\
file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\
file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\
file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\
if=none,id=drive-virtio-disk2 \
-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk2,\
id=virtio-disk2

See
> 
> Looks like none of the existing tests in qemuargv2xmltest test for the
> parsing of new syntax, and qemuParseCommandLineDisk() expects to find
> 'file=' for a drive or it errors out. If this is true, will it be able
> to parse the new syntax? Some help here please!
> 
> Output from the newly added test cases (40 should pass and 41 checks
> for error) :
> 
> 40) QEMU ARGV-2-XML disk-drive-network-vxhs
> ... Got unexpected warning from qemuParseCommandLineString:
> 2017-01-28 00:57:30.814+0000: 10391: info : libvirt version: 3.0.0
> 2017-01-28 00:57:30.814+0000: 10391: info : hostname: localhost.localdomain
> 2017-01-28 00:57:30.814+0000: 10391: error :
> qemuParseCommandLineDisk:901 : internal error: missing file parameter
> in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none'
> libvirt: QEMU Driver error : internal error: missing file parameter in
> drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none'
> FAILED
> 
> 41) QEMU ARGV-2-XML disk-drive-network-vxhs-fail
> ... Got expected error from qemuParseCommandLineString:
> libvirt: QEMU Driver error : internal error: VxHS protocol does not
> support URI syntax
> 'vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251'
> OK
> 42) QEMU ARGV-2-XML disk-usb                                          ... OK
> 
> 
> 
>> #2. Is the desire to ever support more than 1 host? If not, then is the
>> "server" syntax you've borrowed from the Gluster code necessary? Could
>> you just go with the single "host" like NBD and SSH. As it relates to
>> the qemu command line - I'm not quite as clear. From the example I see
>> in commit id '7b7da9e28', the gluster syntax would have:
>>
> 
> Present understanding is to have only one host. You are right, the
> "server" part is not necessary. Will have to check with the qemu
> community on this change.
> 
>> +file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\
>> +file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\
>> +file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\
>>
>> whereas, the VxHS syntax is:
>>  +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\
>>
>> FWIW: I also note there is no ".type=tcp" in your output - so perhaps
>> the "default" is tcp unless otherwise specified, but I'm sure of the
>> qemu syntax requirements in this area. I assume that since there's only
>> 1 server, the ".0, .1, .2" become unnecessary (something added by commit
>> id 'f1bbc7df4' for multiple gluster hosts).
>>
> 
> That's correct. TCP is the default.
> 
>> I haven't closedly followed the qemu syntax discussion, but it would it
>> would be possible to use:
>>
>> +file.host=192.168.0.1,file.port=9999
>>
> 
> That is correct. Above syntax would also work for us. I will pose this
> suggestion to the qemu community and update with their response.
> 
>> Similar to how NBD (see commit id 'a1674fd9') and SSH (see commit id
>> 'bc225b1b5') are handled.
>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index f7bef51..2a071c9 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -2319,9 +2319,9 @@
>>>                <dd>
>>>                The <code>protocol</code> attribute specifies the protocol to
>>>                access to the requested image. Possible values are "nbd",
>>> -              "iscsi", "rbd", "sheepdog" or "gluster".  If the
>>> -              <code>protocol</code> attribute is "rbd", "sheepdog" or
>>> -              "gluster", an additional attribute <code>name</code> is
>>> +              "iscsi", "rbd", "sheepdog", "gluster" or "vxhs".  If the
>>> +              <code>protocol</code> attribute is "rbd", "sheepdog", "gluster"
>>> +              or "vxhs", an additional attribute <code>name</code> is
>>>                mandatory to specify which volume/image will be used. For "nbd",
>>>                the <code>name</code> attribute is optional. For "iscsi"
>>>                (<span class="since">since 1.0.4</span>), the <code>name</code>
>>> @@ -2329,6 +2329,9 @@
>>>                target's name by a slash (e.g.,
>>>                <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not
>>>                specified, the default LUN is zero.
>>> +              For "vxhs" (<span class="since">since 2.5.0</span>), the
>>
>> Next release is 3.1.0
>>
>>> +              <code>name</code> is the UUID of the volume, assigned by the
>>> +              HyperScale sever.
>>>                <span class="since">Since 0.8.7</span>
>>>                </dd>
>>>              <dt><code>volume</code></dt>
>>> @@ -2431,6 +2434,12 @@
>>>                  <td> one or more (<span class="since">Since 2.1.0</span>), just one prior to that </td>
>>>                  <td> 24007 </td>
>>>                </tr>
>>> +              <tr>
>>> +                <td> vxhs </td>
>>> +                <td> a server running Veritas HyperScale daemon </td>
>>> +                <td> only one </td>
>>> +                <td> 9999 </td>
>>> +              </tr>
>>>              </table>
>>>              <p>
>>>              gluster supports "tcp", "rdma", "unix" as valid values for the
>>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>>> index 4d76315..cdc39ca 100644
>>> --- a/docs/schemas/domaincommon.rng
>>> +++ b/docs/schemas/domaincommon.rng
>>> @@ -1470,6 +1470,7 @@
>>>              <value>ftp</value>
>>>              <value>ftps</value>
>>>              <value>tftp</value>
>>> +            <value>vxhs</value>
>>>            </choice>
>>>          </attribute>
>>>          <optional>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index b569dda..7e12d32 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -604,6 +604,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
>>>      case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
>>>      case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>>>      case VIR_STORAGE_NET_PROTOCOL_SSH:
>>> +    case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>      case VIR_STORAGE_NET_PROTOCOL_LAST:
>>>      case VIR_STORAGE_NET_PROTOCOL_NONE:
>>>          virReportError(VIR_ERR_NO_SUPPORT,
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index f8e48d2..08bad8e 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -491,6 +491,9 @@ qemuNetworkDriveGetPort(int protocol,
>>>              /* no default port specified */
>>>              return 0;
>>>
>>> +        case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>> +            return 9999;
>>> +
>>>          case VIR_STORAGE_NET_PROTOCOL_RBD:
>>>          case VIR_STORAGE_NET_PROTOCOL_LAST:
>>>          case VIR_STORAGE_NET_PROTOCOL_NONE:
>>> @@ -899,6 +902,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
>>>  }
>>>
>>>
>>> +#define QEMU_DEFAULT_VXHS_PORT "9999"
>>> +
>>> +/* Build the VxHS host object */
>>> +static virJSONValuePtr
>>> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
>>> +{
>>> +    virJSONValuePtr server = NULL;
>>> +    virStorageNetHostDefPtr host;
>>> +    const char *portstr;
>>> +
>>> +    if (src->nhosts != 1) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                       _("VxHS supports only one server"));
>>
>> make syntax-check failure here - need a format, e.g.
>>
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    host = src->hosts;
>>> +    portstr = host->port;
>>> +
>>> +    if (!portstr)
>>> +        portstr = QEMU_DEFAULT_VXHS_PORT;
>>> +
>>> +    if (virJSONValueObjectCreate(&server,
>>> +                                 "s:host", host->name,
>>> +                                 "s:port", portstr,
>>> +                                 NULL) < 0)
>>> +        server = NULL;
>>> +
>>> + cleanup:
>>> +    return server;
>>> +}
>>> +
>>> +
>>> +static virJSONValuePtr
>>> +qemuBuildVxHSDriveJSON(virStorageSourcePtr src)
>>> +{
>>> +    const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
>>> +    virJSONValuePtr server = NULL;
>>> +    virJSONValuePtr ret = NULL;
>>> +
>>> +    if (!(server = qemuBuildVxHSDriveJSONHost(src)))
>>> +        return NULL;
>>> +
>>> +    /* VxHS disk sepecification example:
>>
>> "specification"
>>
>>> +     * { driver:"vxhs",
>>> +     *   vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
>>> +     *   server.host:"1.2.3.4",
>>> +     *   server.port:1234}
>>> +     */
>>> +    if (virJSONValueObjectCreate(&ret,
>>> +                                 "s:driver", protocol,
>>> +                                 "s:vdisk-id", src->path,
>>> +                                 "a:server", server, NULL) < 0)
>>
>> If you're going with the 1 host only model, then I believe this isn't
>> necessary. For the Gluster code there's a difference based solely on
>> whether >1 hosts are present whether the "server:" syntax is added or
>> the legacy syntax is used.
>>
>> If you're not ever going to allow multiple hosts, the "server" option
>> would seem to be unnecessary...  Rather I would think you should just be
>> able to add "s:host" and "s:port" type options.
>>
>>> +        virJSONValueFree(server);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +
>>>  static char *
>>>  qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>>>                           qemuDomainSecretInfoPtr secinfo)
>>> @@ -1034,6 +1096,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>>>          case VIR_STORAGE_NET_PROTOCOL_TFTP:
>>>          case VIR_STORAGE_NET_PROTOCOL_ISCSI:
>>>          case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
>>> +        case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>              ret = qemuBuildNetworkDriveURI(src, secinfo);
>>>              break;
>>>
>>> @@ -1148,6 +1211,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src,
>>>              if (!(fileprops = qemuBuildGlusterDriveJSON(src)))
>>>                  return -1;
>>>          }
>>> +
>>> +        if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) {
>>> +            if (!(fileprops = qemuBuildVxHSDriveJSON(src)))
>>> +                return -1;
>>> +        }
>>>          break;
>>>      }
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index b359e77..c76e9d4 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -13788,6 +13788,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk)
>>>          case VIR_STORAGE_NET_PROTOCOL_FTPS:
>>>          case VIR_STORAGE_NET_PROTOCOL_TFTP:
>>>          case VIR_STORAGE_NET_PROTOCOL_SSH:
>>> +        case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>          case VIR_STORAGE_NET_PROTOCOL_LAST:
>>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>>                             _("external inactive snapshots are not supported on "
>>> @@ -13851,6 +13852,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
>>>          case VIR_STORAGE_NET_PROTOCOL_FTPS:
>>>          case VIR_STORAGE_NET_PROTOCOL_TFTP:
>>>          case VIR_STORAGE_NET_PROTOCOL_SSH:
>>> +        case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>          case VIR_STORAGE_NET_PROTOCOL_LAST:
>>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>>                             _("external active snapshots are not supported on "
>>> @@ -13996,6 +13998,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn,
>>>          case VIR_STORAGE_NET_PROTOCOL_FTPS:
>>>          case VIR_STORAGE_NET_PROTOCOL_TFTP:
>>>          case VIR_STORAGE_NET_PROTOCOL_SSH:
>>> +        case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>          case VIR_STORAGE_NET_PROTOCOL_LAST:
>>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>>                             _("internal inactive snapshots are not supported on "
>>> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
>>> index 405e655..aab287a 100644
>>> --- a/src/qemu/qemu_parse_command.c
>>> +++ b/src/qemu/qemu_parse_command.c
>>> @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk)
>>>      return -1;
>>>  }
>>>
>>> +static int
>>> +qemuParseVxHSString(virDomainDiskDefPtr def)
>>> +{
>>> +    virURIPtr uri = NULL;
>>> +
>>> +    if (!(uri = virURIParse(def->src->path)))
>>> +        return -1;
>>> +
>>> +    return qemuParseDriveURIString(def, uri, "vxhs");
>>> +}
>>> +
>>>
>>>  /*
>>>   * This method takes a string representing a QEMU command line ARGV set
>>> @@ -737,6 +748,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>>>                          if (VIR_STRDUP(def->src->path, vdi) < 0)
>>>                              goto error;
>>>                      }
>>> +                } else if (STRPREFIX(def->src->path, "vxhs:")) {
>>> +                    def->src->type = VIR_STORAGE_TYPE_NETWORK;
>>> +                    def->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
>>> +
>>> +                    if (qemuParseVxHSString(def) < 0)
>>> +                        goto error;
>>>                  } else {
>>>                      def->src->type = VIR_STORAGE_TYPE_FILE;
>>>                  }
>>> @@ -1947,6 +1964,10 @@ qemuParseCommandLine(virCapsPtr caps,
>>>                  disk->src->type = VIR_STORAGE_TYPE_NETWORK;
>>>                  disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG;
>>>                  val += strlen("sheepdog:");
>>> +            } else if (STRPREFIX(val, "vxhs:")) {
>>> +                disk->src->type = VIR_STORAGE_TYPE_NETWORK;
>>> +                disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
>>> +                val += strlen("vxhs:");
>>>              } else {
>>>                  disk->src->type = VIR_STORAGE_TYPE_FILE;
>>>              }
>>> @@ -2023,6 +2044,11 @@ qemuParseCommandLine(virCapsPtr caps,
>>>                          goto error;
>>>
>>>                      break;
>>> +                case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>> +                    if (qemuParseVxHSString(disk) < 0)
>>> +                        goto error;
>>> +
>>> +                    break;
>>>                  case VIR_STORAGE_NET_PROTOCOL_HTTP:
>>>                  case VIR_STORAGE_NET_PROTOCOL_HTTPS:
>>>                  case VIR_STORAGE_NET_PROTOCOL_FTP:
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index ce6d213..e62f4e6 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -85,7 +85,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST,
>>>                "ftp",
>>>                "ftps",
>>>                "tftp",
>>> -              "ssh")
>>> +              "ssh",
>>> +              "vxhs")
>>>
>>>  VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST,
>>>                "tcp",
>>> @@ -2633,6 +2634,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
>>>      case VIR_STORAGE_NET_PROTOCOL_ISCSI:
>>>      case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
>>>      case VIR_STORAGE_NET_PROTOCOL_SSH:
>>> +    case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>          virReportError(VIR_ERR_INTERNAL_ERROR,
>>>                         _("malformed backing store path for protocol %s"),
>>>                         protocol);
>>> @@ -2964,6 +2966,64 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src,
>>>  }
>>>
>>>
>>> +static int
>>> +virStorageSourceParseBackingJSONVXHS(virStorageSourcePtr src,
>>
>> s/VXHS/VxHS (to be consistent)
>>
>>> +                                    virJSONValuePtr json,
>>> +                                    int opaque ATTRIBUTE_UNUSED)
>>
>> These two arguments need to be aligned under the 'v' instead of under
>> the '('
>>
>>> +{
>>> +    virJSONValuePtr server;
>>> +    const char *hostname, *port;
>>
>> Single lines for each and follow existing model - e.g. move the lines
>> below setting these to here.
>>
>>> +    const char *uri = virJSONValueObjectGetString(json, "filename");
>>> +    const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
>>> +
>>> +    src->type = VIR_STORAGE_TYPE_NETWORK;
>>> +    src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
>>> +
>>> +    /* legacy URI based syntax passed via 'filename' option */
>>> +    if (uri)
>>> +        return virStorageSourceParseBackingJSONUriStr(src, uri,
>>> +                                                      VIR_STORAGE_NET_PROTOCOL_VXHS);
>>
>> Since we don't want to support the legacy option - this becomes a
>> failure path with some sort of error indicating unsupported format.
>>
>> For the other protocols, the legacy exists because 'filename' existed
>> prior to when the "new" syntax support was added so both need to be
>> supported.
>>
>>> +
>>> +    server = virJSONValueObjectGetObject(json, "server");
>>> +    hostname = virJSONValueObjectGetString(server, "host");
>>> +    port = virJSONValueObjectGetString(server, "port");
>>
>> Each of these should be set above similar to how each of the other
>> helpers do this. Although whether "server" is necessary depends on the
>> multiple hosts decision. The *JSONGluster uses it because the legacy
>> syntax supports only one "host", while the non legacy syntax supports
>> having multiple hosts (although it's really not well described in/from
>> commit id '7b7da9e2833").
>>
>>> +
>>> +    if (!vdisk_id) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                       _("missing 'vdisk-id' attribute in "
>>> +                         "JSON backing definition for VxHS volume"));
>>> +        return -1;
>>> +    }
>>> +    if (!server) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                       _("missing 'server' attribute in "
>>> +                         "JSON backing definition for VxHS volume"));
>>> +        return -1;
>>> +    }
>>> +    if (!hostname) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                       _("missing hostname for tcp backing server in "
>>> +                       "JSON backing definition for VxHS volume"));
>>> +        return -1;
>>> +    }
>>
>> You could combine these to be like others and just have one error message.
>>
>>> +
>>> +
>>> +    if (VIR_STRDUP(src->path, vdisk_id) < 0)
>>> +        return -1;
>>> +
>>> +    if (VIR_ALLOC_N(src->hosts, 1) < 0)
>>> +        return -1;
>>> +    src->nhosts = 1;
>>> +
>>> +    src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
>>> +
>>> +    if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 ||
>>> +        VIR_STRDUP(src->hosts[0].port, port) < 0)
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  struct virStorageSourceJSONDriverParser {
>>>      const char *drvname;
>>>      int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque);
>>> @@ -2985,6 +3045,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = {
>>>      {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0},
>>>      {"ssh", virStorageSourceParseBackingJSONSSH, 0},
>>>      {"rbd", virStorageSourceParseBackingJSONRBD, 0},
>>> +    {"vxhs", virStorageSourceParseBackingJSONVXHS, 0},
>>>  };
>>>
>>>
>>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>>> index 1f62244..e60b6e9 100644
>>> --- a/src/util/virstoragefile.h
>>> +++ b/src/util/virstoragefile.h
>>> @@ -134,6 +134,7 @@ typedef enum {
>>>      VIR_STORAGE_NET_PROTOCOL_FTPS,
>>>      VIR_STORAGE_NET_PROTOCOL_TFTP,
>>>      VIR_STORAGE_NET_PROTOCOL_SSH,
>>> +    VIR_STORAGE_NET_PROTOCOL_VXHS,
>>>
>>>      VIR_STORAGE_NET_PROTOCOL_LAST
>>>  } virStorageNetProtocol;
>>> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
>>> index 18d9fe3..9fe24d6 100644
>>> --- a/src/xenconfig/xen_xl.c
>>> +++ b/src/xenconfig/xen_xl.c
>>> @@ -942,6 +942,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src)
>>>      case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
>>>      case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>>>      case VIR_STORAGE_NET_PROTOCOL_SSH:
>>> +    case VIR_STORAGE_NET_PROTOCOL_VXHS:
>>>      case VIR_STORAGE_NET_PROTOCOL_LAST:
>>>      case VIR_STORAGE_NET_PROTOCOL_NONE:
>>>          virReportError(VIR_ERR_NO_SUPPORT,
>>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml
>>> new file mode 100644
>>> index 0000000..660bcde
>>> --- /dev/null
>>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml
>>> @@ -0,0 +1,35 @@
>>> +<domain type='qemu'>
>>> +  <name>QEMUGuest1</name>
>>> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>>> +  <memory unit='KiB'>219136</memory>
>>> +  <currentMemory unit='KiB'>219136</currentMemory>
>>> +  <vcpu placement='static'>1</vcpu>
>>> +  <os>
>>> +    <type arch='i686' machine='pc'>hvm</type>
>>> +    <boot dev='hd'/>
>>> +  </os>
>>> +  <clock offset='utc'/>
>>> +  <on_poweroff>destroy</on_poweroff>
>>> +  <on_reboot>restart</on_reboot>
>>> +  <on_crash>destroy</on_crash>
>>> +  <devices>
>>> +    <emulator>/usr/libexec/qemu-kvm</emulator>
>>> +    <disk type='network' device='disk'>
>>> +      <driver name='qemu' type='raw' cache='none'/>
>>> +      <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
>>> +        <host name='192.168.0.1' port='9999'/>
>>> +        <host name='192.168.0.2' port='9999'/>
>>> +      </source>
>>> +      <backingStore/>
>>> +      <target dev='vda' bus='virtio'/>
>>> +      <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
>>> +      <alias name='virtio-disk0'/>
>>> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>>> +    </disk>
>>> +    <controller type='usb' index='0'/>
>>> +    <controller type='pci' index='0' model='pci-root'/>
>>> +    <input type='mouse' bus='ps2'/>
>>> +    <input type='keyboard' bus='ps2'/>
>>> +    <memballoon model='none'/>
>>> +  </devices>
>>> +</domain>
>>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
>>> new file mode 100644
>>> index 0000000..23045e1
>>> --- /dev/null
>>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
>>> @@ -0,0 +1,25 @@
>>> +LC_ALL=C \
>>> +PATH=/bin \
>>> +HOME=/home/test \
>>> +USER=test \
>>> +LOGNAME=test \
>>> +QEMU_AUDIO_DRV=none \
>>> +/usr/libexec/qemu-kvm \
>>> +-name QEMUGuest1 \
>>> +-S \
>>> +-M pc \
>>> +-cpu qemu32 \
>>> +-m 214 \
>>> +-smp 1,sockets=1,cores=1,threads=1 \
>>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>>> +-nographic \
>>> +-nodefaults \
>>> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
>>> +-no-acpi \
>>> +-boot c \
>>> +-usb \
>>> +-drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
>>> +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\
>>> +id=drive-virtio-disk0,cache=none \
>>> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
>>> +id=virtio-disk0
>>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>>> new file mode 100644
>>> index 0000000..45c807f
>>> --- /dev/null
>>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>>> @@ -0,0 +1,34 @@
>>> +<domain type='qemu'>
>>> +  <name>QEMUGuest1</name>
>>> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>>> +  <memory unit='KiB'>219136</memory>
>>> +  <currentMemory unit='KiB'>219136</currentMemory>
>>> +  <vcpu placement='static'>1</vcpu>
>>> +  <os>
>>> +    <type arch='i686' machine='pc'>hvm</type>
>>> +    <boot dev='hd'/>
>>> +  </os>
>>> +  <clock offset='utc'/>
>>> +  <on_poweroff>destroy</on_poweroff>
>>> +  <on_reboot>restart</on_reboot>
>>> +  <on_crash>destroy</on_crash>
>>> +  <devices>
>>> +    <emulator>/usr/libexec/qemu-kvm</emulator>
>>> +    <disk type='network' device='disk'>
>>> +      <driver name='qemu' type='raw' cache='none'/>
>>> +      <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
>>> +        <host name='192.168.0.1' port='9999'/>
>>> +      </source>
>>> +      <backingStore/>
>>> +      <target dev='vda' bus='virtio'/>
>>> +      <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
>>> +      <alias name='virtio-disk0'/>
>>> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>>> +    </disk>
>>> +    <controller type='usb' index='0'/>
>>> +    <controller type='pci' index='0' model='pci-root'/>
>>> +    <input type='mouse' bus='ps2'/>
>>> +    <input type='keyboard' bus='ps2'/>
>>> +    <memballoon model='none'/>
>>> +  </devices>
>>> +</domain>
>>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>>> index ab3ad08..f751ec9 100644
>>> --- a/tests/qemuxml2argvtest.c
>>> +++ b/tests/qemuxml2argvtest.c
>>> @@ -892,6 +892,8 @@ mymain(void)
>>>  # endif
>>>      DO_TEST("disk-drive-network-rbd-ipv6", NONE);
>>>      DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE);
>>> +    DO_TEST("disk-drive-network-vxhs", NONE);
>>> +    DO_TEST_FAILURE("disk-drive-network-vxhs-multi-host", NONE);
>>>      DO_TEST("disk-drive-no-boot",
>>>              QEMU_CAPS_BOOTINDEX);
>>>      DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",
>>> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
>>> index f766df1..a28fbd9 100644
>>> --- a/tests/virstoragetest.c
>>> +++ b/tests/virstoragetest.c
>>> @@ -1492,6 +1492,22 @@ mymain(void)
>>>                         "<source protocol='rbd' name='testshare'>\n"
>>>                         "  <host name='example.com'/>\n"
>>>                         "</source>\n");
>>> +    TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\","
>>> +                             "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\""
>>> +                            "}",
>>> +                       "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
>>> +                       "  <host name='192.168.0.1' port='9999'/>\n"
>>> +                       "</source>\n");
>>> +    TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\","
>>> +                                       "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\","
>>> +                                       "\"server\": { \"host\":\"example.com\","
>>> +                                                      "\"port\":\"1234\""
>>> +                                                   "}"
>>> +                                      "}"
>>> +                            "}",
>>> +                       "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
>>> +                       "  <host name='example.com' port='1234'/>\n"
>>> +                       "</source>\n");
>>
>> Since the desire is to not support the older "driver=vxhs,..." syntax
>> that means this second example would result in an error.
>>
>> If you wanted to "check" that it does error, then create a
>> TEST_BACKING_PARSE_ERROR macro that *expects* virTestRun to fail and use
>> the older syntax with that.
>>
>> John
>>
>>>
>>>   cleanup:
>>>      /* Final cleanup */
>>>


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