----- Original Message -----
> On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
> >
> >
> > ----- Original Message -----
> > > On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
> > > >
> > > >
> > > > ----- Original Message -----
> > > > >
> > > > >
> > > > > ----- Original Message -----
> > > > > > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley
wrote:
> > > > > > >
> > > > > > >
> > > > > > > * You suggested using the name of the DC that SSSD is
currently
> > > > > > > connected
> > > > > > > to in the smb uri (rather than the domain.name, which
will
> > > > > > > require
> > > > > > > libsmbclient to perform a DNS resolution). Is there an
easy way
> > > > > > > to
> > > > > > > get
> > > > > > > the
> > > > > > > name of the DC that SSSD is currently connected to? I
am having
> > > > > > > trouble
> > > > > > > finding it.
> > > > > > >
> > > > > >
> > > > > > In struct ad_gpo_access_state you have a member struct
> > > > > > sdap_id_conn_ctx
> > > > > > *conn. conn->service->uri is the LDAP uri for the
current
> > > > > > connection.
> > > > > > You can use calls from OpenLDAP or ldb to split it into
components,
> > > > > > picj
> > > > > > the hostname and create the smb uri.
> > > > > >
> > > > > > In general the uri should always be available since you
read the
> > > > > > GPO
> > > > > > data from LDAP before doing the smb operations.
Nevertheless you
> > > > > > can
> > > > > > call be_resolve_server_send() to make sure it is set, see
e.g.
> > > > > > auth_get_server() how to use it.
> > > > > >
> > > > > > HTH
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > > > _______________________________________________
> > > > > > sssd-devel mailing list
> > > > > > sssd-devel(a)lists.fedorahosted.org
> > > > > >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > >
> > > > >
> > > > > I have attached a revised patch that modifies the smb uri to use
the
> > > > > server
> > > > > name rather than the domain name.
> > > > >
> > > > > Thanks,
> > > > > Yassir.
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > sssd-devel(a)lists.fedorahosted.org
> > > > >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > >
> > > > Oops. Forgot to attach the patch.
> > > >
> > > > Yassir.
> > >
> > > Thank you, the patch is working as expected and now uses the hostname to
> > > connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to
> > > split the url and take the hostname from the lud_host member of typedef
> > > struct ldap_url_desc. The LDAP url can contain port numbers which would
> > > currently cause troubles with your scheme.
> > >
> > > As a general comment, please try to split your patches into smaller
> > > units. This would help to review them especially to compare multiple
> > > versions of a patch.
> >
> > I thought you had previously indicated a preference for a single patch
> > (with the understanding that this makes it more difficult for the
> > reviewer). Perhaps I misunderstood. In any case, I have attached two
> > patches to this email: the previous patch, and a new patch to address your
> > comment about using ldap_url_parse().
>
> ah, sorry, I think I didn't explain it well. My point was to split the
> patch into smaller self contained unit, e.g. one patch for the smb
> operation and a second one for parsing the data and extracting the SID
> lists. Maybe there are even more opportunities to split the patch. Then
> each patch get its comments and the changes can be squashed into the
> patch again.
Ah, that makes sense. Thanks.
>
> >
> > >
> > > I have not looked at the child code in details yet, but I would like to
> > > suggest a change in the workflow. I think the child should only download
> > > the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and
> > > then the backend will read an process it. This way you already have the
> > > file available in the offline case. When calling the child the backend
> > > should provide the smb url and a location to store the result. The child
> > > can e.g. return a checksum for the file which the backend can save
> > > together with the download time in the sysdb cache in a subtree below
> > > cn=custom (grep sysdb.h for 'custom' to find the related sysdb
calls).
> > > With the download time it would be even possible to specific cache
> > > lifetime during which the gpo file will not be downloaded again to save
> > > bandwidth. But this should be optional.
> > >
> > > What do you think?
> > >
> > > bye,
> > > Sumit
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel(a)lists.fedorahosted.org
> > >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> >
> > With regard to your suggestion to store the policy data in a local file
> > (and store the file's metadata in the cache), I think it would be
> > preferrable to have the child parse the file, and store the relevant
> > data/metadata directly to the cache. I believe this is how IPA HBAC
> > currently works (i.e. storing to cache, rather than to file). This would
> > allow the file parsing to be done once (when it is written by the child),
> > rather than the file parsing being done every time the policy is processed
> > (when it is read by the backend). Additionally, keep in mind that we are
> > interested in only a subset of the stanzas (i.e. "Privilege Rights"
> > stanza) in the Security Settings policy file (GptTmpl.inf). In other
> > words, only a small portion of the file contains data that is relevant for
> > us.
> >
> > For this initial patch, I am having the child return the allowed_sids and
> > denied_sids to the backend for further processing. However, in subsequent
> > patches, I am planning on having the child store the relevant policy data
> > to the cache, and then have the backend retrieve the relevant policy data
> > from the cache. In other words, the proposed logic for future patches is:
> > * if we are online, the backend calls the child (which stores the parsed
> > data to cache)
> > * regardless of whether we are online or offline, the backend retrieves the
> > parsed data from the cache and makes a policy decision.
> >
> > With regard to reducing unnecessary downloads, I am planning on
> > implementing the logic suggested by [MS-GPOL] of comparing the cached gpo
> > version against the current gpo version (by downloading only the GPT.INI
> > file). If the gpo versions are the same, there is no need to download the
> > policy file. Again, this is intended for a subsequent patch.
> >
> > Regards,
> > Yassir.
>
> > From c2047c17b1fc8dc6ab0df337a3a0fa0af9cc22e4 Mon Sep 17 00:00:00 2001
> > From: Yassir Elley <yelley(a)redhat.com>
> > Date: Wed, 18 Jun 2014 13:42:50 -0400
> > Subject: [PATCH 2/2] Use ldap_url_parse to extract hostname from ldap uri
> >
>
> ACK
>
> > From d0e3541cd2c0be7ca6c511bc40dd37fe569c3a29 Mon Sep 17 00:00:00 2001
> > From: Yassir Elley <yelley(a)redhat.com>
> > Date: Wed, 21 May 2014 15:11:36 -0400
> > Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child
> > process
> >
> > ---
>
> ...
>
> > +/* == ad_gpo_process_cse_send/recv helpers
> > ================================= */
> > +
> > +static errno_t
> > +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
> > + char *smb_uri,
> > + struct io_buffer **io_buf)
> > +{
> > + struct io_buffer *buf;
> > + size_t rp;
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
> > + DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %zu\n",
strlen(smb_uri));
> > +
> > + buf = talloc(mem_ctx, struct io_buffer);
> > + if (buf == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
> > + return ENOMEM;
> > + }
> > +
> > + buf->size = 1 * sizeof(uint32_t);
> > + if (smb_uri) {
> > + buf->size += strlen(smb_uri);
> > + }
> > +
> > + DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
> > +
> > + buf->data = talloc_size(buf, buf->size);
> > + if (buf->data == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > + talloc_free(buf);
> > + return ENOMEM;
> > + }
> > +
> > + rp = 0;
> > + /* smb_uri */
> > + if (smb_uri) {
> > + SAFEALIGN_SET_UINT32(&buf->data[rp], strlen(smb_uri),
&rp);
> > + safealign_memcpy(&buf->data[rp], smb_uri, strlen(smb_uri),
&rp);
>
> you are calling strlen(smb_uri) 4 time. Although it is a highly
> optimized call it will need some time and since I guess that compilers
> are not smart enough to optimize this into a single call I would suggest
> to put the length into variable and calculate it only once.
>
> > + } else {
> > + SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
> > + }
> > +
> > + *io_buf = buf;
> > + return EOK;
> > +}
> > +
> > +static errno_t
> > +parse_gpo_child_response(TALLOC_CTX *mem_ctx,
> > + uint8_t *buf, ssize_t size,
> > + char ***_allowed_sids,
> > + int *_allowed_size,
> > + char ***_denied_sids,
> > + int *_denied_size)
> > +{
> > + size_t p = 0;
> > + uint32_t res;
> > + int allowed_size = 0;
> > + int denied_size = 0;
> > + int i = 0;
> > + int sid_len = 0;
> > + char **allowed_sids;
> > + char *allowed_sid;
> > + char **denied_sids;
> > + char *denied_sid;
> > +
> > + /* operation result code */
> > + SAFEALIGN_COPY_UINT32_CHECK(&res, buf + p, size, &p);
> > +
> > + /* allowed_size */
> > + SAFEALIGN_COPY_UINT32_CHECK(&allowed_size, buf + p, size, &p);
> > + DEBUG(SSSDBG_TRACE_FUNC, "child response allowed_size: %d\n",
> > allowed_size);
> > +
> > + allowed_sids = talloc_array(mem_ctx, char *, allowed_size + 1);
>
> Why do you need '+ 1'? If you planned a NULL-terminated array you forgot
> to set the last element to NULL.
>
> > + if (allowed_sids == NULL) {
> > + return ENOMEM;
> > + }
> > +
> > + for (i = 0; i < allowed_size; i++) {
> > + SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
> > + if ((p + sid_len ) > size) return EINVAL;
> > + allowed_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
> > + if (allowed_sid == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > + return ENOMEM;
> > + }
> > + safealign_memcpy(allowed_sid, buf+p, sizeof(char) * sid_len, &p);
> > + allowed_sid[sid_len] = '\0';
> > +
> > + allowed_sids[i] = talloc_strdup(allowed_sids, allowed_sid);
> > + if (allowed_sids[i] == NULL) {
> > + return ENOMEM;
> > + }
>
> I think you can simplify this by calling talloc_strndup() to copy the
> data from the buffer and advance p manually.
>
> > + }
> > +
> > + /* denied_size */
> > + SAFEALIGN_COPY_UINT32_CHECK(&denied_size, buf + p, size, &p);
> > + DEBUG(SSSDBG_TRACE_FUNC, "child response denied_size: %d\n",
> > denied_size);
> > +
> > + denied_sids = talloc_array(mem_ctx, char *, denied_size + 1);
>
> see above
>
> > + if (denied_sids == NULL) {
> > + return ENOMEM;
> > + }
> > +
> > + for (i = 0; i < denied_size; i++) {
> > + SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
> > + if ((p + sid_len ) > size) return EINVAL;
> > + denied_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
> > + if (denied_sid == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > + return ENOMEM;
> > + }
> > + safealign_memcpy(denied_sid, buf+p, sizeof(char) * sid_len, &p);
> > + denied_sid[sid_len] = '\0';
> > +
> > + denied_sids[i] = talloc_strdup(denied_sids, denied_sid);
> > + if (denied_sids[i] == NULL) {
> > + return ENOMEM;
> > + }
>
> see above
>
> > + }
> > +
> > + *_allowed_size = allowed_size;
> > + *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
>
> you already allocated allocated_sids on mem_ctx. Nevertheless I think it
> would be a good idea to use a tmp_ctx here and free all allocated memory
> in the case of an error.
>
> > + *_denied_size = denied_size;
> > + *_denied_sids = talloc_steal(mem_ctx, denied_sids);
> > +
> > + return EOK;
> > +}
> > +
> > +
>
> ...
>
> > +static errno_t
> > +unpack_buffer(uint8_t *buf,
> > + size_t size,
> > + struct input_buffer *ibuf)
> > +{
> > + size_t p = 0;
> > + uint32_t len;
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "total buffer size: %zu\n", size);
> > +
> > + /* smb_uri size and length */
> > + SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "smb_uri size: %d\n", len);
> > + if (len) {
> > + if ((p + len ) > size) return EINVAL;
> > + ibuf->smb_uri = talloc_strndup(ibuf, (char *)(buf + p), len);
> > + if (ibuf->smb_uri == NULL) return ENOMEM;
> > + DEBUG(SSSDBG_TRACE_FUNC, "got smb_uri: %s\n",
ibuf->smb_uri);
> > + p += len;
> > + }
>
> If I see is correctly you continue processing even if there is no
> smb_uri maybe and error should be returned here if len == 0?
>
> > +
> > + return EOK;
> > +}
> > +
> > +
> > +static errno_t
> > +pack_buffer(struct response *r,
> > + int result,
> > + int allowed_size,
> > + char **allowed_sids,
> > + int denied_size,
> > + char **denied_sids)
> > +{
> > + int len = 0;
> > + size_t p = 0;
> > + int i;
> > + int sid_len = 0;
> > +
> > + /* A buffer with the following structure must be created:
> > + * int32_t status of the request (required)
>
> uint32_t
>
> > + * int32_t allowed_size (required)
> > + * uint8_t[allowed_size] (optional if allowed_size == 0)
>
> the description is a bit misleading, you have a uint32_t sid_size,
> uint8_t[sid_size] and this allowed_size times. I currently do not have a
> good idea how to write it better.
>
> > + * int32_t denied_size (required)
> > + * uint8_t[denied_size] (optional if denied_size == 0)
> > + */
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "entering pack_buffer\n");
> > +
> > + for (i = 0; i < allowed_size; i++) {
> > + len += strlen(allowed_sids[i]);
> > + }
> > +
> > + for (i = 0; i < denied_size; i++) {
> > + len += strlen(denied_sids[i]);
> > + }
> > +
> > + r->size = (3 + allowed_size + denied_size) * sizeof(uint32_t) + len;
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "response size: %zu\n",r->size);
> > +
> > + r->buf = talloc_array(r, uint8_t, r->size);
> > + if(r->buf == NULL) {
> > + return ENOMEM;
> > + }
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC,
> > + "result [%d] allowed_size [%d] denied_size [%d]\n",
> > + result, allowed_size, denied_size);
> > +
> > + /* result */
> > + SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
> > +
> > + /* allowed_size */
> > + SAFEALIGN_SET_UINT32(&r->buf[p], allowed_size, &p);
> > +
> > + /* allowed_sids */
> > + for (i = 0; i < allowed_size; i++) {
> > + sid_len = strlen(allowed_sids[i]);
> > + SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
> > + safealign_memcpy(&r->buf[p], allowed_sids[i], sid_len,
&p);
> > + }
> > +
> > + /* denied_size */
> > + SAFEALIGN_SET_UINT32(&r->buf[p], denied_size, &p);
> > +
> > + /* denied_sids */
> > + for (i = 0; i < denied_size; i++) {
> > + sid_len = strlen(denied_sids[i]);
> > + SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
> > + safealign_memcpy(&r->buf[p], denied_sids[i], sid_len, &p);
> > + }
> > +
> > + return EOK;
> > +}
> > +
>
> ...
>
> > +int
> > +main(int argc, const char *argv[])
> > +{
> > + int opt;
> > + poptContext pc;
> > + int debug_fd = -1;
> > + errno_t ret;
> > + int result;
> > + TALLOC_CTX *main_ctx = NULL;
> > + uint8_t *buf = NULL;
> > + ssize_t len = 0;
> > + struct input_buffer *ibuf = NULL;
> > + struct response *resp = NULL;
> > + size_t written;
> > + char **allowed_sids;
> > + int allowed_size;
> > + char **denied_sids;
> > + int denied_size;
> > + int j;
> > +
> > + struct poptOption long_options[] = {
> > + POPT_AUTOHELP
> > + {"debug-level", 'd', POPT_ARG_INT, &debug_level,
0,
> > + _("Debug level"), NULL},
> > + {"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps,
0,
> > + _("Add debug timestamps"), NULL},
> > + {"debug-microseconds", 0, POPT_ARG_INT,
&debug_microseconds, 0,
> > + _("Show timestamps with microseconds"), NULL},
> > + {"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
> > + _("An open file descriptor for the debug logs"), NULL},
> > + POPT_TABLEEND
> > + };
> > +
> > + /* Set debug level to invalid value so we can decide if -d 0 was used.
> > */
> > + debug_level = SSSDBG_INVALID;
> > +
> > + pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> > + while((opt = poptGetNextOpt(pc)) != -1) {
> > + switch(opt) {
> > + default:
> > + fprintf(stderr, "\nInvalid option %s: %s\n\n",
> > + poptBadOption(pc, 0), poptStrerror(opt));
> > + poptPrintUsage(pc, stderr, 0);
> > + _exit(-1);
> > + }
> > + }
> > +
> > + poptFreeContext(pc);
> > +
> > + DEBUG_INIT(debug_level);
> > +
> > + debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]",
> > getpid());
> > + if (debug_prg_name == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
> > + goto fail;
> > + }
> > +
> > + if (debug_fd != -1) {
> > + ret = set_debug_file_from_fd(debug_fd);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd
> > failed.\n");
> > + }
> > + }
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
> > +
> > + main_ctx = talloc_new(NULL);
> > + if (main_ctx == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
> > + talloc_free(discard_const(debug_prg_name));
> > + goto fail;
> > + }
> > + talloc_steal(main_ctx, debug_prg_name);
> > +
> > + buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
> > + if (buf == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > + goto fail;
> > + }
> > +
> > + ibuf = talloc_zero(main_ctx, struct input_buffer);
> > + if (ibuf == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
> > + goto fail;
> > + }
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
> > +
> > + errno = 0;
> > + len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
> > + if (len == -1) {
> > + ret = errno;
> > + DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret,
> > strerror(ret));
> > + goto fail;
> > + }
> > +
> > + close(STDIN_FILENO);
> > +
> > + ret = unpack_buffer(buf, len, ibuf);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE,
> > + "unpack_buffer failed.[%d][%s].\n", ret,
strerror(ret));
> > + goto fail;
> > + }
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
> > +
> > + result = process_security_settings_cse(main_ctx,
> > + ibuf->smb_uri,
> > + &allowed_sids,
> > + &allowed_size,
> > + &denied_sids,
> > + &denied_size);
> > + if (result != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE,
> > + "process_security_settings_cse failed.[%d][%s].\n",
> > + ret, strerror(ret));
> > + goto fail;
> > + }
> > +
> > + DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n",
allowed_size);
> > + for (j= 0; j < allowed_size; j++) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
> > + allowed_sids[j]);
> > + }
> > +
> > + DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
> > + for (j= 0; j < denied_size; j++) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
> > + denied_sids[j]);
> > + }
> > +
> > + ret = prepare_response(main_ctx, result, allowed_size, allowed_sids,
> > denied_size, denied_sids, &resp);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed.
[%d][%s].\n",
> > + ret, strerror(ret));
> > + goto fail;
> > + }
> > +
> > + errno = 0;
> > + DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n",
resp->size);
> > +
> > + written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
> > + if (written == -1) {
> > + ret = errno;
> > + DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
> > + strerror(ret));
> > + goto fail;
> > + }
> > +
> > + if (written != resp->size) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote
> > %zu\n",
> > + resp->size, written);
> > + goto fail;
> > + }
> > +
> > + DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
> > + close(STDOUT_FILENO);
> > + talloc_free(main_ctx);
> > + return EXIT_SUCCESS;
> > +
> > +fail:
> > + DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
> > + close(STDOUT_FILENO);
>
> just a nitpick which needs no fixing. There are 'goto fail' before
> STDIN_FILENO was closed. So in theory you might want to close this here
> as well. But since close() will fail if STDIN_FILENO is already closed
> it is not worth the effort to detect all of this properly since the
> program exits anyway.
>
> > + talloc_free(main_ctx);
> > + return EXIT_FAILURE;
> > +}
> > --
> > 1.8.5.3
> >
>
> bye,
> Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
I have attached a revised first patch which addresses the code review comments. I'm
not sure if I needed to (since it has already been ACK'ed), but I have also attached
an unchanged second patch.
Thanks,
Yassir.