<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <p><br>
    </p>
    <br>
    <div class="moz-cite-prefix">On 09/21/2016 06:46 PM, Aline Manera
      wrote:<br>
    </div>
    <blockquote
      cite="mid:fcf5379e-3b0f-937d-0405-15fb5a066d55@linux.vnet.ibm.com"
      type="cite">
      <meta content="text/html; charset=windows-1252"
        http-equiv="Content-Type">
      <br>
      <br>
      <div class="moz-cite-prefix">On 09/21/2016 07:29 AM, Archana Singh
        wrote:<br>
      </div>
      <blockquote
        cite="mid:1def5733-c006-ac1e-2ceb-52d367417cad@linux.vnet.ibm.com"
        type="cite">
        <meta content="text/html; charset=windows-1252"
          http-equiv="Content-Type">
        <p><br>
        </p>
        <br>
        <div class="moz-cite-prefix">On 09/20/2016 08:33 PM, Aline
          Manera wrote:<br>
        </div>
        <blockquote
          cite="mid:6179e6aa-ad9c-29fc-2549-2c0fad1255ba@linux.vnet.ibm.com"
          type="cite">
          <meta content="text/html; charset=windows-1252"
            http-equiv="Content-Type">
          <br>
          <br>
          <div class="moz-cite-prefix">On 09/20/2016 03:41 AM, Archana
            Singh wrote:<br>
          </div>
          <blockquote
            cite="mid:27a3c938-8e03-3f60-2ebf-2685d709aba3@linux.vnet.ibm.com"
            type="cite">
            <meta content="text/html; charset=windows-1252"
              http-equiv="Content-Type">
            <p><br>
            </p>
            <br>
            <div class="moz-cite-prefix">On 09/19/2016 10:19 PM, Aline
              Manera wrote:<br>
            </div>
            <blockquote
              cite="mid:31ba4c43-9a32-ee62-8723-26050fdebc48@linux.vnet.ibm.com"
              type="cite">
              <meta content="text/html; charset=windows-1252"
                http-equiv="Content-Type">
              <br>
              <br>
              <div class="moz-cite-prefix">On 09/19/2016 12:33 PM,
                Harshal Patil wrote:<br>
              </div>
              <blockquote
