[libvirt] Propose patch?

[Daniel P. Berrange]

On Mon, Jan 20, 2014 at 02:54:43PM +0100, Joel Simoes wrote:
> Hi, all.
> 
> I'm Joel
> 
> I wan't propose patch to correct bug to refresh volume on sheepdog
> from a sheep pool.
> Bug I'm not understanding how to configure correctly my .git/config
> to send patch.

You don't neccessarily need to change you git config file - you
can just invoke 'git send-email' with any args. As an example if
I just want to send the most recent commit that is in git I can
use '-1' as a shortcut. eg

  $ git send-email --annotate --to "libvir-list at redhat.com" --smtp-server=<hostname of SMTP serveR> --no-chain-reply-to -1

If I have created a whole series of patches, on a separate branch
then I can alternatively use

  $ git send-email --annotate --to "libvir-list at redhat.com" --smtp-server=<hostname of SMTP serveR> --no-chain-reply-to master..

which says send email for every patch that isn't present on the master
branch.

Good tip is to replace 'libvir-list at redhat.com' with your own email
address, so you can practice sending emails without accidentally
spamming the main list. Once you're comfortable with how it works
then send to the main list.

> Warning, I'm not C developper, this patch requiered correction for
> char analyse and copy. It's work but ...
> 
> My patch (add auto volumes (vdi) and refresh when adding a pool sheepdog)

Thanks for taking the effort to write a patch !

> diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
> index a6981ce..5bcad03 100644
> --- a/src/storage/storage_backend_sheepdog.c
> +++ b/src/storage/storage_backend_sheepdog.c
> @@ -34,6 +34,7 @@
>  #include "viralloc.h"
>  #include "virlog.h"
>  #include "virstring.h"
> +#include <assert.h>

We've got a policy that our code avoids any use of 'assert' and
must return errors to the user instead of aborting.

>  #define VIR_FROM_THIS VIR_FROM_STORAGE
>  
> @@ -44,6 +45,56 @@ static int virStorageBackendSheepdogRefreshVol(virConnectPtr conn,
>  void virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
>                                           virStoragePoolObjPtr pool);
>  
> +char** str_split(char* a_str, const char a_delim);
> +
> +char** str_split(char* a_str, const char a_delim) {

If you #include "virstring.h" you can access to virStringSplit
method, so you can avoid writing this str_split code.


> @@ -111,6 +162,69 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
>  
>  
>  static int
> +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> +        virStoragePoolObjPtr pool) {
> +    int ret;
> +    char *output = NULL;
> +
> +    virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL);
> +    virStorageBackendSheepdogAddHostArg(cmd, pool);
> +    virCommandSetOutputBuffer(cmd, &output);
> +    ret = virCommandRun(cmd, NULL);

You need todo

   if (ret < 0)
      goto cleanup;

otherwise you'll end up trying to 'str_split(NULL, '\n')' when
an error occurs.

> +    char** lines;
> +    lines = (char**) str_split(output, '\n');
> +    int i;

Our style rules require 'size_t i' - if you run 'make check' and
'make syntax-check' it will warn you about important style rules
we follow. the HACKING file describes some of the them in more
detail

> +    for (i = 0; *(lines + i); i++) {
> +        char *line = *(lines + i);
> +        char** cells;
> +        cells = (char**) str_split(line, ' ');
> +        int j;

Also 'size_t j' for this one'

> +        for (j = 0; *(cells + j); j++) {
> +
> +            char *cell = *(cells + j);
> +            if (j == 1) {
> +                virStorageVolDefPtr vol = NULL;
> +                if (VIR_ALLOC(vol) < 0)
> +                    goto cleanup;
> +
> +                if (VIR_STRDUP(vol->name, cell) < 0)
> +                    goto cleanup;
> +
> +                vol->type = VIR_STORAGE_VOL_BLOCK;
> +
> +
> +                if (VIR_REALLOC_N(pool->volumes.objs,
> +                        pool->volumes.count + 1) < 0)
> +                    goto cleanup;

We've got a slight preference towards 'VIR_EXPAND_N' instead
of VIR_REALLOC_N with size + 1.

> +                pool->volumes.objs[pool->volumes.count++] = vol;
> +
> +                if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
> +                    goto cleanup;
> +
> +                vol = NULL;
> +            }
> +            
> +            free(*(cells + j));
> +        }
> +
> +        free(cells);
> +        free(*(lines + i));
> +    }
> +
> +
> +    free(lines);

You need to call  VIR_FREE instead of free() for our style rules.

> +
> +    if (ret < 0)
> +        goto cleanup;

Ah, this needs to be much higher up before the loop

> +
> +
> +cleanup:
> +    virCommandFree(cmd);
> +    return ret;
> +}
> +
> +
> +static int
>  virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                       virStoragePoolObjPtr pool)
>  {
> @@ -122,9 +236,10 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>      virStorageBackendSheepdogAddHostArg(cmd, pool);
>      virCommandSetOutputBuffer(cmd, &output);
>      ret = virCommandRun(cmd, NULL);
> -    if (ret == 0)
> +    if (ret == 0){
>          ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output);

This needs to have

   if (ret < 0)
      goto cleanup;

so we skip over the refresh call on error

> -
> +        virStorageBackendSheepdogRefreshAllVol(conn, pool);
> +    }

This should set 'ret' too

>      virCommandFree(cmd);
>      VIR_FREE(output);
>      return ret;

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|