Change in vdsm[master]: Added xml option in gluster command execution.
Bala.FA has posted comments on this change.
Change subject: Added xml option in gluster command execution.
..
Patch Set 1: (1 inline comment)
File vdsm/gluster/cli.py
Line 77: msg = tree.find('opErrstr').text
Line 78: except (etree.ParseError, AttributeError):
Line 79: raise ge.GlusterXmlErrorException(err=out)
Line 80: if rv == '0':
Line 81: return (None, (tree, out))
Ok. I will raise error as exception and catch at consumer, then re-raise
properly to the user.
Line 82: else:
Line 83: return ((rv, [msg]), None)
Line 84:
Line 85:
--
To view, visit http://gerrit.ovirt.org/6999
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Bala.FA
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Timothy Asir
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Dan Kenigsberg has posted comments on this change. Change subject: Added xml option in gluster command execution. .. Patch Set 3: I would prefer that you didn't submit this -- To view, visit http://gerrit.ovirt.org/6999 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Bala.FA Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Timothy Asir Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Dan Kenigsberg has posted comments on this change.
Change subject: Added xml option in gluster command execution.
..
Patch Set 1: (1 inline comment)
File vdsm/gluster/cli.py
Line 77: msg = tree.find('opErrstr').text
Line 78: except (etree.ParseError, AttributeError):
Line 79: raise ge.GlusterXmlErrorException(err=out)
Line 80: if rv == '0':
Line 81: return (None, (tree, out))
I think that raising an exception on error would make this code more readable.
If you dislike the idea too much, then documenting the return value would go a
long way.
Line 82: else:
Line 83: return ((rv, [msg]), None)
Line 84:
Line 85:
--
To view, visit http://gerrit.ovirt.org/6999
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Bala.FA
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Timothy Asir
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added xml option in gluster command execution. .. Patch Set 3: Build Successful http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/630/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/6999 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Bala.FA Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Timothy Asir Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Bala.FA has posted comments on this change.
Change subject: Added xml option in gluster command execution.
..
Patch Set 1: (2 inline comments)
File vdsm/gluster/cli.py
Line 62: DISCONNECTED = 'DISCONNECTED'
Line 63: UNKNOWN = 'UNKNOWN'
Line 64:
Line 65:
Line 66: def _execGluster(cmd, xml=False):
Done
Line 67: if not xml:
Line 68: return utils.execCmd(cmd)
Line 69:
Line 70: cmd.append('--xml')
Line 77: msg = tree.find('opErrstr').text
Line 78: except (etree.ParseError, AttributeError):
Line 79: raise ge.GlusterXmlErrorException(err=out)
Line 80: if rv == '0':
Line 81: return (None, (tree, out))
The return value is (err, xmlout), here err=(rv, msg), xmlout=(tree, out).
Either one is set based on rv.
Issue in raising exception is that each command function should catch and raise
again with appropriate exception specific to the function.
If this is OK, I can do the change accordingly
Line 82: else:
Line 83: return ((rv, [msg]), None)
Line 84:
Line 85:
--
To view, visit http://gerrit.ovirt.org/6999
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Bala.FA
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Timothy Asir
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Dan Kenigsberg has posted comments on this change.
Change subject: Added xml option in gluster command execution.
..
Patch Set 1: (2 inline comments)
File vdsm/gluster/cli.py
Line 62: DISCONNECTED = 'DISCONNECTED'
Line 63: UNKNOWN = 'UNKNOWN'
Line 64:
Line 65:
Line 66: def _execGluster(cmd, xml=False):
the point is that there are two different functions, but they are bundled into
one python function. that's not a good practice. so, yes, please have a
_execGlusterXml() or whatever for the new functionality.
Line 67: if not xml:
Line 68: return utils.execCmd(cmd)
Line 69:
Line 70: cmd.append('--xml')
Line 77: msg = tree.find('opErrstr').text
Line 78: except (etree.ParseError, AttributeError):
Line 79: raise ge.GlusterXmlErrorException(err=out)
Line 80: if rv == '0':
Line 81: return (None, (tree, out))
sorry, I do not understand. Some time you have
(None, (tree, out))
and otherwise, in what seems like an error flow, you have
((rv, [msg]), None)
If the latter is indeed error condition, I suggest to raise an exception.
Line 82: else:
Line 83: return ((rv, [msg]), None)
Line 84:
Line 85:
--
To view, visit http://gerrit.ovirt.org/6999
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Bala.FA
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Timothy Asir
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Dan Kenigsberg has posted comments on this change. Change subject: Added xml option in gluster command execution. .. Patch Set 2: I would prefer that you didn't submit this -- To view, visit http://gerrit.ovirt.org/6999 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Bala.FA Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Timothy Asir Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added xml option in gluster command execution. .. Patch Set 2: Build Successful http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/559/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/6999 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Bala.FA Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Timothy Asir Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Bala.FA has posted comments on this change.
Change subject: Added xml option in gluster command execution.
..
Patch Set 1: (2 inline comments)
File vdsm/gluster/cli.py
Line 62: DISCONNECTED = 'DISCONNECTED'
Line 63: UNKNOWN = 'UNKNOWN'
Line 64:
Line 65:
Line 66: def _execGluster(cmd, xml=False):
Basically all gluster cli commands need to expose output as xml. This is
ongoing process and some commands are in ready state, and some or not (yet).
Eventually there will be only one behaviour of _execGluster().
If you feel to have two separate functions, I will do the change accordingly.
Please let me know.
Line 67: if not xml:
Line 68: return utils.execCmd(cmd)
Line 69:
Line 70: cmd.append('--xml')
Line 77: msg = tree.find('opErrstr').text
Line 78: except (etree.ParseError, AttributeError):
Line 79: raise ge.GlusterXmlErrorException(err=out)
Line 80: if rv == '0':
Line 81: return (None, (tree, out))
Any gluster command outputs xml with
0
0
...
...
The command specific data from xml is extracted from respective functions. If
any error in xml at command specific function (eq. volumeInfo()),
GlusterXmlErrorException is raised with the xml.
Unfortunately, its not possible to convert cElementTree object to string. So
cli output in xml is passed to command specific functions.
Line 82: else:
Line 83: return ((rv, [msg]), None)
Line 84:
Line 85:
--
To view, visit http://gerrit.ovirt.org/6999
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Bala.FA
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Timothy Asir
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Dan Kenigsberg has posted comments on this change.
Change subject: Added xml option in gluster command execution.
..
Patch Set 1: I would prefer that you didn't submit this
(2 inline comments)
File vdsm/gluster/cli.py
Line 62: DISCONNECTED = 'DISCONNECTED'
Line 63: UNKNOWN = 'UNKNOWN'
Line 64:
Line 65:
Line 66: def _execGluster(cmd, xml=False):
it is not clear to me why you overload this function with a second
functionality.
xml=True is *so* different in implementation and return value, that it seems to
deserve a completely different function.
Line 67: if not xml:
Line 68: return utils.execCmd(cmd)
Line 69:
Line 70: cmd.append('--xml')
Line 77: msg = tree.find('opErrstr').text
Line 78: except (etree.ParseError, AttributeError):
Line 79: raise ge.GlusterXmlErrorException(err=out)
Line 80: if rv == '0':
Line 81: return (None, (tree, out))
I find this return type very confusing. can you explain why there are two
types? maybe you can name them.
Line 82: else:
Line 83: return ((rv, [msg]), None)
Line 84:
Line 85:
--
To view, visit http://gerrit.ovirt.org/6999
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Timothy Asir
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Timothy Asir has posted comments on this change. Change subject: Added xml option in gluster command execution. .. Patch Set 1: Verified; Looks good to me, but someone else must approve -- To view, visit http://gerrit.ovirt.org/6999 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Timothy Asir Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added xml option in gluster command execution.
Hello Ayal Baron, Timothy Asir, Saggi Mizrahi, Federico Simoncelli, Dan Kenigsberg, I'd like you to do a code review. Please visit http://gerrit.ovirt.org/6999 to review the following change. Change subject: Added xml option in gluster command execution. .. Added xml option in gluster command execution. This helps to run gluster command which outputs in xml or standard format. Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Signed-off-by: Bala.FA --- M vdsm/gluster/cli.py M vdsm/gluster/exception.py 2 files changed, 28 insertions(+), 2 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/99/6999/1 -- To view, visit http://gerrit.ovirt.org/6999 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I385f992005d446b417be84eb7ff484e4edf6e5b6 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Timothy Asir ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