cite="mid:OF5727B727.96B7C675-ON00258033.0054FCC7-00258033.005575D0@notes.na.collabserv.com"
                type="cite">
                <div class="socmaildefaultfont" dir="ltr"
                  style="font-family:Arial;font-size:10.5pt">
                  <div dir="ltr"> </div>
                  <div dir="ltr">
                    <div class="socmaildefaultfont" dir="ltr"
                      style="font-family:Arial;font-size:10.5pt">
                      <div class="socmaildefaultfont" dir="ltr"
                        style="font-family:Arial;font-size:10.5pt">
                        <div dir="ltr" style="margin-top: 20px;">
                          <div style="font-size: 12pt; font-weight:
                            bold; font-family: sans-serif; color:
                            #7C7C5F;">Harshal Patil</div>
                          <div style="font-size: 8pt; font-family:
                            sans-serif; margin-top: 10px;">
                            <div><span style="font-weight: bold; color:
                                #336699;">E-mail: </span><a
                                moz-do-not-send="true"
                                href="mailto:harshal.patil@in.ibm.com"
                                style="color: #555">harshal.patil@in.ibm.com</a></div>
                          </div>
                        </div>
                      </div>
                    </div>
                  </div>
                  <div dir="ltr"> </div>
                  <div dir="ltr"> </div>
                  <blockquote data-history-content-modified="1"
                    data-history-expanded="1" dir="ltr"
                    style="border-left:solid #aaaaaa 2px;
                    margin-left:5px; padding-left:5px; direction:ltr;
                    margin-right:0px">----- Original message -----<br>
                    From: Aline Manera <a moz-do-not-send="true"
                      class="moz-txt-link-rfc2396E"
                      href="mailto:alinefm@linux.vnet.ibm.com">&lt;alinefm@linux.vnet.ibm.com&gt;</a><br>
                    Sent by: <a moz-do-not-send="true"
                      class="moz-txt-link-abbreviated"
                      href="mailto:kimchi-devel-bounces@ovirt.org">kimchi-devel-bounces@ovirt.org</a><br>
                    To: <a moz-do-not-send="true"
                      class="moz-txt-link-abbreviated"
                      href="mailto:harshalp@linux.vnet.ibm.com">harshalp@linux.vnet.ibm.com</a>,
                    Kimchi Devel <a moz-do-not-send="true"
                      class="moz-txt-link-rfc2396E"
                      href="mailto:kimchi-devel@ovirt.org">&lt;kimchi-devel@ovirt.org&gt;</a><br>
                    Cc:<br>
                    Subject: Re: [Kimchi-devel] [PATCH] Issue #999:
                    Attach storage to guest on s390x without libvirt<br>
                    Date: Sat, Sep 17, 2016 1:03 AM<br>
                     
                    <div><br>
                      <font face="Default Monospace,Courier
                        New,Courier,monospace" size="2">On 09/12/2016
                        06:55 AM, <a moz-do-not-send="true"
                          class="moz-txt-link-abbreviated"
                          href="mailto:harshalp@linux.vnet.ibm.com">harshalp@linux.vnet.ibm.com</a>
                        wrote:<br>
                        &gt; From: Harshal Patil <a
                          moz-do-not-send="true"
                          class="moz-txt-link-rfc2396E"
                          href="mailto:harshalp@linux.vnet.ibm.com">&lt;harshalp@linux.vnet.ibm.com&gt;</a><br>
                        &gt;<br>
                        &gt; This patch enables direct storage
                        attachment to guests running<br>
                        &gt; on s390x arch without going through libvirt<br>
                        &gt;<br>
                        &gt; Signed-off-by: Harshal Patil <a
                          moz-do-not-send="true"
                          class="moz-txt-link-rfc2396E"
                          href="mailto:harshalp@linux.vnet.ibm.com">&lt;harshalp@linux.vnet.ibm.com&gt;</a><br>
                        &gt; ---<br>
                        &gt;   docs/API.md         |  3 +++<br>
                        &gt;   i18n.py             |  3 +++<br>
                        &gt;   model/vmstorages.py | 25
                        +++++++++++++++++++++++--<br>
                        &gt;   utils.py            |  2 +-<br>
                        &gt;   4 files changed, 30 insertions(+), 3
                        deletions(-)<br>
                        &gt;<br>
                        &gt; diff --git a/docs/API.md b/docs/API.md<br>
                        &gt; index 6678ac5..c9d5d2a 100644<br>
                        &gt; --- a/docs/API.md<br>
                        &gt; +++ b/docs/API.md<br>
                        &gt; @@ -239,6 +239,9 @@ Represents a snapshot
                        of the Virtual Machine's primary monitor.<br>
                        &gt;       * path: Path of cdrom iso.<br>
                        &gt;       * pool: Storage pool which disk image
                        file locate in.<br>
                        &gt;       * vol: Storage volume name of disk
                        image.<br>
                        &gt; +    * dir_path: s390x specific attribute
                        to attach direct storage without libvirt<br>
                        &gt; +    * format: s390x specific attribute
                        specify the format of direct storage<br>
                        &gt; +    * size: s390x specific attribute to
                        specify the size of direct storage<br>
                        &gt;<br>
                        &gt;   ### Sub-resource: storage<br>
                        &gt;   **URI:**
                        /plugins/kimchi/vms/*:name*/storages/*:dev*<br>
                        &gt; diff --git a/i18n.py b/i18n.py<br>
                        &gt; index fc7dbc7..ceb4286 100644<br>
                        &gt; --- a/i18n.py<br>
                        &gt; +++ b/i18n.py<br>
                        &gt; @@ -332,6 +332,9 @@ messages = {<br>
                        &gt;       "KCHVMSTOR0016E": _("Volume already
                        in use by other virtual machine."),<br>
                        &gt;       "KCHVMSTOR0017E": _("Only one of path
                        or pool/volume can be specified to add a new
                        virtual machine disk"),<br>
                        &gt;       "KCHVMSTOR0018E": _("Volume chosen
                        with format %(format)s does not fit in the
                        storage type %(type)s"),<br>
                        &gt; +    "KCHVMSTOR0019E": _("On s390x arch one
                        of pool, path of dir_path must be specified"),<br>
                        &gt; +    "KCHVMSTOR0020E": _("On s390x arch
                        'format' must be specified while attaching disk
                        to virtual machine"),<br>
                        &gt; +    "KCHVMSTOR0021E": _("Virtual disk
                        already exists on the system: %(disk_path)s"),<br>
                        &gt;<br>
                        &gt;       "KCHSNAP0001E": _("Virtual machine
                        '%(vm)s' must be stopped before creating a
                        snapshot of it."),<br>
                        &gt;       "KCHSNAP0002E": _("Unable to create
                        snapshot '%(name)s' on virtual machine '%(vm)s'.
                        Details: %(err)s"),<br>
                        &gt; diff --git a/model/vmstorages.py
                        b/model/vmstorages.py<br>
                        &gt; index 2e9f783..156a16f 100644<br>
                        &gt; --- a/model/vmstorages.py<br>
                        &gt; +++ b/model/vmstorages.py<br>
                        &gt; @@ -17,6 +17,7 @@<br>
                        &gt;   # License along with this library; if
                        not, write to the Free Software<br>
                        &gt;   # Foundation, Inc., 51 Franklin Street,
                        Fifth Floor, Boston, MA  02110-1301 USA<br>
                        &gt;<br>
                        &gt; +import os<br>
                        &gt;   import string<br>
                        &gt;   from lxml import etree<br>
                        &gt;<br>
                        &gt; @@ -30,6 +31,7 @@ from
                        wok.plugins.kimchi.model.storagevolumes import
                        StorageVolumeModel<br>
                        &gt;   from wok.plugins.kimchi.model.utils
                        import get_vm_config_flag<br>
                        &gt;   from wok.plugins.kimchi.model.vms import
                        DOM_STATE_MAP, VMModel<br>
                        &gt;   from wok.plugins.kimchi.osinfo import
                        lookup<br>
                        &gt; +from wok.plugins.kimchi.utils import
                        create_disk_image, is_s390x<br>
                        &gt;   from wok.plugins.kimchi.xmlutils.disk
                        import get_device_node, get_disk_xml<br>
                        &gt;   from wok.plugins.kimchi.xmlutils.disk
                        import get_vm_disk_info, get_vm_disks<br>
                        &gt;<br>
                        &gt; @@ -82,11 +84,30 @@ class
                        VMStoragesModel(object):<br>
                        &gt;           # Path will never be blank due to
                        API.json verification.<br>
                        &gt;           # There is no need to cover this
                        case here.<br>
                        &gt;           if not ('vol' in params) ^
                        ('path' in params):<br>
                        &gt; -            raise
                        InvalidParameter("KCHVMSTOR0017E")<br>
                        &gt; +            if not is_s390x():<br>
                        &gt; +                raise
                        InvalidParameter("KCHVMSTOR0017E")<br>
                        &gt; +<br>
                        <br>
                        &gt; +            if 'dir_path' not in params:<br>
                        &gt; +                raise
                        InvalidParameter("KCHVMSTOR0019E")<br>
                        <br>
                        &gt;The dir_path is exclusively for s390x. You
                        need to add this verification<br>
                        &gt;too.</font></div>
                  </blockquote>
                  <div dir="ltr"><font face="Default Monospace,Courier
                      New,Courier,monospace" size="2">Not required.
                      Check the condition again (The line which says 'if
                      not is_s390x()'). Only if the platform is s390x it
                      will enter the code path where 'dir_path' is
                      checked so there is no need to check for s390x
                      arch again. I have tested this flow on x86 as well
                      as on s390x, and it works fine.</font></div>
                </div>
              </blockquote>
              <br>
              <br>
              Sorry but how the first condition is related to the second
              one?!<br>
              <br>
              If I am x86 and I have specified a 'vol' or a 'path' and
              not a 'dir_path' as it is exclusively for s390x, it will
              raise the dir_path exception (the second condition).<br>
              <br>
              So or you add a is_s390x() condition to the second
              statement or you place the whole first block in a if
              conduction:<br>
              <br>
              if not is390x():<br>
                   if not (vol in params) ^ (path in params):<br>
                      (...)<br>
              else:<br>
                    if dir_path not in params:<br>
                        raise ...<br>
              <br>
            </blockquote>
            Below is the flow:<br>
            <br>
            1) On x86, if path or volume is not present in params then
            raise Invalid parameter(<font size="2">KCHVMSTOR0017E</font>).
            (Same as it is in upstream code)<br>
            2) On x86 or s390x, if any one of them is present don't
            raise invalid parameter(<font size="2">KCHVMSTOR0017E</font>)..
            (Same as it is in upstream code)<br>
            3) On s390x, if path or volume is not present instead of
            raising invalid parameter, it should go and check for
            dir_path and if dir_path is also not present then only raise
            Invalid parameter(<font size="2">KCHVMSTOR0019E</font>).<br>
            <br>
            So on x86, if I have specified a vol or a path then it will
            not go inside first if itself(<font size="2">not ('vol' in
              params) ^ ('path' in params) will be false). <br>
              and on x86, if I have neither vol nor path then it will go
              inside first if and also second one and raise invalid
              parameter.<br>
              <br>
              And on x86, due to first nested 'if' (that is </font><font
              size="2"><font size="2">not is_s390x()) it will never go
                to second nested 'if' (</font></font><font size="2"><font
                size="2"><font size="2">'dir_path' not in params).<br>
                  <br>
                  In Summary, on x86 if I have neither vol nor path then
                  it should raise exception </font></font></font><font
              size="2"><font size="2"><font size="2"><font size="2">KCHVMSTOR0017E
                  </font>(same as currently upstream code behaves) and
                  does not proceed further.<br>
                  and on s390x we should check for vol and path, and if
                  non of it is present then instead of raising exception
                </font></font></font><font size="2"><font size="2"><font
                  size="2"><font size="2">KCHVMSTOR0017E go ahead and </font>check
                  for addition key dir_path. If dir_path is also not
                  present then only raise exception </font></font></font><font
              size="2"><font size="2"><font size="2"><font size="2">KCHVMSTOR0019E</font>.<br>
                  <br>
                  Hope I make sense to you.<br>
                </font></font></font></blockquote>
          <br>
          Yeap. That makes sense.<br>
          <br>
          But what about on x86 where I have path or vol and nor
          dir_path? How do you prevent me to go to the second if and
          raise an exception?<br>
          <br>
          The second if is not nested as you said above.<br>
        </blockquote>
        <br>
        On x86, <font face="Default Monospace,Courier
          New,Courier,monospace" size="2"> check 'not is_s390x()' will <font
            face="Default Monospace,Courier New,Courier,monospace"><font
              face="Default Monospace,Courier New,Courier,monospace">always</font>
            <font face="Default Monospace,Courier New,Courier,monospace">t</font>rue
            and exception will be raised and he<font face="Default
              Monospace,Courier New,Courier,monospace">nce it <font
                face="Default Monospace,Courier New,Courier,monospace">will
                never go <font face="Default Monospace,Courier
                  New,Courier,monospace">fu<font face="Default
                    Monospace,Courier New,Courier,monospace">rther to
                    check</font></font> </font></font></font></font><font
          face="Default Monospace,Courier New,Courier,monospace"
          size="2"><font face="Default Monospace,Courier
            New,Courier,monospace"><font face="Default Monospace,Courier
              New,Courier,monospace"><font face="Default
                Monospace,Courier New,Courier,monospace"><font
                  face="Default Monospace,Courier New,Courier,monospace"
                  size="2"><font face="Default Monospace,Courier
                    New,Courier,monospace">"</font>'dir_path' not in
                  params<font face="Default Monospace,Courier
                    New,Courier,monospace">'<font face="Default
                      Monospace,Courier New,Courier,monospace">".<br>
                      <br>
                    </font></font></font></font></font></font></font></blockquote>
      <br>
      That is not true.<br>
      <br>
      The exception on 'not is_s390x' will ONLY be raised if there is NO
      vol or path parameters. Otherwise, the flow will continue to the
      'dir_path' validation.<br>
    </blockquote>
    Aline, on x86, ppc case it will raise exception ONLY when there is
    NO vol or path parameters. <br>
    Now think about if one of them either path or vol exists, on x86,
    ppc or even on s390x case, second if (<font face="Default
      Monospace,Courier New,Courier,monospace" size="2">if 'dir_path'
      not in params:) </font>will not be reached since first if ( <font
      face="Default Monospace,Courier New,Courier,monospace" size="2">if
      not ('vol' in params) ^ ('path' in params):) will be<font
        face="Default Monospace,Courier New,Courier,monospace">come <b>false
        </b><br>
        <font face="Default Monospace,Courier New,Courier,monospace">Please
          look int<font face="Default Monospace,Courier
            New,Courier,monospace">o the tab <font face="Default
              Monospace,Courier New,Courier,monospace">indentation</font>
            of the second if and think about the logic<font
              face="Default Monospace,Courier New,Courier,monospace">
              :-)</font></font></font></font></font> <br>
    <blockquote
      cite="mid:fcf5379e-3b0f-937d-0405-15fb5a066d55@linux.vnet.ibm.com"
      type="cite">
      <blockquote
        cite="mid:1def5733-c006-ac1e-2ceb-52d367417cad@linux.vnet.ibm.com"
        type="cite"><font face="Default Monospace,Courier
          New,Courier,monospace" size="2"><font face="Default
            Monospace,Courier New,Courier,monospace"><font face="Default
              Monospace,Courier New,Courier,monospace"><font
                face="Default Monospace,Courier New,Courier,monospace"><font
                  face="Default Monospace,Courier New,Courier,monospace"
                  size="2"><font face="Default Monospace,Courier
                    New,Courier,monospace"><font face="Default
                      Monospace,Courier New,Courier,monospace"> </font></font></font></font></font></font></font>
        <blockquote
          cite="mid:6179e6aa-ad9c-29fc-2549-2c0fad1255ba@linux.vnet.ibm.com"
          type="cite"> <br>
          <blockquote
            cite="mid:27a3c938-8e03-3f60-2ebf-2685d709aba3@linux.vnet.ibm.com"
            type="cite"><font size="2"><font size="2"><font size="2"> </font></font></font><font
              face="Default Monospace,Courier New,Courier,monospace"
              size="2"><br>
            </font>
            <blockquote
              cite="mid:31ba4c43-9a32-ee62-8723-26050fdebc48@linux.vnet.ibm.com"
              type="cite">
              <blockquote
cite="mid:OF5727B727.96B7C675-ON00258033.0054FCC7-00258033.005575D0@notes.na.collabserv.com"
                type="cite">
                <div class="socmaildefaultfont" dir="ltr"
                  style="font-family:Arial;font-size:10.5pt">
                  <blockquote class="history-quote-1474298873569"
                    data-history-content-modified="1"
                    data-history-expanded="1" dir="ltr"
                    style="border-left:solid #aaaaaa 2px;
                    margin-left:5px; padding-left:5px; direction:ltr;
                    margin-right:0px">
                    <div> </div>
                    <div><br>
                      <br>
                      <font face="Default Monospace,Courier
                        New,Courier,monospace" size="2">&gt;          
                        dom = VMModel.get_vm(vm_name, self.conn)<br>
                        &gt;           params['bus'] =
                        _get_device_bus(params['type'], dom)<br>
                        &gt; -        params['format'] = 'raw'<br>
                        &gt; +<br>
                        &gt; +        if is_s390x() and params['type']
                        == 'disk':<br>
                        &gt; +            if 'format' not in params:<br>
                        &gt; +                raise
                        InvalidParameter("KCHVMSTOR0020E")<br>
                        &gt; +            if 'dir_path' in params:<br>
                        &gt; +                size = params['size']<br>
                        &gt; +                name = params['name']<br>
                        &gt; +                dir_path =
                        params.get('dir_path')<br>
                        &gt; +                params['path'] = dir_path
                        + "/" + name<br>
                        &gt; +                if
                        os.path.exists(params['path']):<br>
                        &gt; +                    raise
                        InvalidParameter("KCHVMSTOR0021E",<br>
                        &gt; +                                          
                        {'disk_path': params['path']})<br>
                        &gt; +              
                         create_disk_image(format_type=params['format'],<br>
                        &gt; +                                
                         path=params['path'], capacity=size)<br>
                        &gt; +        else:<br>
                        &gt; +            params['format'] = 'raw'<br>
                        &gt;<br>
                        &gt;           dev_list = [dev for dev, bus in
                        get_vm_disks(dom).iteritems()<br>
                        &gt;                       if bus ==
                        params['bus']]<br>
                        &gt; diff --git a/utils.py b/utils.py<br>
                        &gt; index 0fca191..7fdec02 100644<br>
                        &gt; --- a/utils.py<br>
                        &gt; +++ b/utils.py<br>
                        &gt; @@ -305,4 +305,4 @@ def
                        create_disk_image(format_type, path, capacity):<br>
                        &gt;       if rc != 0:<br>
                        &gt;           raise
                        OperationFailed("KCHTMPL0035E", {'err': err})<br>
                        &gt;<br>
                        &gt; -    return<br>
                        &gt; \ No newline at end of file<br>
                        &gt; +    return<br>
                        <br>
                        _______________________________________________<br>
                        Kimchi-devel mailing list<br>
                        <a moz-do-not-send="true"
                          class="moz-txt-link-abbreviated"
                          href="mailto:Kimchi-devel@ovirt.org">Kimchi-devel@ovirt.org</a><br>
                        <a moz-do-not-send="true"
                          href="http://lists.ovirt.org/mailman/listinfo/kimchi-devel"
                          target="_blank">http://lists.ovirt.org/mailman/listinfo/kimchi-devel</a></font><br>
                       </div>
                  </blockquote>
                  <div dir="ltr"> </div>
                </div>
                <br>
              </blockquote>
              <br>
              <br>
              <fieldset class="mimeAttachmentHeader"></fieldset>
              <br>
              <pre wrap="">_______________________________________________
Kimchi-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Kimchi-devel@ovirt.org">Kimchi-devel@ovirt.org</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://lists.ovirt.org/mailman/listinfo/kimchi-devel">http://lists.ovirt.org/mailman/listinfo/kimchi-devel</a>
</pre>
            </blockquote>
            <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>