4:34 a.m.
This patch add virNetDevGetGFeaturesSize to get the supported
gfeature size from the kernel
---
src/util/virnetdev.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..3fdf4bb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -89,8 +89,10 @@ VIR_LOG_INIT("util.netdev");
#define RESOURCE_FILE_LEN 4096
#if HAVE_DECL_ETHTOOL_GFEATURES
+#define ETH_GSTRING_LEN 32
# define TX_UDP_TNL 25
-# define GFEATURES_SIZE 2
+# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+# define FEATURE_BITS_TO_BLOCKS(n_bits) DIV_ROUND_UP(n_bits, 32U)
# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
# define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
# define FEATURE_BIT_IS_SET(blocks, index, field) \
@@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char *ifname, struct
ethtool_gfeatures *cmd)
ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
return ret;
}
+/**
+ * virNetDevGetGFeaturesSize
+ * This function return the number of gfeatures supported in
+ * the kernel
+ *
+ * @ifname: name of the interface
+ *
+ * Returns gfeature size on success, and 0 on failure or not supported.
+ */
+ static int
+virNetDevGetGFeaturesSize(const char *ifname)
+{
+ struct {
+ struct ethtool_sset_info hdr;
+ uint32_t buf[1];
+ } sset_info;
+ struct ethtool_gstrings *strings;
+ uint32_t size = 0;
+
+ sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
+ sset_info.hdr.reserved = 0;
+ sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
+
+ if (virNetDevSendEthtoolIoctl(ifname, &sset_info) == -1)
+ return size;
+ size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;
+
+ if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) + size *
ETH_GSTRING_LEN)) {
+ size = 0;
+ return size;
+ }
+
+ strings->cmd = ETHTOOL_GSTRINGS;
+ strings->string_set = ETH_SS_FEATURES;
+ strings->len = size;
+ if (size != 0 && virNetDevSendEthtoolIoctl(ifname, strings) == -1) {
+ size = 0;
+ goto cleanup;
+ }
+
+ size = FEATURE_BITS_TO_BLOCKS(size);
+
+ cleanup:
+ VIR_FREE(strings);
+ return size;
+}
+
# endif
@@ -3189,9 +3238,12 @@ virNetDevGetFeatures(const char *ifname,
# if HAVE_DECL_ETHTOOL_GFEATURES
g_cmd.cmd = ETHTOOL_GFEATURES;
- g_cmd.size = GFEATURES_SIZE;
- if (virNetDevGFeatureAvailable(ifname, &g_cmd))
- ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+ g_cmd.size = virNetDevGetGFeaturesSize(ifname);
+ if (g_cmd.size == 0)
+ VIR_DEBUG("GFeatures unsupported or failure in getting GFeatures size on
ifname %s", ifname);
+ else
+ if (virNetDevGFeatureAvailable(ifname, &g_cmd))
+ ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
# endif
if (virNetDevRDMAFeature(ifname, out) < 0)
--
1.7.1
1:27 a.m.
On 08/08/2015 05:34 AM, Moshe Levi wrote:
This patch add virNetDevGetGFeaturesSize to get the supported
gfeature size from the kernel
---
This is interesting/possibly useful, but it doesn't fix the crash that
users are experiencing. Here is a patch that should fix the crash:
https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
I would rather have that patch pushed before this one (which will mean
rebasing and resolving some merge conflicts).
src/util/virnetdev.c | 60
++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..3fdf4bb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -89,8 +89,10 @@ VIR_LOG_INIT("util.netdev");
#define RESOURCE_FILE_LEN 4096
#if HAVE_DECL_ETHTOOL_GFEATURES
+#define ETH_GSTRING_LEN 32
Where does the "32" come from? Is it from ETH_GSTRING_LEN (defined in
ethtool.h)? If so, why not just use that directly instead of #defining a
new one?
# define TX_UDP_TNL 25
I'll keep asking this every time I encounter it until I get a response -
when I look at the ethtool source files in the kernel, it looks to me
like the index for this capability should be 26 and not 25. Where/how
did you arrive at the value of 25?
-# define GFEATURES_SIZE 2
+# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+# define FEATURE_BITS_TO_BLOCKS(n_bits) DIV_ROUND_UP(n_bits, 32U)
# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
# define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
# define FEATURE_BIT_IS_SET(blocks, index, field) \
@@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char *ifname, struct
ethtool_gfeatures *cmd)
ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
return ret;
}
+/**
+ * virNetDevGetGFeaturesSize
+ * This function return the number of gfeatures supported in
+ * the kernel
+ *
+ * @ifname: name of the interface
+ *
+ * Returns gfeature size on success, and 0 on failure or not supported.
+ */
+ static int
+virNetDevGetGFeaturesSize(const char *ifname)
+{
+ struct {
+ struct ethtool_sset_info hdr;
+ uint32_t buf[1];
+ } sset_info;
+ struct ethtool_gstrings *strings;
+ uint32_t size = 0;
+
+ sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
+ sset_info.hdr.reserved = 0;
+ sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
+
+ if (virNetDevSendEthtoolIoctl(ifname, &sset_info) == -1)
+ return size;
virNetDevSendEthtoolIoctl() logs an error message, but it looks like you
want for an error to be swallowed here (just returning size = 0). If
that's the case then virNetDevSendEthtoolIoctl() needs to be reworked to
not log errors, then every caller to it needs to log the error.
+ size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;
+
+ if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) + size *
ETH_GSTRING_LEN)) {
+ size = 0;
+ return size;
+ }
+
+ strings->cmd = ETHTOOL_GSTRINGS;
+ strings->string_set = ETH_SS_FEATURES;
+ strings->len = size;
+ if (size != 0 && virNetDevSendEthtoolIoctl(ifname, strings) == -1) {
+ size = 0;
+ goto cleanup;
+ }
+
+ size = FEATURE_BITS_TO_BLOCKS(size);
+
+ cleanup:
+ VIR_FREE(strings);
+ return size;
+}
+
# endif
@@ -3189,9 +3238,12 @@ virNetDevGetFeatures(const char *ifname,
# if HAVE_DECL_ETHTOOL_GFEATURES
g_cmd.cmd = ETHTOOL_GFEATURES;
- g_cmd.size = GFEATURES_SIZE;
- if (virNetDevGFeatureAvailable(ifname, &g_cmd))
- ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+ g_cmd.size = virNetDevGetGFeaturesSize(ifname);
+ if (g_cmd.size == 0)
+ VIR_DEBUG("GFeatures unsupported or failure in getting GFeatures size on
ifname %s", ifname);
Should this failure really be ignored? Or should it lead to failure of
the entire operation?
+ else
+ if (virNetDevGFeatureAvailable(ifname, &g_cmd))
+ ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
# endif
if (virNetDevRDMAFeature(ifname, out) < 0)
2:28 a.m.
-----Original Message-----
From: sendmail [mailto:justsendmailnothingelse@gmail.com] On Behalf Of
Laine Stump
Sent: Tuesday, August 11, 2015 9:27 AM
To: libvir-list(a)redhat.com
Cc: Moshe Levi
Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
running kernel
On 08/08/2015 05:34 AM, Moshe Levi wrote:
> This patch add virNetDevGetGFeaturesSize to get the supported gfeature
> size from the kernel
> ---
This is interesting/possibly useful, but it doesn't fix the crash that users are
experiencing. Here is a patch that should fix the crash:
https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
I would rather have that patch pushed before this one (which will mean
rebasing and resolving some merge conflicts).
Ok I will rebase once you patch is merged.
> src/util/virnetdev.c | 60
++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index
> 1e20789..3fdf4bb 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -89,8 +89,10 @@ VIR_LOG_INIT("util.netdev");
>
> #define RESOURCE_FILE_LEN 4096
> #if HAVE_DECL_ETHTOOL_GFEATURES
> +#define ETH_GSTRING_LEN 32
Where does the "32" come from? Is it from ETH_GSTRING_LEN (defined in
ethtool.h)? If so, why not just use that directly instead of #defining a new
one?
It is define in ethtool-copy.h
> # define TX_UDP_TNL 25
I'll keep asking this every time I encounter it until I get a response - when I
look at the ethtool source files in the kernel, it looks to me like the index for
this capability should be 26 and not 25. Where/how did you arrive at the
value of 25?
I printed the value when I run ethtool command
moshe index 25
tx-udp_tnl-segmentation: on
moshe index 26
tx-mpls-segmentation: off [fixed]
ethtool modified code
static void dump_one_feature(const char *indent, const char *name,
const struct feature_state *state,
const struct feature_state *ref_state,
u32 index)
{
if (ref_state &&
!(FEATURE_BIT_IS_SET(state->features.features, index, active) ^
FEATURE_BIT_IS_SET(ref_state->features.features, index, active)))
return;
printf("moshe index %d\n",index);
printf("%s%s: %s%s\n",
indent, name,
FEATURE_BIT_IS_SET(state->features.features, index, active) ?
"on" : "off",
(!FEATURE_BIT_IS_SET(state->features.features, index, available)
|| FEATURE_BIT_IS_SET(state->features.features, index,
never_changed))
? " [fixed]"
: (FEATURE_BIT_IS_SET(state->features.features, index, requested)
^ FEATURE_BIT_IS_SET(state->features.features, index, active))
? (FEATURE_BIT_IS_SET(state->features.features, index, requested)
? " [requested on]" : " [requested off]")
: "");
}
Also this how I got the GFEATURES_SIZE 2
> -# define GFEATURES_SIZE 2
> +# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> +# define FEATURE_BITS_TO_BLOCKS(n_bits) DIV_ROUND_UP(n_bits,
32U)
> # define FEATURE_WORD(blocks, index, field) ((blocks)[(index) /
32U].field)
> # define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
> # define FEATURE_BIT_IS_SET(blocks, index, field) \
> @@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char
*ifname, struct ethtool_gfeatures *cmd)
> ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
> return ret;
> }
> +/**
> + * virNetDevGetGFeaturesSize
> + * This function return the number of gfeatures supported in
> + * the kernel
> + *
> + * @ifname: name of the interface
> + *
> + * Returns gfeature size on success, and 0 on failure or not supported.
> + */
> + static int
> +virNetDevGetGFeaturesSize(const char *ifname) {
> + struct {
> + struct ethtool_sset_info hdr;
> + uint32_t buf[1];
> + } sset_info;
> + struct ethtool_gstrings *strings;
> + uint32_t size = 0;
> +
> + sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
> + sset_info.hdr.reserved = 0;
> + sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
> +
> + if (virNetDevSendEthtoolIoctl(ifname, &sset_info) == -1)
> + return size;
virNetDevSendEthtoolIoctl() logs an error message, but it looks like you want
for an error to be swallowed here (just returning size = 0). If that's the case
then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors,
then every caller to it needs to log the error.
This was comment by John Ferlan he
preferred the method will return size 0 if not supported
or error. My previous patch which I send to him directly and not the ML return -1 on
failure.
But thinking about this again I don't want to swallow if error occur.
What do you think?
> + size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;
> +
> + if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) +
size * ETH_GSTRING_LEN)) {
> + size = 0;
> + return size;
> + }
> +
> + strings->cmd = ETHTOOL_GSTRINGS;
> + strings->string_set = ETH_SS_FEATURES;
> + strings->len = size;
> + if (size != 0 && virNetDevSendEthtoolIoctl(ifname, strings) == -1) {
> + size = 0;
> + goto cleanup;
> + }
> +
> + size = FEATURE_BITS_TO_BLOCKS(size);
> +
> + cleanup:
> + VIR_FREE(strings);
> + return size;
> +}
> +
> # endif
>
>
> @@ -3189,9 +3238,12 @@ virNetDevGetFeatures(const char *ifname,
>
> # if HAVE_DECL_ETHTOOL_GFEATURES
> g_cmd.cmd = ETHTOOL_GFEATURES;
> - g_cmd.size = GFEATURES_SIZE;
> - if (virNetDevGFeatureAvailable(ifname, &g_cmd))
> - ignore_value(virBitmapSetBit(*out,
VIR_NET_DEV_FEAT_TXUDPTNL));
> + g_cmd.size = virNetDevGetGFeaturesSize(ifname);
> + if (g_cmd.size == 0)
> + VIR_DEBUG("GFeatures unsupported or failure in getting
> + GFeatures size on ifname %s", ifname);
Should this failure really be ignored? Or should it lead to failure of the entire
operation?
So I guess only if I will return -1 on failure we should not ignore it
(like I mention in the comment above ) and the unsupported will be size 0
And that can be ignored.
> + else
> + if (virNetDevGFeatureAvailable(ifname, &g_cmd))
> + ignore_value(virBitmapSetBit(*out,
> + VIR_NET_DEV_FEAT_TXUDPTNL));
> # endif
>
> if (virNetDevRDMAFeature(ifname, out) < 0)
4 p.m.
On 08/11/2015 03:28 AM, Moshe Levi wrote:
> -----Original Message-----
> From: sendmail [mailto:justsendmailnothingelse@gmail.com] On Behalf Of
> Laine Stump
> Sent: Tuesday, August 11, 2015 9:27 AM
> To: libvir-list(a)redhat.com
> Cc: Moshe Levi
> Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
> running kernel
>
> On 08/08/2015 05:34 AM, Moshe Levi wrote:
>> This patch add virNetDevGetGFeaturesSize to get the supported gfeature
>> size from the kernel
>> ---
>
> This is interesting/possibly useful, but it doesn't fix the crash that users are
> experiencing. Here is a patch that should fix the crash:
>
> https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
>
> I would rather have that patch pushed before this one (which will mean
> rebasing and resolving some merge conflicts).
Ok I will rebase once you patch is merged.
Laine's patch is now pushed - I assume at least parts of this will be
necessary since there are reports of different GFEATURE_SIZE values...
...<snip>...
> virNetDevSendEthtoolIoctl() logs an error message, but it looks
like you want
> for an error to be swallowed here (just returning size = 0). If that's the case
> then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors,
> then every caller to it needs to log the error.
This was comment by John Ferlan he preferred the method will return size 0 if not
supported
or error. My previous patch which I send to him directly and not the ML return -1 on
failure.
But thinking about this again I don't want to swallow if error occur.
What do you think?
I think my non ML response to you was more to the effect of what purpose
is returning "size = 0" and it certainly wasn't perfectly clear what
size = 0 to the caller meant... Taken out of context regarding the
changes you sent me, my comment was:
"Then virNetDevGetGFeaturesSize can return -1 or a size, but it's not
clear whether "size == 0" could be true. The code only checks if size ==
-1 to fail and spits out another VIR_DEBUG message (one would already be
spit out on ioctl() failures, so that's duplicitous). So the question is
- is it possible to return size == 0 and if so, I assume that wouldn't
be good."
and to be fair I was reading that code after driving 13 hours while
moving ;-)
I do agree with some of the changes Laine posted in his first pass at
fixing some inconsistencies in the code in one large patch (which were
requested to be split up). Some issues were not a result of your
patches, but previous patches which were more or less reused. In the
long run if more patches are added to clean things up - that'll be good.
We move forward and learn from our mistakes.
John
4:24 p.m.
-----Original Message-----
From: John Ferlan [mailto:jferlan@redhat.com]
Sent: Wednesday, August 12, 2015 12:01 AM
To: Moshe Levi; Laine Stump; libvir-list(a)redhat.com
Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
running kernel
On 08/11/2015 03:28 AM, Moshe Levi wrote:
>
>
>> -----Original Message-----
>> From: sendmail [mailto:justsendmailnothingelse@gmail.com] On Behalf
>> Of Laine Stump
>> Sent: Tuesday, August 11, 2015 9:27 AM
>> To: libvir-list(a)redhat.com
>> Cc: Moshe Levi
>> Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be
>> according to running kernel
>>
>> On 08/08/2015 05:34 AM, Moshe Levi wrote:
>>> This patch add virNetDevGetGFeaturesSize to get the supported
>>> gfeature size from the kernel
>>> ---
>>
>> This is interesting/possibly useful, but it doesn't fix the crash
>> that users are experiencing. Here is a patch that should fix the crash:
>>
>> https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
>>
>> I would rather have that patch pushed before this one (which will
>> mean rebasing and resolving some merge conflicts).
>
> Ok I will rebase once you patch is merged.
Laine's patch is now pushed - I assume at least parts of this will be necessary
since there are reports of different GFEATURE_SIZE values...
Ok, Do you want me to
rebase my patch on top on this
http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c...
and fixing all Laine comments or to wait for the cleanup patch you mention below?
...<snip>...
>> virNetDevSendEthtoolIoctl() logs an error message, but it looks like
>> you want for an error to be swallowed here (just returning size = 0).
>> If that's the case then virNetDevSendEthtoolIoctl() needs to be
>> reworked to not log errors, then every caller to it needs to log the error.
> This was comment by John Ferlan he preferred the method will return
> size 0 if not supported or error. My previous patch which I send to him
directly and not the ML return -1 on failure.
> But thinking about this again I don't want to swallow if error occur.
> What do you think?
>
I think my non ML response to you was more to the effect of what purpose
is returning "size = 0" and it certainly wasn't perfectly clear what
size = 0 to the caller meant... Taken out of context regarding the
changes you sent me, my comment was:
"Then virNetDevGetGFeaturesSize can return -1 or a size, but it's not
clear whether "size == 0" could be true. The code only checks if size ==
-1 to fail and spits out another VIR_DEBUG message (one would already be
spit out on ioctl() failures, so that's duplicitous). So the question is
- is it possible to return size == 0 and if so, I assume that wouldn't
be good."
and to be fair I was reading that code after driving 13 hours while
moving ;-)
I do agree with some of the changes Laine posted in his first pass at
fixing some inconsistencies in the code in one large patch (which were
requested to be split up). Some issues were not a result of your
patches, but previous patches which were more or less reused. In the
long run if more patches are added to clean things up - that'll be good.
We move forward and learn from our mistakes.
John
4:31 p.m.
On 08/11/2015 05:24 PM, Moshe Levi wrote:
> -----Original Message-----
> From: John Ferlan [mailto:jferlan@redhat.com]
> Sent: Wednesday, August 12, 2015 12:01 AM
> To: Moshe Levi; Laine Stump; libvir-list(a)redhat.com
> Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
> running kernel
>
>
>
> On 08/11/2015 03:28 AM, Moshe Levi wrote:
>>
>>
>>> -----Original Message-----
>>> From: sendmail [mailto:justsendmailnothingelse@gmail.com] On Behalf
>>> Of Laine Stump
>>> Sent: Tuesday, August 11, 2015 9:27 AM
>>> To: libvir-list(a)redhat.com
>>> Cc: Moshe Levi
>>> Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be
>>> according to running kernel
>>>
>>> On 08/08/2015 05:34 AM, Moshe Levi wrote:
>>>> This patch add virNetDevGetGFeaturesSize to get the supported
>>>> gfeature size from the kernel
>>>> ---
>>>
>>> This is interesting/possibly useful, but it doesn't fix the crash
>>> that users are experiencing. Here is a patch that should fix the crash:
>>>
>>> https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
>>>
>>> I would rather have that patch pushed before this one (which will
>>> mean rebasing and resolving some merge conflicts).
>>
>> Ok I will rebase once you patch is merged.
>
> Laine's patch is now pushed - I assume at least parts of this will be necessary
> since there are reports of different GFEATURE_SIZE values...
Ok, Do you want me to rebase my patch on top on this
http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c...
and fixing all Laine comments or to wait for the cleanup patch you mention below?
I would say fixing a bug is more important than cleanup... Unless you
feel like taking the time and applying the changes Laine proposed in
separate patches rather than one mega patch...
If I have a few cycles I might try to do, but no guarantees ;-)
John
4:33 p.m.
On 08/11/2015 05:24 PM, Moshe Levi wrote:
> Laine's patch is now pushed - I assume at least parts of this will be necessary
> since there are reports of different GFEATURE_SIZE values...
Ok, Do you want me to rebase my patch on top on this
http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c...
and fixing all Laine comments or to wait for the cleanup patch you mention below?
I would say just rebase your patch on top of the simpler patch that I
pushed, and resend it; leave cleaning up the other nits that I found
until later (if even then). Parts of my "cleanup" had the effect of
causing a failure return any time the ioctl returned an error, which I
now see may not be the best course of action, so some of my proposed
changes are suspect at best :-)
3391
days inactive
3394
days old
6 comments
3 participants
participants (3)
-
John Ferlan -
Laine Stump -
Moshe Levi