Re: [Libguestfs] [PATCH v6 3/7] New API: yara_load

[Richard W.M. Jones]

On Thu, Apr 06, 2017 at 11:41:03PM +0300, Matteo Cafasso wrote:
> +#include 
> +
> +#define CLEANUP_DESTROY_YARA_COMPILER   \
> +  __attribute__((cleanup(cleanup_destroy_yara_compiler)))

While we should probably get rid of HAVE_ATTRIBUTE_CLEANUP, while we
still have it you need to use it here and provide the alternative for
people who don't HAVE_ATTRIBUTE_CLEANUP.

> +/* Has one FileIn parameter.
> + * Takes optional arguments, consult optargs_bitmask.
> + */
> +int
> +do_yara_load (void)
> +{
> +  int ret = 0;

You're not returning 'ret', so call it something else, eg. 'r'.

> +  CLEANUP_CLOSE int fd = -1;
> +  char tmpfile[] = "/tmp/yaraXX";
> +
> +  fd = mkstemp (tmpfile);
> +  if (fd == -1) {
> +reply_with_perror ("mkstemp");
> +return -1;
> +  }
> +
> +  ret = upload_to_fd (fd);
> +  if (ret < 0) {

upload_to_fd returns 0 or -1, so only check for r == -1.

> +static void
> +compile_error_callback(int level, const char *name, int line,
> +   const char *message, void *data)

Space before the opening parenthesis.

> +let daemon_functions = [
> +  { defaults with
> +  name = "yara_load"; added = (1, 37, 9);
> +  style = RErr, [FileIn "filename"], [];
> +  progress = true; cancellable = true;
> +  optional = Some "libyara";
> +  shortdesc = "load yara rules within libguestfs";
> +  longdesc = "\
> +Load a set of Yara rules from F within libguestfs appliance.

This is still confusingly worded, but after examining the
code I think I understand what you're trying to say.  Just
replace this first sentence with:

  "Upload a set of Yara rules from local file F.

> +Yara rules allow to categorize files based on textual or binary patterns
> +within their content.
> +See C to see how to scan files with the loaded rules.

This should be: C.  The generator will
replace Chttp://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v6 4/7] New API: yara_destroy

[Richard W.M. Jones]

On Thu, Apr 06, 2017 at 11:41:04PM +0300, Matteo Cafasso wrote:
> The yara_destroy API allows to claim resources back via the removal of
> the previously loaded Yara rules.
> 
> Signed-off-by: Matteo Cafasso 

This one looks fine, ACK.

Rich.

>  daemon/yara.c | 14 ++
>  generator/actions_yara.ml |  8 
>  generator/proc_nr.ml  |  1 +
>  lib/MAX_PROC_NR   |  2 +-
>  4 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/yara.c b/daemon/yara.c
> index 0d33d83cd..186a330c1 100644
> --- a/daemon/yara.c
> +++ b/daemon/yara.c
> @@ -107,6 +107,20 @@ do_yara_load (void)
>return (ret == ERROR_SUCCESS) ? 0 : -1;
>  }
> 
> +int
> +do_yara_destroy (void)
> +{
> +  if (rules == NULL) {
> +reply_with_error ("no yara rules loaded");
> +return -1;
> +  }
> +
> +  yr_rules_destroy (rules);
> +  rules = NULL;
> +
> +  return 0;
> +}
> +
>  /* Compile source code rules and load them.
>   * Return ERROR_SUCCESS on success, Yara error code type on error.
>   */
> diff --git a/generator/actions_yara.ml b/generator/actions_yara.ml
> index 3e55206ec..9d93d9f11 100644
> --- a/generator/actions_yara.ml
> +++ b/generator/actions_yara.ml
> @@ -45,4 +45,12 @@ it is recommended to compile them first.
> 
>  Previously loaded rules will be destroyed." };
> 
> +  { defaults with
> +name = "yara_destroy"; added = (1, 37, 9);
> +style = RErr, [], [];
> +optional = Some "libyara";
> +shortdesc = "destroy previously loaded yara rules";
> +longdesc = "\
> +Destroy previously loaded Yara rules in order to free libguestfs resources." 
> };
> +
>  ]
> diff --git a/generator/proc_nr.ml b/generator/proc_nr.ml
> index d50cc9efa..d471b1a83 100644
> --- a/generator/proc_nr.ml
> +++ b/generator/proc_nr.ml
> @@ -480,6 +480,7 @@ let proc_nr = [
>  470, "internal_find_inode";
>  471, "mksquashfs";
>  472, "yara_load";
> +473, "yara_destroy";
>  ]
> 
>  (* End of list.  If adding a new entry, add it at the end of the list
> diff --git a/lib/MAX_PROC_NR b/lib/MAX_PROC_NR
> index 68cfb10d1..8410b8b89 100644
> --- a/lib/MAX_PROC_NR
> +++ b/lib/MAX_PROC_NR
> @@ -1 +1 @@
> -472
> +473
> --
> 2.11.0

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v6 1/7] daemon: expose file upload logic

[Richard W.M. Jones]

On Thu, Apr 06, 2017 at 11:41:01PM +0300, Matteo Cafasso wrote:
> Allows other modules to use the same logic for uploading files.
> 
> Signed-off-by: Matteo Cafasso 
> ---
>  daemon/daemon.h |  3 +++
>  daemon/upload.c | 70 
> -
>  2 files changed, 42 insertions(+), 31 deletions(-)
> 
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index abec087cd..797ec2dd9 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -259,6 +259,9 @@ extern int64_t ntfs_minimum_size (const char *device);
>  extern int swap_set_uuid (const char *device, const char *uuid);
>  extern int swap_set_label (const char *device, const char *label);
> 
> +/*-- in upload.c --*/
> +extern int upload_to_fd (int fd);
> +
>  /* ordinary daemon functions use these to indicate errors
>   * NB: you don't need to prefix the string with the current command,
>   * it is added automatically by the client-side RPC stubs.
> diff --git a/daemon/upload.c b/daemon/upload.c
> index 655baf29d..144bb246c 100644
> --- a/daemon/upload.c
> +++ b/daemon/upload.c
> @@ -54,60 +54,68 @@ write_cb (void *data_vp, const void *buf, size_t len)
>return 0;
>  }
> 
> +int
> +upload_to_fd (int fd)
> +{
> +  int ret = 0, err = 0;

Let's use 'r' instead of 'ret', since it's the same as what the
previous code used and you're not actually returning 'ret'.

> +  struct write_cb_data data = { .fd = fd, .written = 0 };
> +
> +  ret = receive_file (write_cb, );
> +  if (ret == -1) {   /* write error */
> +err = errno;
> +ret = cancel_receive ();
> +errno = err;
> +reply_with_error ("write error");

Compared to the old code:

> -reply_with_error ("write error: %s", filename);

this loses the filename from the error message.  Pass the file name to
upload_to_fd to avoid losing information.

...
> -r = cancel_receive ();
> +cancel_receive ();
>  errno = err;
>  reply_with_perror ("%s", filename);
>  return -1;

and

>err = errno;
> -  r = cancel_receive ();
> +  cancel_receive ();
>errno = err;
>reply_with_perror ("lseek: %s", filename);
>return -1;

These changes are a separate bug, so they should be in a separate
commit.  More importantly than that, you should use ignore_value (...)
around cancel_receive, else Coverity will complain that the result is
not used.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs